stella-emu / stella

A multi-platform Atari 2600 Emulator
https://stella-emu.github.io
GNU General Public License v2.0
625 stars 111 forks source link

Better phosphor effect #75

Closed thrust26 closed 7 years ago

thrust26 commented 7 years ago

We have been discussing for quite a while now. I think after we finished implementing the new core, we should come back to this.

sa666666 commented 7 years ago

Yes, I have your patch here. The only problem is that there are issues with PAL colour loss, which hasn't be implemented yet (https://github.com/stella-emu/stella/issues/13).

thrust26 commented 7 years ago

I have seen quite some game development lately which seems to rely on Stella's very forgiving phosphor emulation too much. So maybe we should add more focus to this.

ale-79 commented 7 years ago

Related to this, currently the phosphor effect is applied also to the TIA display in the debugger (but not on the "zoom" area). This is not a bug, as it's explicitly mentioned in the documentation Anyway, IMO in the debugger you should only see the raw TIA output, without any filter applied. What do you think?

thrust26 commented 7 years ago

I would prefer raw output here too. But its no big deal, because we have the zoom window.

And, if you want to debug without any effects, you can easily disable those. So the current debugger view is maybe the better solution.

sa666666 commented 7 years ago

This is now implemented in https://github.com/stella-emu/stella/commit/f8ea61875c5ea379b798baf3322be1d9b2c2b13f. Currently it's not working in Blargg mode; this is the next TODO item.

thrust26 commented 7 years ago

Probably we should use much lower default blending values (and convert existing ones). Something around 25 (~1/3rd of 77) seems more realistic.

sa666666 commented 7 years ago

The current implementation looks good at 77. And unless I'm misunderstanding the code, anything below 50 is as if phosphor mode isn't enabled at all (which is why I've capped the lower limit to 50). I think we need another pre-release to experiment with this. I will look into it once I get phosphor mode and Blargg working again.

thrust26 commented 7 years ago

@sa666666: Not sure why you assume anything below 50 disables phosphor. If e.g. the current value is 0 and the previous value was 100, getPhosphor will return 100 * (blend value/100). So a blend value of 25 would return 25 here.

Unfortunately I cannot really test lower values, because my LCD is very slow. So I tried with the debugger, but (unlike the 4.7.x versions, where it is visible in the TIA Display, but not in the TIA Zoom), there the phosphor effect is not displayed at all. So I base my findings on screenshots.

The current default of 77 results into very noticeable traces, especially for fast moving, non-flickering objects: defender 1982 atari

High blend values close to 100 result into some very cool effects. :) asteroids 1981 atari _5

BTW: Changing the values and enabling/disabling the phosphor mode is not immediate. You either have to reload the ROM or enter and leave the debugger. Immediate would be nice, else the menu needs a (*).

sa666666 commented 7 years ago

My mistake on the assumption of below 50 having no effect; it looks like I was using some test code when I made that claim. So I've changed the code to have 0-100 as the valid range. I will get the binaries updated later this evening to reflect that.

The phosphor effect is no longer displayed in the debugger, since with this new code the effect is spread out over time, and the debugger shows only one frame at a time. Previously, the phosphor frame was blended before being shown, so the entire frame was visible. Unfortunately this will also have consequences for taking snapshots in that mode. In any event, a request further up in this issue indicated that you guys would rather that the debugger showed raw output anyway.

As for changing the phosphor mode within the Game Info dialog, it was never immediate in that case. There is already a '*' at the bottom of the dialog indicating as such. Now, if you use the shortcut key (Alt-p), then phosphor mode is toggle immediately. What isn't accessible immediately is the blend amount. It would be easy enough to add a key for the next release.

thrust26 commented 7 years ago

In any event, a request further up in this issue indicated that you guys would rather that the debugger showed raw output anyway.

Yup, and that's fine.

BTW: I noticed that the default in the code is 77 while in the menu it is always 60. How that?

As for changing the phosphor mode within the Game Info dialog, it was never immediate in that case. There is already a '*' at the bottom of the dialog indicating as such.

Ah, my bad. I always thought this would be a footnote, referring to some entries marked with (*).

sa666666 commented 7 years ago

Another error on my part. For the next pre-release (yes, there will be one :smile:), the level is changed to 30 everywhere, and the range is set to 0 - 100. And there will be shortcut keys to change phosphor blend when actually playing a game (it will be immediate).

thrust26 commented 7 years ago

Doesn't Stella have defaults for certain games? If yes, those should be adjusted (by 30/77, based on the new default).

sa666666 commented 7 years ago

There are only 3 on the properties database that use something other than the default (66, I believe). I will look at these. Also, I may have to adjust some new ones, particularly Compumate, since it's pretty unreadable at 30.

thrust26 commented 7 years ago

The old default was 77.

Do you have VSync enabled? For me that makes quite a difference.

sa666666 commented 7 years ago

I always have vsync enabled. To me, it is/should be a requirement.

I've just committed some fixes in https://github.com/stella-emu/stella/commit/5dbd9fee5103aa31b1a1ff2db958bc567e3a3765. The range of blending is now 0 - 100, the default is 30, and the Alt-i and Alt-o keys decrease/increase blending dynamically. I will do another pre-release when I get home this afternoon.

SpiceWare commented 7 years ago

Looks like the decay is still occurring when you enter command mode by hitting \ , which is how I usually take snapshots (due to the Mac's repurposing of the function keys).

Screenshot using F12 - can see decay within the station in the title: draconian

Screenshot using \ then clicking the snapshot button - no decay within the title draconian_1

If you set decay to 99 you can clearly see the decay still happening if you hit \ and leave the menu up.

Decay at 99 works well for screenshots of static interlaced displays, you can subtly see the flicker: draconian_3

Decay at 100 not so much 😆 draconian_2

thrust26 commented 7 years ago

I wonder if we should rename that value. Is Blend still the correct term here?

sa666666 commented 7 years ago

I leave that to you guys to decide. To me, it's just a string that can be easily replaced with something else.

thrust26 commented 7 years ago

I don't care, just wondering.

thrust26 commented 7 years ago

Something in the code seems wrong.

So somehow the values are blended twice. Where is the error? test.zip

BTW: Is it intentional that the values created by ALT+I/O are not stored in the properties?

sa666666 commented 7 years ago

How are you determining this math; what are the equations used? It's possible that something is wrong in the code, but this is the code you sent me. Or maybe I modified something incorrectly??

As for you second point, yes, changing ROM properties dynamically will not store the ROM properties. This is also true for toggling phosphor mode, format, color/bw, etc. If you want to save the properties, simply go to 'Game Properties' and click 'OK'. At that point they are saved. The reasoning was that changing things dynamically was meant to show how things would look when changed, and then when you want to keep the changes, you have to manually do so.

I will look at the phosphor code again tomorrow morning to see what's going on.

thrust26 commented 7 years ago

I simply made some screenshots from the test ROM and checked the RGB values (all the same for grays).

Not sure if I get your code right, but why do you call getRGBPhosphor in TIASurface::render and TIASurface::pixel? So maybe only the screenshots have the double blending error and the emulation is correct?

EDIT: Screenshots made with the OS are correct. So the problem is most likely only in the screenshot code.

sa666666 commented 7 years ago

TIASurface::pixel() is a convenience function only used in the debugger; TIASurface::render() is the function used by during emulation mode to render the frame. At no point are these two called together.

I'm looking at the screenshot code now, and don't see any issues. I will explore this further, and compare its output with ones generated by the OS itself.

thrust26 commented 7 years ago

Here are two screenshots from Asteroids (Blend = 50). The asteroids are flickering at 30Hz. Notice how much darker they are in Stella. Stella: asteroids 1981 atari _7 OS: asteroids_os

sa666666 commented 7 years ago

I am still having major issues with this. On my system, a default blend level of 30 is basically the same as not having phosphor effect turned on at all; it flickers just as much. For me, I need to increase it to 80 or so before the flicker starts to disappear, but is still there enough to simulate the real thing. That's why I'm also implementing #144.

Also, I can't see how to fix the issue of a snapshot taking a darkened image. The relevant code takes a snapshot of the current frame, and whatever is on the screen at that time is going to be captured. And half of the time, that image will be slightly darkened. This is really a symptom of a larger problem; how do you take a discrete snapshot of a continuous event (multiple frames)??

thrust26 commented 7 years ago

I just press F12 multiple times. With 30Hz flicker there is a 50% chance of hitting the right frame.

So how does Asteroids with blend 50 look on your system? (Stella and screenshot)

sa666666 commented 7 years ago

Here are several PNGs generated by pressing F12 fairly quickly in succession. Note that it's doing exactly what you'd expect. In some frames the ship is visible and the asteroids are dark; in others it's vice-versa. Note that without phosphor mode enabled, the dark images would disappear altogether. But it is still only taking a snapshot of a single screen at a time. The fact that one doesn't see that in emulation is an optical illusion. I guess the same would happen on a real TV.

asteroids.zip

thrust26 commented 7 years ago

Can you post your screenshots? Stella and OS when the asteroids are dark.

sa666666 commented 7 years ago

The screenshots are in the attached ZIP file :) Taking them from Stella or the OS (KDE, LInux) doesn't matter. Here are a few: asteroids 1981 atari no copyright asteroids 1981 atari no copyright _1

thrust26 commented 7 years ago

I compared the RGBs of the yellow asteroids:

Yours (Stella, correct?): Enabled: 187, 187, 53 Dark 1: 46, 46, 13 Dark 2: 23, 23, 6 (from the trace of the enabled asteroids) Dark 3: 11, 11, 3 (from the trace of the Dark 1 asteroids)

Mine (Stella): Enabled: 187, 187, 53 Dark 1: 46, 46, 13 Dark 2: 23, 23, 6 Dark 3: 11, 11, 3

Mine (OS): Enabled: 187, 187, 53 Dark 1: 93, 93, 26 Dark 2: 46, 46, 13 Dark 2: 23, 23, 6

Only my OS screenshots (can you post some too?) are like expected. And IMO they should speak the truth, because one error path (screenshots) is eliminated. If you look at the values, for me it seems very clear, that when taking a screenshot the disabled objects are blended once more.

I think this happens in TIASurface::pixel. This is called by TIASurface::baseSurface which is only called by EventHandler::takeSnapshot.

sa666666 commented 7 years ago

When taking snapshots and not in 'ss1x' mode (ie, snapshots are always 2x or bigger, all effects enabled, etc), then TIASurface::pixel is not being called. I verified this by adding a print statement to the methods you mention above; they are not being called. So they cannot be causing this issue.

Here are some snapshots from the OS itself:

screenshot_20170611_171532-3 screenshot_20170611_171532-4

DirtyHairy commented 7 years ago

Hmmm, I just checked this out, and I can add two observations:

I have a feeling that the differences between the amount of flicker reduction that we see is due to different postprocessing in our video stack --- OS, video driver and display. If this boosts low intensities, then lower blend levels should be sufficient.

sa666666 commented 7 years ago

I have to use 60-70 as well, but I believe we're both using Linux, so maybe that is the commonality. I don't have to toggle phosphor mode to see a difference, though; using Alt-i and Alt-o to change the blend level immediately starts to show a difference.

This is also why I think we need #144 implemented; there will never be a hardcoded default value that works for everyone.

DirtyHairy commented 7 years ago

I don't have to toggle phosphor mode to see a difference, though; using Alt-i and Alt-o to change the blend level immediately starts to show a difference.

And there we have the difference: I was using the menu :stuck_out_tongue: I can confirm that the effect is immediate if I use the keyboard.

This is also why I think we need #144 implemented; there will never be a hardcoded default value that works for everyone.

:+1: I agree.

DirtyHairy commented 7 years ago

It just occurred to me: @sa666666, what video driver are you using on linux? For me, it's an intel IGP with the usual opensource drivers.

sa666666 commented 7 years ago

Update: the post above with the OS snapshots have been updated; I was using 30% blend, when the others posted before it were 50%. These new ones are 50% too.

sa666666 commented 7 years ago

AMDgpu drivers, open source (with an RX 480).

thrust26 commented 7 years ago

For me 30 works nicely. Probably you are right and this is OS related (I have Intel IGP graphics too). Still weird.

Or maybe its my very slow LCD?

And Stephens OS screenshot provide the same values as my OS screenshots. Now, why are the disabled objects in Stella's screenshots darker by one blend factor?

sa666666 commented 7 years ago

Confirmed that Stella-generated screenshots are applying one more blend level. This must be in Stella, since we now have OS screenshots from Windows and Linux which are the same.

thrust26 commented 7 years ago

And after thinking about it, it MUST be the different LCDs we are using (and not the OS). The OS screenshots provide the exact same values for us.

Probably the grey to grey fall time of my display is pretty long. So we better adjust the default to your needs (assuming your LCDs are more modern, mine is from 2005 or so with 20ms response time).

sa666666 commented 7 years ago

I've found the problem with Stella applying blending one too many times. A past issue was that snapshots would sometimes have UI messages erroneously overlaid on the TIA image, so I added code that essentially turned off the messages and rendered the frame once, to erase everything. Turns out this is interfering with correct snapshot output. When I disable lines 1859-1860 in EventHandler.cxx, Stella generates snapshots as follows: asteroids 1981 atari no copyright _3 asteroids 1981 atari no copyright _4

So we now see the correct output, but the messages aren't being removed. I guess I will have to figure out how to deal with that another way.

However, we still have the issue of blend level 30 being fine on @thrust26 system, and way too low on our systems. This can't be taken care of with a set value, so we need to implement #144. Problem is, what should the default value be, keeping in mind that many people never change the defaults??

thrust26 commented 7 years ago

See above, use your values. My LCD is most likely not representative at all.

EDIT: I just tested with a notebook (~5 years old) and a 2 year old convertible. Both look nice with 30 too. Probably we should add some info to the documentation about the influence the display has.

DirtyHairy commented 7 years ago

And after thinking about it, it MUST be the different LCDs we are using (and not the OS). The OS screenshots provide the exact same values for us.

This is likely, allthough I am not completely convinced :smirk: I think it is possible that there is some postprocessing (e.g. gamma correction) happening above the layer in which the screenshot is captured. Otherwise, the screenshot would look different from the screen on the same machine due to double postprocessing.

Anyway, I think that augmenting the documentation is the only the viable option. This is the same thing as gamma correction in games: everyone has to configure it for themselves.

thrust26 commented 7 years ago

I have never heard about any software post processing above the screenshot level. At least not in Windows. Do you have any links?

What monitor are you using? And how is it set up (e.g. frequency and acceleration level)? Maybe we can identify, why your and @sa666666's monitor(s) need more blending than mine. So far my best guess is the response time.

DirtyHairy commented 7 years ago

I have never heard about any software post processing above the screenshot level. At least not in Windows. Do you have any links?

None, and I am not sure whether such a thing exists, I am just trying to guess what might be going on :smirk: What I could imagine is that some settings like color temperature (I remember the NVIDIA driver has such options) or ICC profiles don't take effect in screenshots. This would at least make sense to me as such settings adjust how the screen content is presented on the display, while a screenshot captures the screen contents.

Regarding display: phosphor behaves pretty much the same on my laptop display and an external Dell LCD (displayport, both 60Hz refresh, I think). I just checked what happens if I turn up gamma using xrandr, flicker gets a bit better, but 30 still is too low. Response time is a good guess, I think, but I'll have to look up the specs of my displays.

thrust26 commented 7 years ago

Don't forget that phosphor should only reduce flicker (to match TV flicker), not completely eliminate it like the old one.

DirtyHairy commented 7 years ago

:smirk: I know. To have a baseline, cranking blend up to 80 gives a flicker that is comparable to 6502.ts in WebGL mode (I am using a shader for flicker reduction there, so it doesn't work with WebGL off), although the asteroids have slightly longer trails in Stella.

sa666666 commented 7 years ago

In Windows, about 50 is just right (a slight amount of flicker, but not too much). In Linux, I also need to go to about 80 for the same effect. I haven't tested in OSX yet. Note that the Linux and Windows tests are on different systems, but the OSX one will be on the same system as Windows.

Right now I'm leaning towards making 50 the default, and let users adjust from there.

DirtyHairy commented 7 years ago

I think that this is a good way to proceed, in particular as Windows likely applies to the overwhelming majority of users.

sa666666 commented 7 years ago

OK, 50 it is then. I hope to get this finished by the end of the week, now that work is slowing down a little (midterm break). After that is #144, and then I'm done I think.

@DirtyHairy, could you look at the latest issues and determine whether they can be done for 5.0 or should be pushed to 5.1 or so?