libretro-mirrors / beetle-saturn-libretro

Standalone port of Mednafen Saturn to the libretro API.
GNU General Public License v2.0
61 stars 40 forks source link

Use actual resolution for output image size & simplify core options #126

Closed hiddenasbestos closed 5 years ago

hiddenasbestos commented 5 years ago

Experimental patch - testing required!

This patch reports the actual resolution of the Saturn frame buffer as the output geometry size. 320 and 352 pixel wide modes are supported as are the 224, 240 and 256 line modes (plus x2 with interlacing)

The front-end is always told that it needs a 4:3 aspect ratio, so should scale the pixels as required.

I've also removed a number of settings that are no longer required such as the start/end line, hblend (which is duplicating work the shaders can do) and hmask which trims the front/back porch and is no longer needed.

hiddenasbestos commented 5 years ago

The two issues that I can imagine causing problems are:

  1. 15KHz CRT mode in RetroArch not liking 224 mode (since I assume it's wanting 240p or 480i)
  2. Should some of these modes actually have borders on a real Saturn?

For 2 perhaps an option could be created to toggle the cropped appearance added with this patch, vs. what it would look like on a real 240p / 480i display.

hiddenasbestos commented 5 years ago

Note this patch was created for issue #125

retrorepair commented 5 years ago

224 mode will be fine as porches are created by the CRT switching code, all the core needs to report is the correct active resolution. These porches (blanking period) make up any borders the saturn would have had.

On an LCD where CRT switching isn't used it will possibly need borders but then doesn't retroarches aspect ratio options fix that?

To be honest, at some point it would be excellect if cores could also report accurate hblank timing if it's obtainable since porches at the moment are apporximated. It's quite close in terms of how it compares to the real consoles but I'd like it to be a little more accurate.

The correct timing info is known for most consoles, retroarch just needs a structure where this i formation can be passed from the core.

retrorepair commented 5 years ago

Will test as soon as I get a chance by the way!

hiddenasbestos commented 5 years ago

I hope it's better but I'm not sure cropping the 224 resolutions down was the right decision. The aspect ratio is no longer correct doing this, I think. Since you said the top/bottom borders would be added automatically to get it back up to 240p, and that LCD screens will need it - seems to make sense that the old locked fixed 240 (/ 288) behaviour by default to preserve the aspect ratio of 224 and 256 line modes.

Really wish I had real hardware and a PVM :)

hiddenasbestos commented 5 years ago

Pushed a modification to use a fixed height of 240/288 (or x2 for interlace) and dynamic width. Seems to look ok compared to random real footage I've found on YouTube. Still not 100% sure this patch is correct, but if it makes your CRT experience better I'm happy to help.

Looking forward to feedback :)

inactive123 commented 5 years ago

Does this work fine in RetroArch?

Are there any objections still to merging this from anybody? Does it come with performance regressions or is everything fine there?

retrorepair commented 5 years ago

Will be testing tonight and will report back

ghost commented 5 years ago

the current behavior suppose to remedy the the condition when certain game uses interlaced and non-interlaced modes (for example Sega Touring Car Championship which uses interlaced in most parts and non-interlaced in main race). When using integer scale, your screen will keep changing size depending on height of monitor(or screen height), like if one uses 900px max monitor height(yeah i have such a monitor) , during the interlaced portion the screen will be smaller than when its on non-interlaced.

@Tatsuya79 should have idea of this behavior as well.

On Mon, Oct 1, 2018 at 10:20 PM retrorepair notifications@github.com wrote:

Will be testing tonight and will report back

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/libretro/beetle-saturn-libretro/pull/126#issuecomment-425925667, or mute the thread https://github.com/notifications/unsubscribe-auth/AWPDtii7YND1UAKiis8k7HOXbCD_6HyCks5ugiSygaJpZM4XAk91 .

hiddenasbestos commented 5 years ago

Hi @twinaphex I'd like to add an additional core option first to control cropping the horizontal and vertical borders. Right now it's a compiled in bool so that @retrorepair can test how 224 line mode works on a CRT.

I am curious what a real console would do when it changes between 320 and 352 wide modes -- does it have larger borders either side (current behaviour), or does it stretch out by 10% so that it's only a change in pixel density along the scanline? I don't have a real console to test this.

Options to crop borders will still be useful even if it's not accurate to the hardware, I'm just trying to decide on the default option really. Plus, there is an additional nit-picking question on whether the aspect ratio should be reported differently with cropped borders to scale the image to the correct pixel aspect ratio of the hardware. Right now it's compiled in to report 4/3 at all times.

I've not tested quite as thoroughly in RetroArch - I was able to debug a little easier in my own front-end so I did that, but it's probably fine given the nature of what's changing. We'll find out when we hear back from @retrorepair about whether CRT behaviour is improved. Happy to supply additional support for this.

In short, please don't merge just quite yet. We're getting there, I think :)


@retro-wertz If you are using 1:1 integer scale, then when the console changes resolution (or interlacing is enabled) it will, of course, change the size of the window to accommodate the additional pixels. That's what you're asking RetroArch to do - unless you specify a fixed window size which will override it.

Seems like this is a front-end setup issue for how the frame is presented, nothing to do with the core which is responsible for reporting a frame buffer size (whatever it may be) and a target aspect ratio.

hiddenasbestos commented 5 years ago

Okay a bit of evidence here: https://www.youtube.com/watch?v=lbPtJBLTibU

... at 0:30 the Saturn boot screen running at 320x224 fills the frame with white flash showing the edge of the active display area. ... at 0:43 the character select screen at 352x224 is slightly wider.

2nd video: https://www.youtube.com/watch?v=Dj-xLVSx_I0

Again the bios screen at 320 is narrower than the game running at 352.

So this tells me that the current core behaviour is correct and the default option should be no horizontal cropping.

However, cropping might be desirable so I will add it as an option, but disabled by default.

ghost commented 5 years ago

last time i touched ss core, the behavior was framebuffer changes depending if game is using high-width mode or interlaced modes, but output frame was fixed to be 352x240(288 on pal) as what mdfn originally do just to avoid this switching. with the current PR, it reverts to the original behaviour, it keeps changing window-size. dont know where or what should control this but it works and avoids this behavior (does you 14" tv change in size when running interlace mode? probably not, youll still be watching on a 14" tv no matter what the game is using, this is the simpliest example of what im trying to say here). it needs work, but thats the concept, unless we should be playing in constant switching screensize. im just saying.

horizontal and vertical cropping (1px reso) was suppose to be in this core, what happened to those?

On Mon, Oct 1, 2018 at 11:56 PM David Walters notifications@github.com wrote:

Hi @twinaphex https://github.com/twinaphex I'd like to add an additional core option first to control cropping the horizontal and vertical borders. Right now it's a compiled in bool so that @retrorepair https://github.com/retrorepair can test how 224 line mode works on a CRT.

I am curious what a real console would do when it changes between 320 and 352 wide modes -- does it have larger borders either side (current behaviour), or does it stretch out by 10% so that it's only a change in pixel density along the scanline? I don't have a real console to test this.

Options to crop horizontal borders and stretch the image will still be useful even if it's not accurate to the hardware, I'm just trying to decide on the default option really.

There is an additional question on whether the aspect ratio should be reported differently with cropped borders to scale the image to the correct pixel aspect ratio of the hardware. Right now it's compiled in to report 4/3 at all times.

I've not tested quite as thoroughly in RetroArch - I was able to debug a little easier in my own front-end so I did that, but it's probably fine given the nature of what's changing. We'll find out when we hear back from @retrorepair https://github.com/retrorepair about whether CRT behaviour is improved. Happy to supply additional support for this.

In short, please don't merge just quite yet. We're getting there, I think :)

@retro-wertz https://github.com/retro-wertz If you are using 1:1 integer scale, then when the console changes resolution (or interlacing is enabled) it will, of course, change the size of the window to accommodate the additional pixels. That's what you're asking RetroArch to do - unless you specify a fixed window size.

Seems like this is a front-end setup issue for how the frame is presented, nothing to do with the core which is responsible for reporting a frame buffer size (whatever it may be) and a target aspect ratio.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/libretro/beetle-saturn-libretro/pull/126#issuecomment-425960942, or mute the thread https://github.com/notifications/unsubscribe-auth/AWPDtj_7WwMlBx0g5fIDxYS6RQKYjIiSks5ugjsigaJpZM4XAk91 .

hiddenasbestos commented 5 years ago

A 14" TV isn't a good example because that's designed to handle a fixed 480i (or 576i for PAL) signal and hacks were used to get 240p / 288p image. A better example would be a multi-sync PC monitor which can accept a variety of resolutions and stretches the image to fill the screen.

What you're doing by using integer scaling is disabling the stretching to fit a fixed physical size, so of course the window will have to get taller to fit the extra vertical pixels generated by interlacing. Again this is a front-end issue and you should configure RetroArch to enable a fixed sized window if you want a fixed sized window.

If pixel perfect horizontal and vertical cropping is needed, that's a front-end feature IMHO and not something this specific core should be doing. It just adds bloat. If the front-end does it then all cores will benefit from this feature.

Cropping the 320 modes down from 352 is something the core can offer as an option because it's not something the front-end can know about easily.

hiddenasbestos commented 5 years ago

@retrorepair I've pushed a new change which adds two new core options to remove horizontal / vertical borders. They're disabled by default to match real hardware but if you want to fill the screen those options are there.

@twinaphex I've tested this in RetroArch more than I remembered and it does work.

I'm now going to see about reporting the correct aspect ratio when borders are cropped.

hiddenasbestos commented 5 years ago

Okay, final patch I think.

This update now reports the correct aspect ratio when borders are removed from either axis.

Note - this means that in RetroArch, where the default setting is to use the core provided aspect ratio - it will at first seem that cropped borders don't appear to do anything (because RA will counter-act the cropping with narrowing/shortening of the display). In order to change that, a forced 4:3 aspect ratio (or some other aspect you desire) needs to be set from the display settings menu.

I look forward to feedback - this pull request cleans up a few options, puts front-end tasks on the front-end and core tasks on the core, but by default probably doesn't appear to do anything.

@twinaphex : I'm now waiting for feedback from the issue reporter @retrorepair

inactive123 commented 5 years ago

OK.

hizzlekizzle commented 5 years ago

I'm concerned that this is catering to CRT/native-res at the expense of everyone else (read: the majority of users). As @retro-wertz mentioned, the previous behavior was more predictable. Regardless, I think those two cropping options could be combined into 'beetle_saturn_crop horizontal|vertical|both'

Is there a reason you're removing the horizontal blending option altogether? I'm not familiar with it, but I assumed it blends the awful dithering that's common in SotN?

hiddenasbestos commented 5 years ago

It's not catering to CRT users at the expense of others - anyone who doesn't want borders being added to the frame buffer can benefit from these options. If you don't want to crop the borders, don't use the options and the behaviour is the same as before. Except a bunch of confusing options that, IMO have no place being in the core have been removed - I'm not sure what issue @retro-wertz is having with it that can't be fixed by adjusting window settings in RetroArch.

Is there a reason you're removing the horizontal blending option altogether? I'm not familiar with it, but I assumed it blends the awful dithering that's common in SotN?

This feature duplicates work in software that can be performed by the front-end shader system in hardware. It's less efficient to use this option.

Tatsuya79 commented 5 years ago

It's been a while I haven't played around with the saturn core and I deleted some PAL games I had for testing. I compared current version with this PR and tested some games.

The "remove horizontal borders" options doesn't seem to do anything? It's always "remove vertical borders" that cuts the empty overscan, even when it's on top and bottom like in virtua fighter 2.

In Shinrei Jusatsushi Taroumaru (J) there's some black bars I can't remove now on top/bottom as the initial/last scanlines options are gone. Perhaps you have those lines on a real saturn but it was an easy fix integrated within the core that exists in stand-alone, and some users could already have done game overrides using it. Also, idk if that's the case here, but in mednafen pce_fast some games aren't centred vertically like Castlevania Rondo of Blood near the top (scanlines 3 to 226).

Apart from that I didn't see a problem, integer looked fine with raw scanlines. @retro-wertz tell me exactly what to look for if you want me to confirm something.

hizzlekizzle commented 5 years ago

The integer-scale issue that @retro-wertz mentioned is primarily an issue with odd scale factors (as in 3x, 5x, etc.), where this can happen (bios is non-interlaced, menu is interlaced, gameplay is non-interlaced): https://imgur.com/a/DAnC1tL That is, on a ~720p =< res < ~960p display with integer scaling, the size jumps around with interlaced/non-interlaced res-changes. Same thing happens on 4K (9x scale) displays, I think, though it's probably less noticeable there..?

Whether this is something we want to be be concerned with is a somewhat different question. /shrug

Re: horizontal blending, I typically agree with that line of reasoning, but we do have precedent for cores including similar software options, like higan's high-res blending option.

I do agree that the simplification of the options is nice.

hiddenasbestos commented 5 years ago

@Tatsuya79

The "remove horizontal borders" options doesn't seem to do anything?

The thing is when you enable cropping options the core updates the aspect ratio. RetroArch, by default, then uses that aspect ratio to adjust the shape of the image back and in the process adds its own borders of the same size. When removing horizontal borders you also need to force a display aspect ratio instead of the "use core provided" option.

Tatsuya79 commented 5 years ago

Would it be possible to keep the old rendering and scanlines core options, just adding an "auto" step in each of them that would switch to the new rendering?

hiddenasbestos commented 5 years ago

After all this, I'm actually doubting the value of this patch...

Certainly it has a lot to do with the fact that I don't think I'll be using any of the new features personally so I'm finding it hard to justify. Especially after researching it has now shown me that the current behaviour is correct wrt the original hardware. OTOH you might want to remove the 320->352 or 224 -> 240 borders so it at least does that.

I think I'd probably like to see fine per-line / column cropping added to RetroArch and removed from the core, but removing it from here first is putting the cart before the horse.

Still, the patch is here if it's useful but I'm not so sure merging it is the right thing to do at this time.

andres-asm commented 5 years ago

Yeah some video adjustment on the frontend (cropping, stretching, etc) like a video player would be nice imho.

On Tue, Oct 2, 2018 at 6:13 PM David Walters notifications@github.com wrote:

After all this, I'm actually doubting the value of this patch...

Certainly it has a lot to do with the fact that I don't think I'll be using any of the new features personally so I'm finding it hard to justify. Especially after researching it has now shown me that the current behaviour is correct wrt the original hardware. OTOH you might want to remove the 320->352 or 224 -> 240 borders so it at least does that.

I think I'd probably like to see fine per-line / column cropping added to RetroArch and removed from the core, but removing it from here first is putting the cart before the horse.

Still, the patch is here if it's useful but I'm not so sure merging it is the right thing to do at this time.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/libretro/beetle-saturn-libretro/pull/126#issuecomment-426460956, or mute the thread https://github.com/notifications/unsubscribe-auth/ABpC0Ki1PnfOZrcC7EeVqKTtHikoSnKXks5ug_MIgaJpZM4XAk91 .

hiddenasbestos commented 5 years ago

I'm thinking this might as well be closed, it's no great feat of engineering to redo it.

retrorepair commented 5 years ago

Well just to resurrect this, Yabause now appears to have correct resolution switching in retroarch so I'd think this is evidence enough? It looks pretty perfect in terms of screen image placement on a real CRT and switches at every expected point (as your patch did). I've not tested on an LCD.

I've since realised by the way that all the other mednafen cores have hard set resolutions as well, at least for the height. I can't imagine what the need for it is, it's confusing and all the other cores work great without this.

To be honest, I don't see the problem with having the patch you submitted as an option you can toggle if you are worried how it'd affect LCD displays. The CRT Switch code adds the extra porch information to properly display the image and will adjust this depending on resolution as it does with all the other non mednafen cores. It IS important for how the resulting image looks on the screen.

I can show images of Yabause vs Beetle Saturn on my CRT to show the difference it makes if required.

It's important for the CRT switch option for the cores to all follow the same approach to reporting resolution, at least by way of an option toggle. There's been a lot of support questions relating to this very issue so it'd be nice to clear it up once and for all. Just tell me what you need from me.