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

PR: Implement support for Y'CbCr matrices generation. #664

Merged
merged 2 commits into from
Jan 14, 2021

Conversation

KelSolaar
Copy link
Member

References #486.

@KelSolaar KelSolaar requested a review from a team December 23, 2020 20:30
@KelSolaar KelSolaar added this to the v0.4.0 milestone Dec 23, 2020
@coveralls
Copy link

coveralls commented Dec 23, 2020

Coverage Status

Coverage increased (+0.0003%) to 99.721% when pulling 2181c93 on feature/ycbcr_matrices into d716e14 on develop.

@KelSolaar KelSolaar force-pushed the feature/ycbcr_matrices branch 6 times, most recently from 7d6e538 to 9498041 Compare December 24, 2020 02:38
@KelSolaar
Copy link
Member Author

@nick-shaw : Might be worth one of your eyes! :)

@nick-shaw
Copy link
Contributor

Couple of points and/or references: in Supplement 18 of ITU-T (page 24) a matrix with 14 digits after decimal point is given and is to be used for Y′CbCr to R′G′B′.

This function would be able to calculate those 14 decimal place coefficients (or higher precision if desired). The 14 decimal place coefficients are just the 4 decimal place constants from BT.2020 run through the weighting to matrix calculations.

@nick-shaw
Copy link
Contributor

>>> np.set_printoptions(suppress=True, formatter={'float_kind':'{:0.14f}'.format})
>>> matrix_YCbCr(K=WEIGHTS_YCBCR['ITU-R BT.2020'])
array([[1.00000000000000, -0.00000000000000, 1.47460000000000],
       [1.00000000000000, -0.16455312684366, -0.57135312684366],
       [1.00000000000000, 1.88140000000000, 0.00000000000000]])

This is exactly the matrix shown in the document.

Copy link
Contributor

@nick-shaw nick-shaw left a comment

Choose a reason for hiding this comment

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

Looks good to me, except perhaps the docstring should be more explicit that the returned matrix is the Y'CbCr to R'G'B' matrix, not the R'G'B' to Y'CbCr matrix. Other than that I am happy to approve the PR.

@KevinJW
Copy link

KevinJW commented Jan 6, 2021

I haven't fully looked at this, but I wonder if ITU T-REC-H.273 might have something to say on these, at least it states the expected behaviours for a number of the combinations. It does appear to be attempting to be definitive: This Specification can be used to provide universal descriptions to assist interpretation of video and image signals following decoding or to describe the properties of these signals before they are encoded

Kevin

@nick-shaw
Copy link
Contributor

Not sure I completely follow, @KevinJW. Are you suggesting we adopt the "Video Code Points" values as an (optional?) way of passing the K and is_legal arguments to the functions? This feels like it is more a part of the metadata layer that we do not yet have. Ideally the functions would also return metadata describing how the result was encoded.

@nick-shaw
Copy link
Contributor

nick-shaw commented Jan 13, 2021

BTW, can you add Kodak Photocd quantization?

The RGB_to_YCbCr function can already use arbitrary output scaling and offsets, using the out_range argument. See here for details. So the PhotoCD quantisation could be achieved using that together with the out_int argument.

@KelSolaar
Copy link
Member Author

Thanks for the review @nick-shaw! I addressed the docstring notes and rebased on-top of develop. I will merge after the tests pass.

@KevinJW
Copy link

KevinJW commented Jan 14, 2021

The numbers in that line in FFmpeg would be my "fault", can't remember exactly what source I used for it

Kevin

@KelSolaar KelSolaar merged commit 31e3b7a into develop Jan 14, 2021
@KelSolaar KelSolaar deleted the feature/ycbcr_matrices branch January 23, 2021 22:05
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

4 participants