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

Add BT.601 YCbCr output #618

Merged
merged 1 commit into from
Feb 15, 2021
Merged

Add BT.601 YCbCr output #618

merged 1 commit into from
Feb 15, 2021

Conversation

ifb
Copy link
Contributor

@ifb ifb commented Feb 13, 2021

Add planar YCbCr output (YUV444P16 or GRAY16).

Copy link
Collaborator

@atsampson atsampson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this - the overall approach looks good, and it's nice to see references for the maths!

I've added a few review comments - most of these are minor stylistic things, but you can also simplify how the Configuration stuff works (by making MonoDecoder work like the PAL/NTSC decoders).

tools/ld-analyse/tbcsource.cpp Outdated Show resolved Hide resolved
tools/ld-chroma-decoder/comb.cpp Outdated Show resolved Hide resolved
tools/ld-chroma-decoder/comb.cpp Outdated Show resolved Hide resolved
tools/ld-chroma-decoder/comb.h Outdated Show resolved Hide resolved
tools/ld-chroma-decoder/decoder.cpp Outdated Show resolved Hide resolved
tools/ld-chroma-decoder/ycbcr.cpp Outdated Show resolved Hide resolved
tools/ld-chroma-decoder/ycbcr.cpp Outdated Show resolved Hide resolved
tools/ld-chroma-decoder/ycbcr.cpp Outdated Show resolved Hide resolved
tools/ld-chroma-decoder/ycbcr.h Outdated Show resolved Hide resolved
tools/ld-chroma-decoder/main.cpp Outdated Show resolved Hide resolved
@atsampson
Copy link
Collaborator

atsampson commented Feb 13, 2021

Incidentally - have you tried my testsuite for ld-chroma-decoder with your changes? If you clone this repo, the try-chroma-decoder script has a fairly extensive set of tests, many of which can work using the files in ld-decode-testdata; it should produce the same output before your changes and after.

@ifb
Copy link
Contributor Author

ifb commented Feb 13, 2021

@atsampson Do you want changes as a separate patch or squashed into this one? If I push -f to my branch, I'm not sure what GitHub does to the merge request and your comments. I could always squash later?

@ifb ifb force-pushed the add-ycbcr branch 6 times, most recently from 8499614 to 71bfb27 Compare February 14, 2021 06:26
@ifb
Copy link
Contributor Author

ifb commented Feb 14, 2021

Incidentally - have you tried my testsuite for ld-chroma-decoder with your changes? If you clone this repo, the try-chroma-decoder script has a fairly extensive set of tests, many of which can work using the files in ld-decode-testdata; it should produce the same output before your changes and after.

I cloned it a long time ago, but never used it since I didn't have any of the test LD files. I recently downloaded GGV1011 so I'd have something that's PAL, so try-chroma-decoder is running with that right now. I don't know if my laptop can decode it before I have to leave with it.

@ifb ifb requested a review from atsampson February 14, 2021 06:34
Copy link
Collaborator

@atsampson atsampson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks good to me now - thank you very much!

I've just run my testsuite over it and it produces bitwise identical output to master for all the existing tests (so I guess I'll have to write some YCbCr tests now...).

I'd suggest squashing the revisions into your original changes in this case - it doesn't make much difference here but it makes the history a bit tidier if anyone needs to do a bisection in the future.

@ifb ifb force-pushed the add-ycbcr branch 2 times, most recently from b621c16 to e8b24d9 Compare February 15, 2021 01:21
ld-chroma-decoder: add the -p/--output-format option to select between
'rgb' and 'yuv' output formats.

'rgb' is the default and always outputs 16-bit packed RGB48 data. 'yuv'
outputs 16-bit planar YUV444P16, unless the mono chroma decoder is used
and then 16-bit GRAY16 is output instead.

Note that YCbCr is often mistakenly refered to as YUV, a convention we
continue here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants