AppleWin / AppleWin

Apple II emulator for Windows
GNU General Public License v2.0
703 stars 162 forks source link

Various bugs in NTSC.cpp leading to incorrect hues #1166

Open benrg opened 1 year ago

benrg commented 1 year ago

It's been noted that AppleWin's lores color 1 is very purplish compared to other emulators. As far as I can tell this is due to bugs/design flaws in the NTSC code.

The code cites this page (by Robert Munafo) as a reference. According to that page, lores color 1 should have a UV phase of 90 (IQ phase of 33) and amplitude of 60/100. From that I get RGB coordinates (.93, -.10, .25). After clipping and applying the NTSC gamma of 2.2, I get (.86, 0, .05). That's NTSC RGB, not sRGB, but comparing the gamuts it's clear the color should be close to sRGB's #f00 in hue.

There are several problems with the code. First, the 33° offset isn't added to the IQ phase anywhere; the phase is started at 45° (mod 90°). Second, the samples are doubled (from 560 per line to 1120) and the two half-samples are added at 45° increments, which effectively adds another 22.5°, except it's not exactly that because the amplitudes of the values that are added aren't equal. Third, the chroma amplitude is passed through two IIR filters, adding a lot of delay.

Fourth, the IIR filters aren't reinitialized between iterations of the loop. The signal and luma filters are damped enough that I think it doesn't matter, but the chroma filter has a long memory, so the previous bit pattern has an effect on the current bit pattern's chroma.

There's a comment in the source suggesting that the 45° start point (CYCLESTART) was chosen as a fudge factor to compensate for the other phase shifts, but I don't think any fudge factor works here. There's something screwy about the whole design. Even if you feed it a pure sine wave at the chroma frequency, the phase and amplitude never stabilize.

I don't know how real TVs of the period did it, but perhaps what you need is to replace the initFilterChroma oscillator with a 2D IQ-plane oscillator, and apply the phase to the input of it, instead of the output.


I think there are some mistakes in Munafo's page. First, I don't understand why the chroma amplitudes are 60 and 100. The ratio between these should be √2, presumably, so they should perhaps be 71 and 100. This doesn't significantly affect the hue of lores color 1. Second, he says conversion to RGB "requires a gamma correction for display on an RGB monitor", but the NTSC standard says that the RGB values obtained by a linear transformation from YIQ are already gamma corrected (with a nominal gamma of 2.2).

Another issue, as I mentioned above, is that NTSC RGB is significantly different from sRGB. Strictly speaking you should apply the NTSC gamma factor, convert to linear sRGB, then apply the inverse of the sRGB transfer function. The trouble is what to do with out-of-gamut values. Working out the best sRGB approximation to a color not in sRGB is hard.

tomcw commented 1 year ago

Hi @benrg,

It's been noted that AppleWin's lores color 1 is very purplish compared to other emulators.

Where has this been noted? (I think it's the first I've heard about this.)

The code cites this page (by Robert Munafo) as a reference.

I checked the original code for AppleWin's NTSC implementation (submitted as a source code zip by Sheldon Simms in 2011), and it doesn't contain this reference. So someone from the AppleWin team must have added this reference after refactoring the code for inclusion in AppleWin. (GitHub's "blame" says it was me... but I'm very sure it wasn't!)

So I don't think there's any link between the AppleWin NTSC implementation and this Munafo reference.

There's a comment in the source suggesting that the 45° start point (CYCLESTART) was chosen as a fudge factor

I think you are referring to this: https://github.com/AppleWin/AppleWin/blob/597cea1d86bc5dfe697a136c2aca75a497d915ae/source/NTSC.cpp#L930

It's possible that when one of us (AppleWin team members) refactored Sheldon's original code, that we changed the behaviour of something. The original code (wsvideo.cpp) can be viewed here. (Sorry it's a diff - I don't know how to reference an old revision of a deleted file.)

If you want to suggest some code changes to improve things, then feel free to submit a PR (pull request). NB. Please see the CONTRIBUTING doc.

Michaelangel007 commented 1 year ago

So someone from the AppleWin team must have added this reference after refactoring the code for inclusion in AppleWin.

Yes, I did when I noticed Sheldon's colors were slightly off. I documented this in #254, specifically see this comment years ago.

If you examine the first commit when NTSC.cpp was introduced you'll see I added a couple of comments when I noticed Sheldon's colors weren't quite correct.

#if CHROMA_BLUR
                    //z = z * 1.25;
                    zz = signal_prefilter(z);
                    c = chroma_filter(zz); // "Mostly" correct _if_ CYCLESTART = PI/4 = 45 degrees
                    y0 = luma0_filter(zz);
                    y1 = luma1_filter(zz - c);
#else // CHROMA_BLUR
                    y0 = y0 + (z - y0) / 4.0;
                    y1 = y0; // fix TV mode

    #if CHROMA_FILTER == 0
                    c = z; // sharper; "Mostly" correct _if_ CYCLESTART = 115 degrees
    #endif // CHROMA_FILTER
    #if CHROMA_FILTER == 1 // soft chroma blur, strong color fringes
                    // NOTE: This has incorrect colors! Chroma is (115-45)=70 degrees out of phase! violet <-> orange, green <-> blue
                    c = (z - y0); // Original -- smoother, white is solid, brighter; other colors
                    //   ->
                    // c = (z - (y0 + (z-y0)/4))
                    // c = z - y0 - (z-y0)/4
                    // c = z - y0 - z/4 + y0/4
                    // c = z-z/4 - y0+y0/4; // Which is clearly wrong, unless CYCLESTART DEG_TO_RAD(115)
                    // This mode looks the most accurate for white, has better color fringes
    #endif
    #if CHROMA_FILTER == 2 // more blur, muted chroma fringe
                    // White has too much ringing, and the color fringes are muted
                    c = signal_prefilter(z); // "Mostly" correct _if_ CYCLESTART = PI/4 = 45 degrees
    #endif
#endif // CHROMA_BLUR

For comparison here is the original code (Thanks Tom for digging that up!)

#define PI 3.1415926535898
#define CYCLESTART (PI * 4.0 / 16.0)

static void filterloop (void)
{
    int p,s,t,n;
    double z,y0,y1,c,i,q,r,g,b;
    double phi,zz;

    for (p = 0; p < 4; ++p)
    {
        phi = p * PI / 2.0 + CYCLESTART;
        for (s = 0; s < 4096; ++s)
        {
            t = s;
            y0 = y1 = c = i = q = 0.0;

            for (n = 0; n < 12; ++n)
            {
                z = (double)(0 != (t & 0x800));
                t = t << 1;
#if 1
                //z = z * 1.25;

                zz = signal_prefilter(z);
                c = chroma_filter(zz);
                y0 = luma0_filter(zz);
                y1 = luma1_filter(zz - c);

                c = c * 2.0;
                i = i + (c * cos(phi) - i) / 8.0;
                q = q + (c * sin(phi) - q) / 8.0;

                phi += (PI / 4);
                if (fabs((2 * PI) - phi) < 0.001) phi = phi - 2 * PI;

                zz = signal_prefilter(z);
                c = chroma_filter(zz);
                y0 = luma0_filter(zz);
                y1 = luma1_filter(zz - c);

                c = c * 2.0;
                i = i + (c * cos(phi) - i) / 8.0;
                q = q + (c * sin(phi) - q) / 8.0;

                phi += (PI / 4);
                if (fabs((2 * PI) - phi) < 0.001) phi = phi - 2 * PI;
#else

While I could remove the ringing and ghost I never could get Sheldon's color to match my monitor. :-(

It didn't help that I didn't understand his phase initialization code. :-/

is that NTSC RGB is significantly different from sRGB. Strictly speaking you should apply the NTSC gamma factor, convert to linear sRGB, then apply the inverse of the sRGB transfer function

Yes, I wouldn't doubt that there are lots of gamma conversion issues residing in that old code.

Sadly, I'm not a signal engineer so I had to spend a lot of time "reverse engineering" all the magic numbers before I integrated it into AppleWin. Thankfully @sicklittlemonkey was able to provide some context years ago which helped immensely but sadly my knowledge here in (video) signal processing is badly lacking. ("I'm a software guy, not a hardware guy" is my excuse. :-) )

About Marc Ressl's From http://www.digitalfaq.com/forum/video-restore/2944-ntsc-pal-video.html "Regarding the filter: It's a simple Lanczos lowpass filter with variable cutoff.

Edit: Cleanup grammar.