Extend color range note to explain limited range #1154
Conversation
meta-cla
bot
added
the
CLA Signed
NicolasHug
left a comment
NicolasHug
left a comment
There was a problem hiding this comment.
Thanks for the PR @Dan-Flores . It looks good, I left a few comments and questions.
One general comment: The new section is slightly at odds with the existing note structure. The existing note is mostly about decoding (not encoding), and largely about YUV -> RGB conversion. The last section for example mainly refers to nppiNV12ToRGB_8u_ColorTwist32f_P2C3R_Ctx, not to nppiRGBToNV12_8u_ColorTwist32f_C3P2R_Ctx, and the intro specifically mentions decoding. The new section doesn't flow very well within this structure (and switches from a decoding context to an encoding context without notice).
Re-writing the entire note to make it holistically consistent would be a fair amount of work, and it's probably not worth it. I'd suggest to insead write the new section you added into a separate place. For example, in the CudaDeviceInterface code, where the new matrices are. Maybe this can just be included as part of #1154 ?
| // This yields: | ||
| // tensor([[ 0.1826, 0.6142, 0.0620], | ||
| // [-0.1006, -0.3386, 0.4392], | ||
| // [ 0.4392, -0.3989, -0.0403]]) |
There was a problem hiding this comment.
Ideally, could we find an authoritative source from which we can validate that these values are correct? Something similar to what we have for the full range:
It doesn't have to be for 709 specifically, but validating it for one limited range config would be great.
There was a problem hiding this comment.
I'll try to find a source that gets our exact values. There is one section on that wiki page that gets close, but results in different values due to an optimization: https://en.wikipedia.org/wiki/YCbCr#Approximate_8-bit_matrices_for_BT.601
| // - RGB [0,255] -> Y [16,235]: compress by (235-16)/255 ≈ 219/255 | ||
| // - RGB [0,255] -> U,V [16,240]: compress by (240-16)/255 ≈ 224/255 |
There was a problem hiding this comment.
Is this meant to be "≈" or should it be "=" ?
| // | ||
| // | ||
| // Next, lets consider encoding RGB -> YUV in the limited color range. | ||
| // Y is in the range [16-235] and U,V are in [16-240]. |
There was a problem hiding this comment.
Reading this might be slightly confusing because the first part of the note above started with
Conventionally Y is in [0, 1] range, and U and V are in [-0.5, 0.5]
And that's also how the matrices below are defined. I think we can get around this by saying something like:
In limited range, the [0, 255] range is mapped into [16-235] for Y, and into [16-240] for U and V.
|
Closing this PR and moving the note into #1125. |
2 participants
This PR extends the note on color range to mention how limited range matrices are generated.
This method is used to generate the matrices in #1125.