Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better phosphor effect #75

Closed
thrust26 opened this issue Jan 17, 2017 · 81 comments
Closed

Better phosphor effect #75

thrust26 opened this issue Jan 17, 2017 · 81 comments
Assignees

Comments

@thrust26
Copy link
Member

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
Copy link
Member

Yes, I have your patch here. The only problem is that there are issues with PAL colour loss, which hasn't be implemented yet (#13).

@thrust26
Copy link
Member Author

thrust26 commented Apr 5, 2017

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
Copy link

ale-79 commented Apr 9, 2017

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
Copy link
Member Author

thrust26 commented Apr 12, 2017

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
Copy link
Member

sa666666 commented May 28, 2017

This is now implemented in f8ea618. Currently it's not working in Blargg mode; this is the next TODO item.

@thrust26
Copy link
Member Author

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

@sa666666
Copy link
Member

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
Copy link
Member Author

thrust26 commented May 29, 2017

@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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

Another error on my part. For the next pre-release (yes, there will be one 😄), 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
Copy link
Member Author

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

@sa666666
Copy link
Member

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
Copy link
Member Author

thrust26 commented May 29, 2017

The old default was 77.

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

@sa666666
Copy link
Member

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

I've just committed some fixes in 5dbd9fe. 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
Copy link
Contributor

SpiceWare commented May 29, 2017

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
Copy link
Member Author

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

@sa666666
Copy link
Member

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

@thrust26
Copy link
Member Author

I don't care, just wondering.

@thrust26
Copy link
Member Author

thrust26 commented May 29, 2017

Something in the code seems wrong.

  • With a blend value of 50, when switching between black and white, the black frame should be displayed with 50% (118) of the white RGB values (236). But the RGB values of the attached test ROM are just 1/4 (59).
  • With blend = 66, the expected values is 2/3 or 236 (~156), but the result is 4/9 (102).

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
Copy link
Member

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
Copy link
Member Author

thrust26 commented May 30, 2017

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
Copy link
Member

sa666666 commented Jun 4, 2017

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
Copy link
Member Author

thrust26 commented Jun 4, 2017

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

@DirtyHairy
Copy link
Member

DirtyHairy commented Jul 5, 2017

I see a fundamental issue here: the phosphor code operates in RGB color space, and the result generally won't map to a discrete palette at all. We'd have to use a quantization algorithm to reduce it to 256 colors, and that would be dead slow.

I haven't looked at the Blargg code yet, but I think a viable approach would be:

  • Replace the palette lookups in Blargg with function calls
  • Create a Blargg palette from the TIA palette, store the indices in a lookup table (std::map should do) and try to map the 24 bit colors to precomputed palette values
  • Perform the necessary calculations on the fly if the lookup fails

Most parts of a typical game screen are static and should not differ from the TIA palette, so the overhead will only apply to the moving parts. In addition, this algorithm should be only marginally slower than Stella 4 if phosphor is turned off (provided the calculation is actually slower than the lookup if potential cache misses are taken into account). Comparing calculation and lookup performance would be as simple as disabling the lookup code,

If this fails, we could also apply phosphor after Blargg --- no changes would be required for this approach at all 😏

@thrust26
Copy link
Member Author

thrust26 commented Jul 5, 2017

The Blargg palette issue was a major problem for me too when I created the phosphor code (though I didn't fully understand it back then).

A game like Asteroids (up to 8 big asteroid IIRC) might be a test case for slow calculations, as it flickers the asteroids.

Phosphor mixes the previous displayed frame (which would be after Blargg, correct?) with the currently rendered frame. Then we should use phosphor after Blargg for the latter too. So the Blargg code wouldn't have to be changed, but phosphor would become a post processing step then. (if we would do phosphor before Blargg, we would mix a "Blargged" frame with a non-"Blargged" one, which makes no sense).

I am not sure if phosphor after Blargg will work very well, but it could be a good workaround for 5.0. Maybe a mixed solution would be most correct (some effects happen before phosphor and some after it). I am no expert here though and we can discuss this later.

BTW: The TIA palette has only 128 entries (for NTSC). And aren't that only 3.5 array accesses per pixel?

@sa666666
Copy link
Member

sa666666 commented Jul 5, 2017

Yes, that's what I was basically trying to say. Currently, there are only ever 256 RGB colours, so they can be added to an array-based lookup table (aka, a palette). The RGB data created from the phosphor code changes per frame, and while there will only be 256 of them at a time, the actual 256 will change per frame.

I like the idea of using a map to store RGB -> Blargg conversions. However, it's not just the RGB data that needs to be cached. There is also a 'kernel' that is created based on the RGB values. If you cache one, you need to cache the other too.

@DirtyHairy
Copy link
Member

In order for phosphor before Blargg to work, we'll have to retain the frame before applying Blargg and have phosphor mix with it --- mixing with the "Blargged" one makes no sense indeed.

@DirtyHairy
Copy link
Member

Currently, there are only ever 256 RGB colours, so they can be added to an array-based lookup table (aka, a palette).

Interesting. I guess I'll just have to look at the code tonight, but how does this work? If I understand correctly, the phosphor code calculates a weighted average between its two input frames which can be anywhere in RGB space. How does the quantization work?

There is also a 'kernel' that is created based on the RGB values

Ouch. If that depends on the whole palette, then caching lookups won't be of much help as each frame will esentially create a new cache entry.

@sa666666
Copy link
Member

sa666666 commented Jul 5, 2017

We always have access to the previous, pre-Blargg frame. Some confusion here, so clarification is in order:

TIA::myFramebuffer -> holds current TIA frame in a 256 entry array-based, RGB palette.

TIASurface::myRGBFramebuffer -> holds combination of current RGB frame mixed with TIA frame (no Blargg applied yet). This is what you call the 'currently rendered frame'. The previously displayed frame also comes from this, before Blargg is applied. Remember, Blargg and phosphor only interact when both are enabled.

What is output by Blargg -> whatever RGB data is passed to Blargg code is then processed and output into the TIASurface buffer itself. TIASurface contains a 'FBSurface', which has its own pointer to pixel data (it is essentially the final place for data to go, to be presented to the screen).

Also, while there are 128 colours in the TIA palette, we also have 128 for black and white. So 256 total. We need those other 128 colours to quickly process PAL colour loss. Also, they're used when greying out the TIA image in the debugger.

@thrust26
Copy link
Member Author

thrust26 commented Jul 5, 2017

I am for applying phosphor by mixing the previous after Blargg and phosphor (previous TIASurface?) frame with the current frame after Blargg (current TIASurface). So Blargg can still rely on its own, pre-computed palettes. And phosphor becomes an additional post-processing step after Blargg.

That should make it as fast as now and relatively easy to implement, doesn't it?

Thinking about it, this even makes sense, since Blargg emulates signal distortions (before the beam reaches the screen) and the CRT's phosphor effect happens on top of those.

@DirtyHairy
Copy link
Member

DirtyHairy commented Jul 5, 2017

Considering the complexity involved, I agree: let's apply phosphor after Blargg.

@sa666666
Copy link
Member

sa666666 commented Jul 5, 2017

@thrust26, no it won't be faster, for several reasons:

  • you're operating on a larger surface now; 560x210 now instead of 160x210, since every 2 pixels have been converted to 7 pixels, so the horizonal resolution is now converted from 160 to 560
  • the scanlines have been applied; ideally, the scanlines should be applied after everything, since they're an artifact of the TV screen itself, not NTSC effect

@DirtyHairy
Copy link
Member

@sa666666 I just checked the code; to me it seems that TIASurface::getRGBPhosphor actually calculates a full 24 bit RGB triple which is then passed to SDL for conversion to the platform format (which is requested as 8bit RGBA). If I am not mistaken, then there are no palettized values for Blargg to start with.

@sa666666
Copy link
Member

sa666666 commented Jul 5, 2017

There is another way:

  • compute TIASurface::myRGBFramebuffer each frame, as we do now
  • tell the Blargg code about it each frame (which implies re-calculating all the palette/kernel entries
  • pass TIA::myFramebuffer to Blargg code, and use its palette indices to render the frame

I think this will work, since if the TIA, for example, wants to use colour index 7, that 7 will correspond to raw colour 7 in the TIA palette, and RGB colour 7 in the RGB palette. Furthermore, it will correspond to Blargg colour 7 in the Blargg palette. IOW, they're all essentially parallel arrays with corresponding RGB data.

@DirtyHairy, maybe I'm not using the word palette correctly. The entire rendering system using 32 ARGB data. The only thing that uses a palette (aka, an array-based lookup table) is the TIA itself. And those indices map to 32-bit colours.

So we're back to the same issue I had at the beginning. How do we adapt the current code to work with a frame that has a constantly changing 256-colour RGB palette? I guess we just re-compute the palette every frame.

Another alternative is to say that phosphor and Blargg don't work together, and release it for 5.0 as-is.

@thrust26
Copy link
Member Author

thrust26 commented Jul 5, 2017

@sa666666

  • I see. But the larger surface is only relevant for the rather simple phosphor calculation, correct? So Blargg would still be as fast as today.
  • If I understand scanlines correct, they are resulting from the beam movement. Which also happens before phosphor comes into play.

For a progressive display (which we have here) in theory things would get more complicated, but as far as I can tell, that's not what Blargg currently emulates.

IF we want to be more faithful here, we would have to create a surface with a double vertical resolution where the scanlines flicker interlaced at 30 Hz (can Blargg create this?). And then add phosphor. IMO something to think about after 5.0.

@thrust26
Copy link
Member Author

thrust26 commented Jul 5, 2017

So we're back to the same issue I had at the beginning. How do we adapt the current code to work with a frame that has a constantly changing 256-colour RGB palette? I guess we just re-compute the palette every frame.

If we apply phosphor after Blargg, then the palette isn't changing. Or do I miss something here?

@sa666666
Copy link
Member

sa666666 commented Jul 5, 2017

If we apply phosphor after Blargg, the colours have still changed; phosphor mode creates colours that didn't exist in the base TIA palette. It does this by definition on each frame, so the Blargg code needs to know about it each frame.

@thrust26
Copy link
Member Author

thrust26 commented Jul 5, 2017

Call me stupid, but I don't get you. :)

Blargg will only see the TIA palette, create new RGB values which will then be processed by phosphor, no?

@sa666666
Copy link
Member

sa666666 commented Jul 5, 2017

@thrust26

Say we have only a 4 colour palette. So the TIA has only 4 colours (and also pretend they are only 8-bit colours), say [0x10, 0x20, 0x30, 0x40].

The Blargg code has a method called initializePalette(). It takes a pointer to theese 4 colours, and processes them in some fashion, generating new colours that it stores internally.

Now, phosphor mode is applied, and all 4 colours have changed. So we need to call initializePalette() again, so that the Blargg code knows about the new colours, and in turn generates new internal colours. Since phosphor mode potentially changes the colours every frame, initializePalette() needs to be called every frame.

Whether phosphor mode is applied before or after Blargg is irrelevant; the colours currently on the screen will be potentially different every frame, and initializePalette() therefore needs to be called every frame.

My initial question was that if there's a way to not call initializePalette() each frame. But there doesn't appear to be.

@thrust26
Copy link
Member Author

thrust26 commented Jul 5, 2017

@sa666666
Agreed with the first two paragraphs.

But then, why do we have to call initializePalette() again? Why does Blargg have to know about colors changed by phosphor or which colors are displayed on screen???

@DirtyHairy
Copy link
Member

@sa666666

I don't understand either 😏 If we leave out phosphor, then Blargg just sees the 256 colors in the TIA palette, and those don't change. If we apply phosphor to the frames after they have been processed by Blargg, then nothing changes as far as Blargg is concerned: it still sees only the TIA palette. Where is the error in my thinking?

@sa666666
Copy link
Member

sa666666 commented Jul 5, 2017

Maybe I am confused now, since two people agree and one doesn't 😄

I think what I'm not stating clearly is that the colours passed to Blargg aren't the ones rendered to the screen. Blargg takes those colours and applies filters to them, and converts them to YIQ format. And those YIQ colours are what is placed in the output framebuffer. So if phosphor mode changes the initial colours, then Blargg must see them, to calculate its own new colours.

@thrust26
Copy link
Member Author

thrust26 commented Jul 5, 2017

Where does phosphor change the initial colors? It comes after Blargg.

Phosphor has to work on the YIQ colors (as I just learned) created by Blargg, not vice versa (Blargg only works on the static TIA palette). And then the result from phosphor is placed into the final framebuffer.

But since we are still discussing, I get the impression that I might be missing some relevant information.

@sa666666
Copy link
Member

sa666666 commented Jul 5, 2017

I think I finally got it 💡 I was still thinking about doing phosphor before Blargg, not after. OK, let's try that approach.

BTW, I just tried my suggestion (changing the palette before calling phosphor), and it's not working. So best if we can just not have to touch the Blargg code at all.

@thrust26
Copy link
Member Author

thrust26 commented Jul 5, 2017

Puh, in between I almost started to question my sanity. 😌
😄

@sa666666
Copy link
Member

sa666666 commented Jul 5, 2017

After 75 comments, we finally finish with issue #75 (is that a coincidence or what?). Implemented in 09af9d0.

@sa666666 sa666666 closed this as completed Jul 5, 2017
@thrust26
Copy link
Member Author

thrust26 commented Jul 5, 2017

That went fast. Does the result look ok?

@sa666666
Copy link
Member

sa666666 commented Jul 5, 2017

Result looks fine to me. I will be posting a pre-10 release tonight.

It ended up not being a difficult problem once we hashed it out back and forth. In particular, I never considered the case of blending after Blargg, so it wasn't even in my mind. Even when you guys pointed it out, it took some time for it to sink in 👿

Sometimes just talking about an approach helps figure things out, even before coding starts.

@DirtyHairy
Copy link
Member

Sometimes just talking about an approach helps figure things out, even before coding starts.

If you have a hammer, everything looks like a nail I suppose 😏

@thrust26
Copy link
Member Author

thrust26 commented Jul 25, 2017

@sa666666 @DirtyHairy Some cool (or not) finding about how slow my LCD is:

The attached ROM looks quite odd on my slow LCD. Without phosphor, the black smiling face shows some heavy trails when moving, with phosphor enabled (blend = 50) it looks much sharper. I would have expected the opposite. So at first glance I thought the modes were reversed and was about to open an issue. But then I realized that it must be my LCD, which doesn't like switching from black to something only slightly brighter.

I suppose the effect is not visible on your LCDs?

Hacker_Face.zip

@DirtyHairy
Copy link
Member

That's interesting. I can see the same effect, although much less pronounced than what you describe: on my laptop screen, I get some slight trails when moving, and enabling phosphor removes them 😏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants