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

Remove burst-based chroma gain calculation #481

Merged
merged 8 commits into from
Apr 24, 2020

Conversation

atsampson
Copy link
Collaborator

This is a "request for comments" PR rather than a "merge immediately" PR :-)

At the moment, the NTSC and PAL decoders adjust the gain they apply to the IQ or UV signals based on the medianBurstIRE value that ld-decode reports in the JSON. According to the comments, this was intended to act as a makeshift MTF compensator, before ld-decode was able to do that itself.

Now that ld-decode has working MTF compensation, I think this mechanism is probably more trouble than it's worth - the relationship between the burst size and the gain applied depends on a fudge factor (different for PAL/NTSC), and when feeding ld-chroma-decoder with known-correct data you have to provide a synthetic medianBurstIRE value and disable the fudge factor.

This set of changes stops ld-chroma-decoder from looking at medianBurstIRE entirely, so the chroma gain is just based on the black/white levels in the TBC (see comments in the first commit). For troublesome discs, --chroma-gain now defaults to 1.0 and works for both PAL and NTSC, and ld-analyse gets a chroma gain slider.

(Somewhat related to #478 in that the current video LPF rolls off a bit more PAL chroma than it should.)

@atsampson atsampson added enhancement ld-decode-tools An issue only affecting the ld-decode-tools labels Apr 11, 2020
@atsampson
Copy link
Collaborator Author

ld-analyse question for @simoninns : the chroma gain slider would be useful for both NTSC and PAL. But should there be a separate "NTSC chroma decoder settings" dialog, or a shared dialog for both? (I'm leaning towards the latter since there aren't that many settings overall...)

@simoninns
Copy link
Collaborator

I'm fine with the changes - I think, on the dialogue side of things, perhaps a tabbed dialogue is better with the PAL and NTSC settings in the same window on separate tabs. Keeps the overall number of dialogues down, whilst still keeping the UI obvious and clean.

This is no longer necessary because ld-decode has MTF compensation and
produces reasonable chroma levels by default.

--chroma-gain now defaults to 1.0, and works for both PAL and NTSC.

As with the old PAL code, the NTSC YIQ to RGB conversion wasn't applying
an appropriate scaling factor to the IQ components. Poynton "Digital
Video and HDTV" 1st edition p514 says that the scaling factor is the
same as for Y -- i.e. when 7.5% setup is in use, the chroma range is
proportionately smaller.
As with PAL, we have about 5 s.f. in our samples, so the 6 s.f.
constants are worthwhile.
No new UI yet, but ChromaDecoderConfigDialog now has both sets of
settings, the PAL widgets are disabled in NTSC mode, and the chroma gain
slider works for both.
It's true when white is at 75 IRE (and this matches the name of the same
option in the RGB class).
@atsampson
Copy link
Collaborator Author

Now with fancier PAL/NTSC chroma decoder options window. The options not applicable to the current standard are greyed out as before, and the tab for the current standard is selected automatically when you open a file.

cdc-pal
cdc-ntsc

@happycube
Copy link
Owner

happycube commented May 24, 2020

As I said in #490, I think this should eventually be reinstated but in improved form, using VITS to guide it - there's usually a lot of info there that ld-decode's never used. When MTF compensation isn't working correctly (or sometimes too far into a CAV disk for it to have any) the color burst is non-linear with luma, and it can change during the disk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ld-decode-tools An issue only affecting the ld-decode-tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants