search

libretro / libretro-handy

K. Wilkins' Atari Lynx emulator Handy (http://handy.sourceforge.net/) for libretro
13 stars 35 forks source link

Add OpenDingux target #82

Closed jdgleaver closed 3 years ago

jdgleaver commented 3 years ago

This PR adds an OpenDingux target to the makefile.

It also adds a workaround for a specific hardware limitation of OpenDingux:

In addition, this PR disables the 24bit display mode on OpenDingux, since it is incredibly slow, and toggling the bit depth can cause crashes (this seems to happen on Linux, too, with several gfx drivers - I am not certain of the underlying cause...)

Tested on an RG350M, most games run full speed with a 3x upscaling video filter. Performance is vastly superior to beetle-lynx-libretro.

SimpleTease commented 3 years ago

In addition, this PR disables the 24bit display mode on OpenDingux, since it is incredibly slow, and toggling the bit depth can cause crashes (this seems to happen on Linux, too, with several gfx drivers - I am not certain of the underlying cause...)

(Ruminating about a future commit)

Maybe we should default disallow 24bpp and manually allow it via a define flag for safe systems. There's some platforms (iirc) which can only handle 16bpp.

Also want to voice a concern about real-time bit depth toggling, since there are known buggy and unstable hardware video drivers that behave erratically on screen switches. I think it may require a hard core shutdown though, since soft resets via Retroarch may not cut it.

I've seen the above 2 concerns in several libretro cores.

When requesting a display mode of width 102 (or an integer multiple thereof), the OpenDingux OS crashes (this is not an error we can catch - the OS straight up dies).

Would it be more convenient to catch this workaround in the official Retroarch frontend for OpenDingux - to avoid patching future cores with this curious and dangerous quirk?

I get it now, it's a pitch alignment issue. Would OpenDingux work if height is aligned to 104? Similar to how PS Classic and Android Shield want horizontal alignment of 4 bytes?

jdgleaver commented 3 years ago

Maybe we should default disallow 24bpp and manually allow it via a define flag for safe systems. There's some platforms (iirc) which can only handle 16bpp.

I would agree with that completely

Also want to voice a concern about real-time bit depth toggling, since there are known buggy and unstable hardware video drivers that behave erratically on screen switches. I think it may require a hard core shutdown though, since soft resets via Retroarch may not cut it.

Yes, I think reverting to the old behaviour of requiring a core restart (hard shutdown, as you say) would be a good idea

I get it now, it's a pitch alignment issue. Would OpenDingux work if height is aligned to 104? Similar to how PS Classic and Android Shield want horizontal alignment of 4 bytes?

Ah - 4 byte alignment - yes, I need to try that. The problem is that it's really unclear what requirements OpenDingux actually has. There is no documentation, and it's only by trial and error that I've found problematic resolutions (very few cores are affected - VICE is another rare example, where its zoom settings also crash the OS). I will investigate this further, but there is a performance issue here - if we pad on the frontend side then we have to use a slower blitting method, which is worrisome.

SimpleTease commented 3 years ago

I will investigate this further, but there is a performance issue here - if we pad on the frontend side then we have to use a slower blitting method, which is worrisome.

I realized that part about frontend doing more memcpy and thought Oops! Bad idea. Strikethough!. Having to do every frame would buzz kill the fps. :ouch:

Actually you could apply the forced alignment for all platforms and remove the safety guards for DINGUX. I see this done for Stella and Snes9x cores, plus I've got a feeling more future devices will use minimum bus 32-bit alignment as a performance optimization.

edit: There is no documentation, and it's only by trial and error that I've found problematic resolutions (very few cores are affected - VICE is another rare example, where its zoom settings also crash the OS).

If it is 4-byte boundary, then heights of 101,102,103 should OS crash.

jdgleaver commented 3 years ago

Actually you could apply the forced alignment for all platforms and remove the safety guards for DINGUX. I see this done for Stella and Snes9x cores, plus I've got a feeling more future devices will use minimum bus 32-bit alignment as a performance optimization.

I like the way you think :)

I'm out of time for today, but I'll do this tomorrow and try applying it to the VICE core as well

If it is 4-byte boundary, then heights of 101,102,103 should OS crash.

Thanks, I'll test this!

inactive123 commented 3 years ago

Yeah I was never a fan of the core option bitdepth option for some of these cores. If it allows for less color banding I would see that being a good thing to have it as an option, if it won't make a difference though then a higher bitdepth will just be slower and it means we can optimize less around a specific bitdepth.

SimpleTease commented 3 years ago

For Handy, all I can think higher bitdepth (555 -> 888) being useful is

To soften 565 color banding would be great, but I don't know who does that.

Beetle-Ngp = mmm.. Beetle-Wswan = mmm.. Beetle-Vb = uses 888 internally for anaglyphs and gamma brightness, so I guess it makes sense there.

Can't remember where else the bit depth switch is used. But even for Vb, default 16-bpp would seemingly be helpful all-around.

edit: Still I wonder how useful the depth switch is, like having variable audio rates at run-time. Slower devices would likely be happy with a fixed value of 48000 and the compiler can optimize around it. But it's interesting to hear opinions not my own.

Vice does have switches for both, but depth requires a core restart thankfully.

jdgleaver commented 3 years ago

@SimpleTease You were spot on about the OpenDingux byte alignment - but it's even worse! The video mode width also has to be a multiple of 16, otherwise the hardware video scaler glitches out and starts wrapping pixel columns around the edge of the screen.

I've fixed this in the frontend, and since it's such an obscure issue I think I'll remove the DINGUX padding hack from this core. I know you said forced alignment is a good idea (and I agree!), but there is a downside to doing this: while the raw image output looks fine, applying a shader or video filter results in columns of 'garbage' either side of the image (the pixels themselves are black, but some shaders/filters put a grid around them, or otherwise adjust the colour, and I think that looks bad...)

As for the bit depth - I agree very strongly with you that 24bit mode is absolutely pointless for most of these cores, and should only be included as a fallback if RGB565 is unsupported (if that is even possible...). Computer cores (VICE, PUAE, etc.) and 'modern' systems do benefit from reduced colour banding with 24bit (the CD32 boot animation is a good test of this!), but for the beetle cores you listed there is no tangible effect other than a massive performance hit. Even considering colour correction, for example - in the Gambatte and mGBA cores I do this in RGB565 colour space and the results are absolutely fine (virtually indistinguishable from using a colour correction shader).

So I will certainly put the 24bit option of this core behind a general compiler flag, and leave it disabled by default (it can be added per-system, if required). And I'll also make the option require a restart :)

SimpleTease commented 3 years ago

Wow! That's strict. I read your frontend PR fix and it does make sense now. https://github.com/libretro/RetroArch/pull/11436

PS Classic and Shield though may have the pixel skewing problems during rotate since 102 is not 4-byte aligned. But I'm not testing those. :meh:

edit: Got a new Dingux question. SDL video mode must be 16-byte aligned. But blitting a texture to that video surface can be any flexible dimension, or that must be aligned also?

jdgleaver commented 3 years ago

PS Classic and Shield though may have the pixel skewing problems during rotate since 102 is not 4-byte aligned. But I'm not testing those. :meh:

Yes, I think if the maintainers of those ports find similar issues with the 102 width, they should PR fixes to the frontend. I wouldn't expect you to test that, not at all :)

edit: Got a new Dingux question. SDL video mode must be 16-byte aligned. But blitting a texture to that video surface can be any flexible dimension, or that must be aligned also?

Once we've managed to successfully create an SDL surface, the hard part is done - after that, it's just a data array, and we can copy anything we want to it. So yes, the texture we blit can have any dimensions. If the pitch matches, we can memcpy() in one go - otherwise we memcpy() line by line, with a padding offset if the surface width had to be rounded up.

On another note - I'm trying to implement the colour depth changes, but it seems the 24bit option doesn't actually work at all. The pitch is all wrong (there's a 1 pixel gap between each column of display pixels), and rotating the screen generates a mysterious segfault. This happens with all gfx drivers on Linux. So I was wondering - does the 24bit option work for you? It's kinda looking like it was never tested...

SimpleTease commented 3 years ago

Windows. 16bit + 24bit + rotate = no glitch, no crash.

jdgleaver commented 3 years ago

Thanks for testing!

Hmm... This almost seems like a stack corruption issue - I've seen this in several other cores, where the effects only manifest on one platform due to different memory management... I'll try to investigate...

jdgleaver commented 3 years ago

Ah, turns out it was a stupid error in the core - it's using poorly defined data types which have different widths on different platforms. On Windows, a ULONG is 32 bit - on Linux, it's 64 bit. Since these data widths are used directly when manipulating the video buffer pointer, everything explodes on Linux... :)

This is very easy to fix - I'll include it in my forthcoming PR...

jdgleaver commented 3 years ago

Okay, this should address all the issues: https://github.com/libretro/libretro-handy/pull/83

SimpleTease commented 3 years ago

Great catch!