Skip to content

Commit

Permalink
Fix YUV->RGB shader (YCbCr->RGB really).
Browse files Browse the repository at this point in the history 
WebRTC SDK got it wrong.

See https://web.archive.org/web/20180421030430/
http://www.equasys.de/colorconversion.html
  • Loading branch information
@john-preston
john-preston committed Jun 4, 2021
1 parent 45c476d commit 06f7b1d
Showing 1 changed file with 8 additions and 2 deletions.
10 changes: 8 additions & 2 deletions ui/gl/gl_shader.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,16 @@ uniform sampler2D u_texture;
uniform sampler2D v_texture;
)",
.body = R"(
float y = texture2D(y_texture, v_texcoord).r;
// float y = texture2D(y_texture, v_texcoord).r;
float y = texture2D(y_texture, v_texcoord).r - 0.0625;
float u = texture2D(u_texture, v_texcoord).r - 0.5;
float v = texture2D(v_texture, v_texcoord).r - 0.5;
result = vec4(y + 1.403 * v, y - 0.344 * u - 0.714 * v, y + 1.77 * u, 1);
// result = vec4(y + 1.403 * v, y - 0.344 * u - 0.714 * v, y + 1.77 * u, 1);
result = vec4(
1.164 * y + 1.596 * v,
1.164 * y - 0.392 * u - 0.813 * v,
1.164 * y + 2.17 * u,
1.);
)",
};
}
Expand Down

4 comments on commit 06f7b1d

@Tynach
Copy link

@Tynach Tynach commented on 06f7b1d Apr 2, 2022

Choose a reason for hiding this comment

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

@john-preston, this should work mostly fine for BT.601-encoded videos, but won't work correctly for videos encoded with BT.709 or BT.2020. I say 'mostly fine' only because you missed the 0 in 2.017 on line 85 (now line 88 in the current version of the file (as of 2022-04-02)).

Personally, I would recommend performing a matrix multiply and using vectors instead of individual float values. GPUs can better optimize and better parallelize operations when GLSL code is organized to leverage native data types and sizes, with relevant data grouped together.

Unfortunately, it also looks like the GLSL has been heavily split apart into multiple pieces now, which makes it harder to find and fix this type of thing, especially with single-letter variables that are now separated from when they're defined (making it unclear what exactly they represent).

However, it's also a good thing it's split up so much - that gives precedence to define multiple alternate YUV→RGB conversion matrices, which can be used depending on the metadata or resolution of the data. Video codecs like H.264 can indicate which variation of YUV was used to encode the video, which affects which variation to use when decoding it.

Here's an example of how I'd personally organize this (note: the fractions are simply more precise versions of the matrix coefficients you used):

  • Fragment for BT.601: 
    const mat3 yuvToRgb = mat3(
	85.0/73.0, 85.0/73.0, 85.0/73.0,
	0.0, -1287801.0/3287200.0, 22593.0/11200.0,
	35751.0/22400.0, -10689549.0/13148800.0, 0.0
);
  • Fragment for BT.709: 
    const mat3 yuvToRgb = mat3(
	85.0/73.0, 85.0/73.0, 85.0/73.0,
	0.0, -28469543.0/133504000.0, 236589.0/112000.0,
	200787.0/112000.0, -71145527.0/133504000.0, 0.0
);
  • Fragment for BT.2020 (non-constant luminance): 
    const mat3 yuvToRgb = mat3(
	85.0/73.0, 85.0/73.0, 85.0/73.0,
	0.0, -94831967.0/506240000.0, 479757.0/224000.0,
	376023.0/224000.0, -329270807.0/506240000.0, 0.0
);
  • Fragment for decoding YUV 4:2:0 (specifically FragmentSampleYUV420Texture(), though this is just an example of the desired format; this would be applied to the other variations as well): 
    vec3 yuv = vec3(
	texture2D(y_texture, v_texcoord).a,
	texture2D(u_texture, v_texcoord).a,
	texture2D(v_texture, v_texcoord).a
) - vec3(16, 128, 128)/255.0; // 0.0625 and 0.5 are slightly incorrect, apparently
  • Fragment for converting to RGB (replaces the contents of FragmentYUV2RGB()): 
    result = vec4(yuv*yuvToRgb, 1.0);

Generally, if a video file isn't tagged, resolutions lower than some resolution (chosen arbitrarily to be somewhere between 480p and 720p) are treated as BT.601, and anything higher than the arbitrary cutoff point is treated as BT.709. BT.2020 is significantly different and requires further processing, especially if it's the 'constant luminance' variation.

Other colorspace metadata that mp4 and mov files might include (and I think webm can as well, but I'm not as sure about that) is for the RGB colorspace, those primarily being the gamma correction standard used and the color primaries. That's all beyond the scope of what I'm talking about here, though it's a topic I've studied in full and already have extensive experience implementing in GLSL.

@Tynach
Copy link

@Tynach Tynach commented on 06f7b1d Apr 2, 2022

Choose a reason for hiding this comment

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

I apologize if the above comment caused 3 notifications to be sent off. I accidentally hit Ctrl+Enter twice while still writing it, finger slipping off of 'Shift' (which copies the whitespace from the previous line to the new line being created, useful while writing those code snippets). I then promptly copied the contents of the incomplete posts, deleted the posts, and kept working.

@Tynach
Copy link

@Tynach Tynach commented on 06f7b1d Apr 2, 2022

Choose a reason for hiding this comment

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

Alternatively, if you want to avoid the weirdness of wrapping the operation at the end in vec4 with the trailing 1, you could instead turn those mat3s into mat4s like this:

const mat4 yuvToRgb = mat3(
	85.0/73.0, 85.0/73.0, 85.0/73.0, 0.0,
	0.0, -1287801.0/3287200.0, 22593.0/11200.0, 0.0,
	35751.0/22400.0, -10689549.0/13148800.0, 0.0, 0.0,
	0.0, 0.0, 0.0, 1.0
);

And then the last couple fragments become:

  • Decoding YUV 4:2:0: 
    vec3 yuv = vec3(
	texture2D(y_texture, v_texcoord).a,
	texture2D(u_texture, v_texcoord).a,
	texture2D(v_texture, v_texcoord).a,
	1.0
) - vec3(16, 128, 128, 0)/255.0;
  • Converting to RGB: 
    result = yuv*yuvToRgb;

@Tynach
Copy link

@Tynach Tynach commented on 06f7b1d Apr 2, 2022

Choose a reason for hiding this comment

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

While tempted to bring up the weirdness with how you have 3 separate textures for the image data, you guys probably have your reasons for it (I'm guessing something to do with receiving separate planes from the decoder? I've never messed with actually displaying decoded video with GLSL, I'm just a colorspace nurd).

There might be other things I could comment on, but the only other thing that comes to mind is that perhaps instead of hard-coding each possible matrix into the code, instead have sets of YUV coefficients, and generate each matrix from those sets. Makes it easier to support multiple variations of YUV, and only 2 numbers are needed per variation (Kr and Kb; technically Kg is needed too, but it's always 1 - Kr - Kb. Weellll, teechnically you could calculate any of them given the other 2, but Wikipedia seems to only show Kr and Kb values for some of them, so it's just easier to consider those to be the givens).

You might really want to consider doing this, so that you can also support the case where the metadata in a file states to use the 'full' (or 'pc') range instead of the 'limited' (or 'tv') range. When such data is provided, the vector being subtracted at the end of the 'decoding' fragment would be changed to vec3(0, 128, 128)/255, the top row of values in the matrices all turn to 1.0, and the other values in the matrix are scaled differently. BT.601's matrix, for example, becomes:

const mat3 yuvToRgb = mat3(
	1.0, 1.0, 1.0,
	0.0, -25251.0/73375.0, 1.772,
	1.402, -209599.0/293500.0, 0.0
);

You might recognize a couple of those numbers as being similar to - but not exactly - the wrong values used previously. I don't know why they have 1.403 instead of 1.402, nor do I know why they'd truncate 1.772 to 1.77 (they're weirdly inconsistent with the number of decimal places they use), but basically the values they used were for 'pc'-scaled YUV values, not 'tv'-scaled values. If I don't use fractions, it becomes (approximately):

const mat3 yuvToRgb = mat3(
	1.0, 1.0, 1.0,
	0.0, -0.3441362862, 1.772,
	1.402, -0.7141362862, 0.0
);

Please sign in to comment.