ekeeke / Genesis-Plus-GX

An enhanced port of Genesis Plus - accurate & portable Sega 8/16 bit emulator
Other
699 stars 202 forks source link

(libretro) Core-option for PAR core-provided aspect ratio #104

Closed arakerlu closed 8 years ago

arakerlu commented 8 years ago

I'd like to be able to set a core-option to get a (resolution independent) constant pixel aspect ratio for the core-provided aspect ratio given by Genesis Plus GX libretro module.

For example in Nestopia core you have two options for core-provided aspect ratio, 4:3 and PAR 8:7, where the former means "always 4:3 display aspect ratio" and the latter "always 8:7 pixel aspect ratio". The difference effectively is, that with "8:7 PAR" the ratio of individual pixels, and more importantly the objects created by those pixels in the screen, stay constant whatever the resolution of the total displayed area, whereas with "4:3" the display area is always squeezed or stretched to be in 4:3 whatever the resolution.

With NES this even isn't that big of a problem, because you have only one resolution, 256x240. Nestopia core has core-option to display or to not display horizontal and/or vertical overscan area, which changes the display resolution a bit.

Systems supported by Genesis Plus GX, SMS and Genesis forefront, on the other hand, have numerous possible resolutions: at least 256x192 or 256x224, 320x224 and 320x240. Currently there's no way to influence what the core-provided aspect ratio is, it's always 4:3 (DAR). For some resolutions it looks ok, but for other it doesn't. Especially the SMS resolutions (256 px wide) suffer from it and look badly stretched vertically. You can set the core-option for the borders to be visible, which makes the source resolution always either 284x240 or 348x240. That's if the game is in NTSC system region, for PAL the resolution is either 284x288 or 348x288. This actually works and the SMS resolutions look ok, but now you have the shrunken active display area.

For Genesis the PAR depends on two independent varibles, system region (PAL or NTSC) and horizontal resolution (256px or 320px). That means that even in NTSC region, the PAR is different if the resolution is 256px or 320px, so it wouldn't be correct to call the option as any constant PAR. Even in NES there actually does exists the system region variable, so having only one PAR (8:7 PAR) addresses only one system region (NTSC). 8:7 PAR would actually also work in NTSC Genesis and SMS for the 256px wide resolutions, but for 320px wide it would look horizontally stretched.

Thus, in order for the core-option for setting the core-provided aspect ratio, I suggest at least three options: "4:3", "NTSC PAR" and "PAL PAR".

arakerlu commented 8 years ago

Sorry for the wall of text, but I wanted to address this enhancement as well as possible. I'm a software developer myself and I'm willing to do the implementation work, but I may need help in some aspects. I submitted this ticket because I thought that this would be the best place to discuss about it.

I have already done some initial testing and committed the results in my fork of the core. See commits: https://github.com/arakerlu/Genesis-Plus-GX/commit/5ebded918122778f909b43fd34ce30e17146aef3 https://github.com/arakerlu/Genesis-Plus-GX/commit/14acf9c6a9d8de6f4b8d16621f0b20f16051dc99

ekeeke commented 8 years ago

No problem, I agree this is one of the things missing in libretro port compared to standalone Wii/GC version, which is made confusing by the fact libretro API expects a display aspect ratio while it should rather ask for core pixel aspect ratio since it already has display width/height. I am also not sure if libretro frontends adjust the scaling regarding host system PAR to maintain correct aspect ratio on platforms using non-square pixels (like the Wii). Similarly, I am not sure how it would deal with overscan when emulated (for example, my TV crops approximately 8+8 lines of overscan in 60hz mode and 10+10 lines in 50 hz mode, while cropping all left/right pixels of overscan so 320x224 frame would be displayed fullscreen in 4:3 mode at 60hz and with approx. 22+22 lines of overscan in 50 hz mode).

Now, the obtained values seem correct however I don't really follow your calculations and why you are using NTSC and PAL standard square pixel resolution for them, it's much more simpler to divide industry-standard square (1:1) pixel clocks (which are indeed 12,27 MHz for NTSC and 14,5 MHz for PAL) by emulated system pixel clock, which is master_clock/10 in H32 mode and master_clock/8 in H40 mode (it does not matter if dot clock is variable in this mode because that only happens for non-visible pixels).

The results should then be divided by 2 in non-interlaced mode, which give exactly your own calculated values but this should NOT be the case in interlaced mode 2 when 448 lines are rendered by the core so it would be more correct to apply this division when calculating the returned DAR, if necessary (you can test Sonic 2 VS mode with core setting "interlace mode 2 output" set to "double field").

I also don't understand why you made this as a core option. As you figured, returning 4:3 for everything is not correct anyway so it would make more sense if it was automatically calculated depending on current emulated system mode (h32/h40) and master clock (preferably using directly system_clock variable because it is automatically initialized by the emulator and could let you emulate a PAL console switched to 60Hz).

For GG and GGMS, I assume you are trying to simulate aspect ratio of LCD screen but this should not be applied when overscan is emulated because in that case the emulator renders the screen as if Game Gear was connected to a TV (via GGTV adapter) so usual PAR should be used. For GGMS, cropping of master system screen is only applied in LCD mode and is a little bit more complicated than your implementation (see http://www.smspower.org/forums/9562-GameGearSMSModeVideoScaling) as it requires some horizontal filtering to be really accurate.

arakerlu commented 8 years ago

Yes, that would make sense that Retroarch took pixel aspect ratio instead of display aspect ratio. In the end it's of course just simple mathematics to convert PAR to DAR, if you have the display height and width.

The calculations in source code (and the comments) are based on the method described in the nesdev article, and it indeed is more complex that required. Since then I have discovered the method you described later (that's a few days ago, I've been researching this subject for a few weeks now) and both methods indeed bring exactly same values, so both are good in that respect.

Sure there's no need for this to be core-option. I'm just usually oriented to maintain backwards compatibility, especially when I'm not the architect. That's also how some of the other cores work, so I thought it's the standard way. But if you think there's no need to keep the old aspect ratio even as an option, then I'm not going oppose it.

However, being able to force using NTSC PAR might make some sense, because almost without exception all games look more correct in NTSC. There are even some PAL exclusives with no NTSC version available, such as Micro Machines for SMS, that look better if you use NTSC PAR. With Micro Machines you can see that the cars etc look wrong even if you look it with just you eye, but in this case you can verify it by comparing it to the almost identical Game Gear version. The versions look same when you force the SMS version to NTSC PAR.

What comes to the Game Gear. I didn't mention it in this ticket because with it things are more compicated. But as you noticed I've been testing it as well and the 6:5 PAR and horizontal cropping is indeed for replicating the looks of Game Gear in LCD mode. I'm aware of the forum post about Game Gear's downscaling algorithm backward engineering you linked. At some point I actually considered implementing it as well, but after looking a few hours into it, it seemed a bit too complicated a task. Luckily the 6:5 PAR applies to both GG and GGMS when in LCD mode so, for now, that's good enough for me - even if it doesn't match 1:1 to the original (which should be the target of course).

Thanks for your comments. I'll return to you by a new comment in this ticket once I've done the suggested changes and fixed the issues.

ekeeke commented 8 years ago

The calculations in source code (and the comments) are based on the method described in the nesdev article, and it indeed is more complex that required. Since then I have discovered the method you described later (that's a few days ago, I've been researching this subject for a few weeks now) and both methods indeed bring exactly same values, so both are good in that respect.

Actually, your PAR calculations are off by a factor 2, as explained earlier. DAR should rather be divided by 2 when calculated (or more exactly screen height used for its calculation multiplied by 2) for non-interlaced content (since on TV screen, lines are more spaced in this case).

Sure there's no need for this to be core-option. I'm just usually oriented to maintain backwards compatibility, especially when I'm not the architect. That's also how some of the other cores work, so I thought it's the standard way. But if you think there's no need to keep the old aspect ratio even as an option, then I'm not going oppose it.

I would get ride of 4:3 option since it's not correct anyway (with NTSC PAR, calculated DAR for 320x224 would be 1.3 so this is close enough to old core-provided aspect ratio) and 6:5 option since this only applies to game gear and would be covered by AUTO (just make sure it's not applied if config.overscan is enabled). However, being able to force using NTSC PAR might make some sense, because almost without exception all games look more correct in NTSC. There are even some PAL exclusives with no NTSC version available, such as Micro Machines for SMS, that look better if you use NTSC PAR. With Micro Machines can see that the cars etc look wrong even if you look it with just you eye, but in this case you can verify it by comparing it to the almost identical Game Gear version. The versions look same when you force the SMS version to NTSC PAR.

I agree, thus keeping AUTO, NTSC and PAL (for consistency, even if I don't think anyone would want to play in 60hz with PAL aspect ratio) seems the best tradeoff to me.

What comes to the Game Gear. I didn't mention it in this ticket because with it things are more compicated. But as you noticed I've been testing it as well and the 6:5 PAR and horizontal cropping is indeed for replicating the looks of Game Gear in LCD mode. I'm aware of the forum post about Game Gear's downscaling algorithm backward engineering you linked. At some point I actually considered implementing it as well, but after looking a few hours into it, it seemed a bit too complicated a task. Luckily the 6:5 PAR applies to both GG and GGMS when in LCD mode so, for now, that's good enough for me - even if it doesn't match 1:1 to the original (which should be the target of course).

I could eventually look into it and make it into an optional LCD filter for GG-MS mode. Having also an option to force LCD mode (and therefore cropping in GG-MS mode) off inside core emulation is probably also necessary now. Thanks for your comments. I'll return to you by a new comment in this ticket once I've done the suggested changes and fixed the issues.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

clobber commented 8 years ago

@arakerlu Just FYI, there is a wiki created by Tepples from nesdev that will save you the trouble of calculating dot clock rates to get the correct PAR for various systems:

https://pineight.com/mw/index.php?title=Dot_clock_rates

arakerlu commented 8 years ago

I'm aware of that wiki. It's a good list, but unfortunately addresses only the NTSC versions and skips the PAL versions entirely, so it only saves half the trouble. And actually, if you calculate Genesis' and Master System's PARs using their exact master clock frequencies, the values in that wiki are off for those systems by a tiny bit. Not by any meaningful amount though.

arakerlu commented 8 years ago

I created a new branch (https://github.com/arakerlu/Genesis-Plus-GX/tree/testing) that contains only the desired changes for the core-provided aspect ratio core option. The options are now: auto, NTSC PAR, PAL PAR. Default is auto.

Also, for non-interlaced video PAR is divided by 2.

For now, the cropping of 8+8 pixels from left and right in GGMS mode is not included, because I'm not sure if I were supposed to create another core-option for it? I'm referring to this sentence of yours:

"Having also an option to force LCD mode (and therefore cropping in GG-MS mode) off inside core emulation is probably also necessary now."

So should it be core-option "force LCD mode" with options enabled|disabled?

ekeeke commented 8 years ago

Just a few things:

1) NTSC and PAL PAR should also be using system_clk since it's a separated setting and PAR is more impacted by 50hz/60hz difference than minimal differences between 53.69mhz/53,20mhz master clocks . I will add later a core setting that let you change system_clock to AUTO (automatically based on vdp_pal), NTSC_MCLK or PAL_MCLK so you could have 50/60hz and region switched PAL/NTSC consoles like it is possible in standalone Wii port.

2) It is not necessary to call calculate_aspect_ratio() function within check_variables() function since the latter will set bitmap.viewport.changed if necessary, which will then call update_viewport() function in retro_run before emulating next frame.

3) The PAR by itself is the same in non interlaced or interlaced mode, it's just lines are more spaced in the former and occupy the same screen space as lines in interlaced mode although there are twice more so it's DAR calculation that needs to be adjusted, not PAR. It would be cleaner to have the DAR always calculated using (vheight * 2) as divider and call calculate_aspect_ratio() function BEFORE vheight is multiplied by 2 in update_viewport() when config.interlaced and config.render are set.

The rest is fine to me.

ekeeke commented 8 years ago

As for "Game Gear LCD mode" option, i am more thinking about "auto" (config.gglcd=1) and "off" (config.gglcd=0) to stay consistant with other dependent options. If config.gglcd=1 and config.overscan=0 then it should: 1) use a PAR of 6:5 in GG and GGMS modes if aspect ratio core option is set to auto 2) crop display for GGMS mode inside core like you did (no matter of aspect ratio core option)

arakerlu commented 8 years ago

2) It is not necessary to call calculate_aspect_ratio() function within check_variables() function since the latter will set bitmap.viewport.changed if necessary, which will then call update_viewport() function in retro_run before emulating next frame.

The reason why calculate_aspect_ratio() is called also in check_variables() is that, in case the aspect ratio changes, it's the only way to get the window dimensions to update according to the new aspect ratio. That's because it is the call to environ_cb(RETRO_ENVIRONMENT_SET_SYSTEM_AV_INFO, &info) that updates the window dimensions and it is called only there. However, I found out that it doesn't work correctly in some cases (such as when interlaced mode is active, or ntsc filter is in use), so I removed the line.

Anyways, if you change between NTSC and PAL regions in windowed mode, it currently looks weird, as the windows dimensions change, but not to a correct dimension. Especially if you do it repeatedly. Not a huge issue though.

3) The PAR by itself is the same in non interlaced or interlaced mode, it's just lines are more spaced in the former and occupy the same screen space as lines in interlaced mode although there are twice more so it's DAR calculation that needs to be adjusted, not PAR. It would be cleaner to have the DAR always calculated using (vheight * 2) as divider and call calculate_aspect_ratio() function BEFORE vheight is multiplied by 2 in update_viewport() when config.interlaced and config.render are set.

This is where I don't manage to follow you. As I understand, there are twice the lines in interlaced mode vs. non-interlaced mode and, afaik, the horizontal resoltion doesn't change between the modes. As the picture is, in both modes, fitted in the same 4:3 screen, I don't understand how the pixel aspect ratio would not be different. Either we aren't talking about the same "pixel aspect ratio" or one (or both) of us is misguided on something here. I'm still newbie on this matter and especially with this interlaced mode, so it may be that there's some simple thing I've got wrong.

Anyways, if I follow your advice and calculate the DAR using always (vheight * 2), then the Game Gear DAR calculation is wrong by factor of two. Unless, of course, I use 12:5 as pixel aspect aspect ratio, but that doesn't seem correct. The pixel aspect ratio for Game Gear should be 6:5 if you calculated it using the facts that there are 160x144 pixels in the 4:3 screen. The formula for calculating display aspect ratio is DAR = PAR * SAR (https://en.wikipedia.org/wiki/Pixel_aspect_ratio). SAR, or storage aspect ratio, which is somewhat a fuzzy concept to me, but basically, as I've understood, represents the ratio of width and height of the pixels in the raw bitmap (or similar) representation of the picture. Thus, in this case, the SAR is 160/144. From the forementioned facts, DAR = (4/3) and SAR = (160/144), follows that PAR = (4/3) / (160/144) = 6/5.

ekeeke commented 8 years ago

The reason why calculate_aspect_ratio() is called also in check_variables() is that, in case the aspect ratio changes, it's the only way to get the window dimensions to update according to the new aspect ratio. That's because it is the call to environ_cb(RETRO_ENVIRONMENT_SET_SYSTEM_AV_INFO, &info) that updates the window dimensions and it is called only there.

You are right, did not thought about that. It would then indeed be better to call it there as well then. It should be called whenever aspect ratio is changed then, not just when region is changed like you initially did.

However, I found out that it doesn't work correctly in some cases (such as when interlaced mode is active, or ntsc filter is in use), so I removed the line.

It might be because these change values of vwidth and vheight but changes have not been applied yet (they are applied in update_viewport function which is called during retro_run if bitmap.viewport changed, it is done there because rendered screen resolution can be changed dynamically by MD games).

On that regard, I think NTSC filter upscales the original screen width so you might need to call DAR calculation in update_viewport() BEFORE vwidth is adjusted when that filter is enabled.

A solution could be to have a static boolean (update_av) being set in check_variables() whenever aspect ratio is changed then call environ_cb(RETRO_ENVIRONMENT_SET_SYSTEM_AV_INFO, &info) in retro_run() when update_av is TRUE. This should be done after update_viewport has been called in retro_run. In retrospect, update_av should also be set in check_variables() when region is changed (instead of calling environ_cb here) and probably also whenever reinit or update_viewport booleans are set, just to be safe.

This is where I don't manage to follow you. As I understand, there are twice the lines in interlaced mode vs. non-interlaced mode and, afaik, the horizontal resoltion doesn't change between the modes. As the picture is, in both modes, fitted in the same 4:3 screen, I don't understand how the pixel aspect ratio would not be different.

That's because, in non-interlaced mode, there are still the same amount of "lines" on DISPLAY, it's just that half of them are blanked (what people generally miscall "scanlines") and only even lines hold active pixels.To maintain correct DISPLAY aspect ratio, the number of RENDERED lines must therefore be doubled when calculating DISPLAY height used for DAR calculation: DAR = PAR x display width (in number of displayed pixels) /display height (in number of displayed lines). But the dot clock and therefore pixel aspect ratio remain the same in both modes, pixels themselves are not stretched vertically in non interlaced mode and have the same "width" in both modes.

Either we aren't talking about the same "pixel aspect ratio" or one (or both) of us is misguided on something here. I'm still newbie on this matter and especially with this interlaced mode, so it may be that there's some simple thing I've got wrong.

There is only one correct definition of "pixel aspect ratio" that I know of for analog video. It depends on only two factors: the dot clock (which defines the number of individual pixels within a defined period of the line and, consequently, their "width" when displayed on screen) and the TV refresh rate (which defines the spacing between lines on screen and, consequently, the "height" of pixels when displayed on screen). It does not depend on the number of rendered or displayed lines.

Anyways, if I follow your advice and calculate the DAR using always (vheight * 2), then the Game Gear DAR calculation is wrong by factor of two. Unless, of course, I use 12:5 as pixel aspect aspect ratio, but that doesn't seem correct. The pixel aspect ratio for Game Gear should be 6:5 if you calculated it using the facts that there are 160x144 pixels in the 4:3 screen.

Indeed, that's because LCD screen is different from analog TV screen and those 144 lines fill the entire LCD screen. Analog video does not really know what pixels are (those are just voltage variations at regular time intervals) but LCD are digital. Your PAR calculation is correct assuming LCD display aspect ratio is indeed 4:3 (game gear service manual says screen dimensions are 65.27 x 48,90 mm which is indeed very close to it). I guess it does not really matter where you apply the 1/2 factor for NTSC and PAL, but maybe there is no need for distinct functions for PAR and DAR calculation and this could be done in a single function, with distinct calculations for each cases.

arakerlu commented 8 years ago

Great, it's all clear to me now. We actually had a slightly different ways to think of the pixel aspect ratio after all. You regarded it as the aspect ratio of pixels in the image shown in the CRT TV, whereas I regarded it as the aspect ratio of the pixels in the image produced by the emulator, which doesn't necessarily contain the blank scanlines. That's why the division by 2 was done for the PAR in my calculations, which you pointed out in one of the previous comments. Both, as already noted, lead to same results, and the only difference is where exactly to apply that divider or multiplier of 2.

I've now pushed all the changes related to this into my "testing" branch and all the issues and other points that have been raised should have been solved. So perhaps I could create a pull request now?

ekeeke commented 8 years ago

I reviewed your changes by simulating a pull request and everything is fine to me except one thing: SET_SYSTEM_AV is not called when timing.fps is changed (which happens when changing region in core settings) so it should be called dependless of the return value of update_viewport().

arakerlu commented 8 years ago

Good point, thanks. Done.

ekeeke commented 8 years ago

Still not good ;-)

retro_get_system_av_info() should be called after update_viewport() so that returned info values are actually updated

Something like that should work better

   if (bitmap.viewport.changed & 1)
   {
      struct retro_system_av_info info;
      bool geometry_changed = update_viewport();

      if (bitmap.viewport.changed & 8)
      {
        retro_get_system_av_info(&info);
        environ_cb(RETRO_ENVIRONMENT_SET_SYSTEM_AV_INFO, &info);
        bitmap.viewport.changed &= ~8;
      }
      else if (geometry_changed)
      {
         retro_get_system_av_info(&info);
         environ_cb(RETRO_ENVIRONMENT_SET_GEOMETRY, &info.geometry);
      }

      bitmap.viewport.changed &= ~1;
   }
arakerlu commented 8 years ago

Damn. Hmm, maybe I shouldn't do coding, or at least push any changes, in the evening after being a whole day alone with two small children. :-P Will fix soon.

ekeeke commented 8 years ago

No problem, your contribution is still very helpful so don't blame your kids too much ;-)

That's said, looking at your latest commit, please respect ANSI C standard: variable should be declared at the start of blocks, before any other code statement. There are some libretro platforms where the compiler requires this specific rule. Sorry :-)

arakerlu commented 8 years ago

I sure don't blame them, it was more a joke on how it was such a simple thing I just missed. ;)

please respect ANSI C standard: variable should be declared at the start of blocks, before any other code statement. There are some libretro platforms where the compiler requires this specific rule. Sorry :-)

I wasn't aware of the relevance of that ANSI C standard. Thanks for pointing that out and no need to be sorry. ;) Pure C is somewhat in the unknown territory for me. Seems that it's much used in libretro/retroarch projects, so there's a good excuse to learn it. :)

You've been super helpful and kind with your thorough explanations, good tips and patience. Thank you for that. :)

arakerlu commented 8 years ago

My code should now, after the latest commit, be 100% ANSI compatible.

By the way, if (or as) the ANSI conformance is a requirement, it might be a good idea to set the compiler flags -ansi -pedantic in makefile.libretro. That's how I checked my code.

ekeeke commented 8 years ago

Seems fine to me, you can submit the pull request and I will merge your changes.

As for libretro makefile, I agree but I generally prefer to leave this file under the responsibility of libretro team.

ekeeke commented 8 years ago

This is now merged into main repository.