Split decoding from output conversion #635
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
Previously, both the NTSC and PAL versions of the code read from uninitialised memory, and this resulted in garbage data being fed into the filter at the starts and ends of lines. Use 0s explicitly outside the active area instead. Also avoid hardcoding the filter delay, and make the three versions of the code more consistent.
Previously, Comb and PalColour both produced R'G'B' or Y'CbCr output directly. They now produce Y'UV output (with Comb rotating IQ to UV), storing the result into a ComponentFrame output, and the new OutputWriter class converts this to R'G'B' or Y'CbCr. This removes quite a bit of duplication between the PAL and NTSC code, and makes the Y'UV signal available for ld-analyse to use in the future (e.g. so it can show the decoded C on the scope, or UV on a vectorscope). Unlike the old conversion code, OutputWriter's output only includes the active area plus padding, so there's no need to crop it afterwards. The usePadding flag disables padding entirely for ld-analyse. FrameCanvas now draws onto ComponentFrame in Y'UV (rather than onto OutputFrame in R'G'B'), so the visualisations work for all output formats. The transform2/3d visualisations should look the same as before. The ntsc3d visualisation now keeps the original Y and just replaces UV, so the colour scheme is generally the same but it'll look a bit different. Since the noise reduction code now operates on the ComponentFrame, it would be possible to make a generic version that would work for all decoders. The chromaGain option is still handled by the encoders because it can be done efficiently there as part of the UV rotation matrix. Comparing the output with the previous version: mono output is identical, and RGB and YCbCr output is either identical or +/- 1 value (for PAL, this is because cbScale/crScale weren't quite right before; for NTSC, a different RGB conversion matrix is now being used which is only given to 6 sf).
After discussion, we think this should be 256 to match the others given the refactored scaling calculations...
atsampson
added
enhancement
ld-decode-tools
To be precise, this is a rotation applied to the I/Q or U/V components after filtering and demodulation but (for PAL) before the V-switch. For NTSC it acts as a hue control. For PAL, it can be adjusted to minimise Hanover bars when using the Simple PAL decoder. The full adjustment range (-180 to +180 degrees) is allowed, but since the adjustment will generally be small in practice, the slider in ld-analyse uses a non-linear scale.
|
Chroma phase control added. |
|
Just wanna note that for non-laserdisc things, especially CVBS, it would be nice if the code accommodates for that there can be sources like PAL-M, PAL-N and more obscure formats that are not 625-line PAL or 525-line NTSC. Would probably still be input so that the sample rate 4fsc of whatever sub carrier frequency was used for ease of decoding, so it's mainly about accounting for that line count and width can be different than standard PAL/NTSC. |
|
Yes - if there's enough interest in supporting other standards, we could replace isOutputPal in the metadata with a field specifying a standard name, and then the metadata reader can fill in the other details based on that. (I wonder what a sensible sampling rate for mono-only standards like UK 405 and French 819 lines would be... it would be easy enough to generate composite samples of these with hacktv.) |
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.
Previously, ld-chroma-decoder's PAL, NTSC and monochrome decoders all produced R'G'B' or (after @ifb's work in #618) Y'CbCr output directly. This made for a fair amount of duplication between the different decoders.
This set of changes adds a ComponentFrame class, which holds a complete frame of Y'UV data, and an OutputWriter class that can convert a ComponentFrame to any of the supported output formats (outputting just the selected active area, so there's no need to crop afterwards). The Decoder classes now just produce ComponentFrames, with the NTSC decoders rotating IQ to UV, and output is handled centrally.
Besides simplifying the decoders, this also means that unscaled Y'UV is now available to ld-analyse - for example, to show real Y/C on the line graph or to implement a vectorscope - see #305 and #570.
I've left chroma gain as part of the decoder configuration rather than bringing it into OutputWriter because (a) it can be done efficiently as part of the UV rotation and (b)
the next plan is to addthe last commit adds a chroma phase control, which needs to be done in the decoder (because it has to be before the V-switch for PAL).I've checked that this produces (effectively) the same output as the old code, and that all the performance-critical bits vectorise properly with GCC. Testing with valgrind revealed some uninitialised reads in the noise reduction code so there's a fix for that included as well.
Edit: I should note that @ifb has already had a look through this and sent me some suggestions which I've incorporated.