Menu
▾
▴
#667 Color emphasis not functional
Color emphasis bits don't appear to work properly. All of them emphasize blue?
The ROM here demonstrates multiple color issues with FCUEX:
http://wiki.nesdev.com/w/index.php/Full_palette_demo
Old/New PPU setting doesn't seem to have an effect on this problem.
looks more like "Direct color control" doesn't work right. Color emphasis has been understood for a long time, but that level of detail of the PPU unusual behaviours hasnt been understood for a long time.
I'm trying to write a better test that demonstrates the problem, since the palette demo is demonstrating more than just the emphasis problem.
I think emphasis works if it's for the whole frame, but if I try to change it in the middle of the frame, I only seem to get blue. I'll post again when I manage to make a test ROM that shows this.
Here's a ROM that demonstrates the problem. Emphasis changes done mid-screen appear to be broken in some way; they may only partially take effect depending on what the emphasis was at the start of rendering?
Also, the particular cycle of emphasis colours is a bit different than what I see on my NES.
bits - expected (FCEUX)
000 - white (white)
001 - red (yellow)
010 - green (green-cyan)
011 - dim yellow (yellow)
100 - blue (blue)
101 - dim magenta (red)
110 - dim cyan (strong green)
111 - dark (dark)
Last edit: rainwarrior 2014-08-12
It seems likely that FCEUX only applies deemph once per frame, since it runs deemph by calculating a new palette. This code in FCEUX is far beyond f***ed and shouldnt be touched unless someone's willing to own it for a long time.
regarding your opinions on what the colors should be, do you know if any other emulators are using full palettes for all the deemph states? it should be 512 entries for 64 colors * 8 deemph states. That's the way I'd like to implement it.
FCEUX has some numerical calculations which don't appear to be pulled out of asses, so according to at least one other person's opinion, they must be correct.
Ahh, well if it's that complex to fix, I'll put it on my to-do list and take a hard look at the code in a few days when I have time.
Hmm, is FCEUX limited to a 256 color output palette? We'd need 512 to cover all the states, unfortunately.
It looks like colors 0-63 are used for the regular palette (no emphasis bits set) and 64-127 for if any emphasis bits are set. There is code in ppu.cpp that tries to find the most commonly used emphasis setting over the course of the frame (a per-scanline check) and uses that for the emphasis palette, so it's capable of rendering an emphasis on/off toggle mid-frame, but not more than one emphasized state.
I don't know where the de-emphasis color table came from, but some values are clearly wrong. Here are the ones that stand out to me in order of decreasing severity:
palette.cpp:125 > .101 is nonsense, should be something like .98?
palette.cpp:114 > 1.026 is wrong, green should be de-emphasized here, maybe .8 instead?
palette.cpp:111 > .915 is too high for de-emph green, maybe .8?
palette.cpp:115 > .908 is too high for de-emph green, maybe .8?
So... the strange emphasis colours can be addressed with the minor tweaks I've just suggested (though my suggested values are "out of ass", I still think they're more worthwhile than the very incorrect colors in use). That's the only thing I'd suggest doing right now.
In the longer term, it seems like arbitrary emphasis changes are a bit of a pipe dream. I mean, by complicating the current system further we could support 3 different emphasis states and emphasis-off in the same frame. To do more than that seems like it would require a cross-platform rewrite of how colors get to the user's video system.
It would also be nice to improve the palette system to store a 512 color palette instead of 64, and do all the emphasis calculations on load rather than per-frame, or if someone supplies a 512 entry palette just take the emphasis colors directly from that. (Maybe while I'm at it I can figure out where the existing de-emphasis tables come from, or create a total replacement.)
So... I'll poke around at the longer term ideas as I have time. If I can improve the situation cleanly enough I'll submit a patch. For now, consider at least changing palette.cpp line 125 which I think has to be a typo. The other 3 lines, change at your discretion, and I'll try to come up with something better later.
I'm not going to entertain patches which consist of someone fine-tuning some random colors to be more to their liking. The deemph colors depend on the exact PPU revision, and theres no one correct set. If you can find a derivation for these figures and ID typos, then I might be interested. I know you said basically that, I'm just summarizing my thoughts.
https://github.com/libretro/bnes-libretro/blob/master/libretro/libretro.cpp
Look, bnes uses the same numbers, from where?
If you're going to dig into this, I recommend TOTAL OVERHAUL. Half measures will just leave things broken. I approve of the 512 element palette, in fact, just today mednafen adopted it, and bizhawk will be as well. The current code is so much garbled nonsense that if it's going to be broken, I'd rather it be broken in the direction of clean no-nonsense.
One thing to be aware of is this MYSTERIOUS CODE
ppu.cpp line 1794 (what the hell?)
ppu.cpp line 791 (only set it when 2001.RGB != 0???)
But I think you can just rip it all out. It isn't that complicated! its just effectively a 512 entry palette, right?
Well, having spent time on it, I can explain the mysterious code:
ppu.cpp:791 is becaue "deemph" is only used to select the output palette for colours 64-127. When the pixels are rendered, the emphasis is applied with "PPU[1]>>5". It's bizarre, but not actually a problem, functionally.
ppu.cpp:1794 is the thing that counts (once per scanline) which demphasis bits were in effect, trying to pick the one that is used for most of the frame, so that when it has to pick just one (to apply to the output palette 64-127) it can be the least incorrect about it. Note that the cases where x=0 (no emphasis) or x=7 (all bits on) are skipped because they are always present in the output palette.
Not that I think either is a good way to do it, just that within the scope of the current "solution" to support one non-zero emphasis setting per-frame, there is at least a reason for that code.
I understand how the renderer applies the emphasis, the bottleneck is the 256 colour output palette, which is currently allocated like this:
0-63 is reserved for 7 special colours used by FCEUX (overlay, etc.)
64-127 is the most-used emphasis setting per frame
128-195 is the palette with no emphasis
196-255 is the palette with all emphasis bits on
Changing the PPU output from indexed 8 bit to something wider (16 bit indexed? 32 bit RGBA?) seems like a massive change that goes all the way into the platform specific video output stuff, so... I would be really hesitant to go into that. I can't think of a good solution for arbitrary emphasis changes that doesn't involve doing this, though. (There's also a whole bunch dependencies on the existing palette scheme in drawing.cpp that uses the 196-255 palette for dim colours, etc.)
I will consider upgrading the internal palette representation from 64 to 512 colours, though, and finding a better basis for the emphasis colours. At least this would put correction of the emphasis colours in the hands of a user via custom palettes, and at the same time make a future conversion to >8bit output colour easier to do.
I will still assert that the value for palette.cpp:125 is dead wrong, but obviously I can't suggest what the "correct" value would be in the scheme that was used. The other three I suggested are off to the point of red being yellow, etc. Lately a few people, notably Bisqwit, have been attacking it from an NTSC signal generation basis, which I think is probably the right way to go. As for revisions of the PPU, I have seen reports that the PAL PPU has red/green emphasis swapped, and the RGB PPU works by maximizing one component rather than darkening the others. I sincerely doubt the inconsistencies in FCEUX's emphasis table can be explained by any PPU revision, though I understand why you want something better than my ballpark suggestions.
the PPU output should be 16 or 32bit and let the paletteizing be done by a different layer which uses a 512 color palette. Mednafen just adopted this approach, and bizhawk is going to soon.
Finally, there are many people who have used fceu for so long that whatever it does is considered right. For example there are hardware RGB upscalers that provide 'fceu' as a palette option. So even if it's wrong, it's a standard.
Having studied this subject a little more along with you, it now seems like it might not be too terribly hard to rip out most of the palette and deemph code (it's all garbledy nonsense) and replace it with about 5% as much code. Not really very hard at all. If you can wait a few weeks, I'll probably do it.
I've done the bulk of the work to make FCEUX palettes support full 512-size palettes. palette.cpp now makes sense. Additionally I added an 'XDbuf' in parallel to 'XBuf' which contains deemph bits. Therefore all the old codepaths can remain unmodified. The deemph bitplane gets incorporated at the final display stage (Blit8ToHigh). It works great.. unfortunately, that's just one of several codepaths. Several others are right near it. Another is screenshots (8bit pngs are written). I can probably handle all those, but there may be others I didn't notice, and I want you aboard to bugtest it before I commit it, if you're game. Let's discuss it further on IRC
Wouldn't committing the WIP gain more testers?
Yes, for regressions, but I'm not sure I want to commit it at all unless the other expert on deemph and palettes is here. But I'm glad youre paying attention too.
I think I fixed all the codepaths now, but I'm not 100% sure. I didnt test the 24bpp desktop bitdepth codepath.
I tested it against the emphasis-using games I know of. Looks like the change is working fine, I don't see any regressions so far.
Multiple emphasis splits per-frame now appears to work very well, per-scanline at least. (I like this a lot, as I use emphasis for profiling my game.)
The palette generator's set of colours for emphasis is still wrong, as I pointed out before, but that's just the same as it was.
The "full palette" demo I mentioned in the first post doesn't fully work, of course (we'd need mid-scanline colour effects for that) but the emphasis portion of it appears to be correct now, which is good.
We haven't found a single 512 color .pal in google. What were you testing against, guys?
There's at least one 512-entry palette in bizhawk (it's quicknes's). However while researching this, I discovered it doesnt match the table in our nes_emu.cpp precisely, so I don't know where I got our "quicknes" palette from.
rainwarrior: I attempted to add code from nes_ntsc which calculates the deemph colors, but i dont get the exact results you listed. it's close enough, except for "100 - blue (blue)" which is just some kind of pink with this code. Maybe I've done something wrong while adapting it. I'll make the deemph switchable between classic and nes_ntsc method via the gui in the future but for now ill leave it on the new ntsc method
feos:
//index += 256; // feos: why?
because the first 256 palette entries are for the old palette and the next 512 entries are for the fully expanded. see:
palrgb = (uint32 )FCEUdmalloc((256+512)16sizeof(uint32));
and the creation palettetranslate in SetPaletteBlitToHigh().
But I found other bugs that explained why you had to do that.
Hmm, I don't yet have good knowledge of how NTSC analog signals work, so I can't offer any tips for debugging that stuff from nes_ntsc.c, but the method used looks similar to this one outlined by bisqwit:
http://forums.nesdev.com/viewtopic.php?t=8209
His method seems to divide each color into 12 clock cycles, and apply a transformation on each cycle appropriate for that phase? The code from nes_ntsc.c appears to try to do something equivalent in a single step, though, looking up from a tint table to decide how much to rotate the chroma coordinate?
Bisqwit also posted pictures with the expected results (which look very much how I expect, and looks like what I see on my NES + TV).
Some useful reference here: http://wiki.nesdev.com/w/index.php/NTSC_video
What the nes_ntsc.c code appears to be doing is:
1. Reduce luma by some attenuation amount. (y -= sat * 0.5f;)
2. If two or more tint bits are set, increase the reduction. (sat = 0.6f; y -= sat;)
3. Bias the chroma coordinate toward the chosen colour. (i/q += TO_ANGLE_SIN/COS(tint_color) * sat;)
The reason we're erroneously getting "pink" tor tint=4 and "yellow" for tint=1 appears to be that just biasing the chroma like this causes an oversaturation and the values are getting clamped. So, for example the "blue" phase (tint_color = 8) eventually converts to something like a strong +B and a weak +R to the resulting RGB, but the clamping causes de-emphasizes the +B and over-emphasizes +R.
If the description on that wiki page, and bisqwit's code based on it are accurate (and I believe they are), the approach in nes_ntsc.c is incorrect. The emphasis should not bias the chroma uniformly, the chroma is the sum of 12 phased components, each of which is ether off, on, or attenuated, I think? The key difference I think is that a color with no red in it should not get biased towards red by the tint operation (only half of the colours should be shifted toward red), it would just be attenuated, but in nes_ntsc.c's approach, everything gets the same luma attenuation, and everything gets the same chroma bias.
For example, I can change "y -= sat * 0.5f;" to just "y -= sat;" to increase the attenuation, which removes the colour clamping problem and we get colours that look correct, but everything is now too dark.
Not really suggesting this as a solution, since I don't understand quite where all these values are coming from, but also even if we "fixed" it to defeat the clamping somehow, it's still the incorrect thing to do to the signal. I'll see if I can figure something out based on that wiki method / bisqwit's method later this evening.
I just looked at bisqwit's method and I think it's probably better than the nes_ntsc approach. I had to come up with something weirdly mutated though to make it work by modifying an existing palette.. it's checked in now, take a look. I think it finally gives colors something like what you expect.
Sure, what you've done looks okay for the generated palette.
Don't we still need an RGB emphasis conversion for palettes that only have 64 entries though? Tricky to try and work backwards from RGB, but maybe there's no real "accurate" way to do that. I think I could get something close, though, by approximating the phase.
Oh, I just took a look at how you did it. From what it looks like you basically apply an RGB tweak based on how that palette entry would have transformed under bisqwit's model?
That's probably a good way. It would apply the emphasis tweak according to the palette entry index, rather than the incoming RGB, which might actually be more sensible than treating each RGB one as if it was an individual colour to be emphasized on its own merits.
yes, it sounds like youre reading the code the same way I am.
I'm not sure there's an accurate way to do it either. You can see how I did it in the new bisqwit and ntsc ApplyDeemphasis functions, maybe find a better way.
Presently our runtime-created NTSC palette is only 64 entries and then munged by the stuff I just did. That isn't great, we also need to have a more modern NTSC palette based on a nes_ntsc (with the tint and contrast controls). It'd be best if we could combine that somehow with bisqwit's stuff. I think we can, right? It's like, some YIQ-space fiddling around derived from nes_ntsc after bisqwit's basic palette generation?
Presently our default internal palette is only 64 entries and then munged by the stuff I just did, and I feel no need to change the default.
Last edit: zeromus 2015-09-17
Yes I think what you did is good. It's simpler in approach than what I was thinking of (i.e. trying to esimate phase based on RGB and rebuilding from there), and it's good enough. People that want a specific behaviour can make 512 entry palettes.
Speaking of which, perhaps the next step is updating the .pal files in the palettes folder to each have 512 entries. Maybe at this point it would be a good idea to make a set of RGB PPU palette files with the alternative RGB emphasis behaviour, based on: http://wiki.nesdev.com/w/index.php/PPU_palettes#2C04
Also the revised palette file format deserves a mention in the documentation somewhere. (I haven't figured out where the docs are on the repository yet.)
I could do this stuff later today.