Improve, and always enable, NTSC IQ filters #774
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
atsampson
added
enhancement
ld-decode-tools
Describe the two modes as "equiband" and "narrowband", to match standard terminology: these are the terms used in Carnt and Townsend (1961) and Poynton (2003), so they seem pretty well-established.
These are now FIR filters generated by sps.remez, based on the ones the PAL decoder uses in 1D mode. They're a bit more expensive to compute but they're phase-linear and have less ripple and a sharper cutoff. This looks much better, to the point where I think enabling the filter now slightly improves quality in non-phase-compensating mode. The original Comb::filterIQ only fed alternate samples from I and Q into the filters, so they were designed for 2fSC -- but Comb::filterIQfull, used in phase-compensating mode, was using them at 4fSC, so it wouldn't have been getting the intended response. Comb::filterIQ now feeds all samples through the filter, so it runs at 4fSC and can be used in both modes.
Since it's always on in phase-compensating mode anyway, and it provides a small improvement in non-phase-compensating mode, we may as well remove the option to disable it entirely. Narrowband mode as currently implemented makes a negligable difference to the output, and Poynton says that narrowband encoding "has not been practiced since about 1970", so remove that as well. (Poynton also notes that in theory a narrowband decoder should use a different shape for the I filter as well.)
atsampson
force-pushed
the
ntsciqfilter
branch
from
0713a9a
to
719456e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the result of some experimentation following the discussion in #513...
Comb::filterIQ now uses FIR filters generated by sps.remez, based on the ones the PAL decoder uses in 1D mode. They're a bit more expensive to compute but they're phase-linear and have less ripple and a sharper cutoff. This looks much better, to the point where I think enabling the filter now slightly improves quality in non-phase-compensating mode (and it was already always on in phase-compensating mode), so we can safely remove the options to disable it.
Narrowband Q mode as currently implemented makes a negligable difference to the output, and Poynton says that narrowband encoding "has not been practiced since about 1970" (plus it ought to use a different I filter too), so remove that as well.
Fixes #513.
Some comparisons - note the chroma delay on the blocks at the bottom, and the slight aliasing on the block to the left of "SW2".
No IQ filter (old default):
With the original equiband IQ filter (old default in phase-compensating mode):
With the new equiband IQ filter (new default):
Note to self: update the ld-analyse and ld-chroma-decoder docs when merged.