Add a YUV image shader. #528

nical · 2016-11-04T11:01:18Z

The YUV image shader is implemented in the image shader using #ifdefs for feature selection since most of the code is identical.

This change is

nical · 2016-11-04T11:04:28Z

At the moment this is very incomplete. The shader is created but other than that it is not integrated in webrender yet. Feedback on the approach are welcome!

kvark

Looks good, although I'd prefer to see the actual usage code as well.

kvark · 2016-11-04T14:44:04Z

webrender/res/ps_image.fs.glsl

+#ifdef WR_YUV_IMAGE
+uniform sampler2D sYTexture;
+// TODO: investigate storing the U and V planes in the same texture.
+uniform sampler2D sUTexture;


@glennw do you think we can afford allocating 2 more slots for the UV stuff? Doesn't look like they are going to be used for anything else, and we are limited to 16 slots in total.

I think that's fine - we can fix that when it becomes a problem. I can very easily reduce the number of samplers currently in use by combining some of the vertex data textures too, so I don't expect it to be a problem in the near future.

kvark · 2016-11-04T14:44:31Z

webrender/res/ps_image.fs.glsl

+    float y = texture(sYTexture, st).a;
+    float u = texture(sUTexture, st).a;
+    float v = texture(sVTexture, st).a;
+    oFragColor.rgb = uYuvColorMatrix * vec3(y - 0.06275, u - 0.50196, v - 0.50196);


nit: wondering why not having mat4 uYuvColorMatrix to avoid magic constants here

nical · 2016-11-04T15:36:22Z

This is a work in progress, I don't think it is landable in its current state. It will become more easily testable as Jerry and I fill the gaps (Jerry is working on exposing externally allocated textures to WR, which will add APIs to exercise that code).

I hesitated about reusing existing sampler names. If the 16 samplers limit is an issue (currently 10 of them so 13 with YUV), we should make that less confusing in the shader code (just name them uniform sampler sTexture0..N and use functions adequately named to sample from them such as vec3 sample_color(vec2 coords)).

The color conversion was shamefully copy-pasted from gecko's own YUV shader, I had the same reaction about folding the the magic constants in the matrix, but I postponed the investigation. I'd rather make sure I am going in the right direction before ironing out the details.

kvark · 2016-11-04T15:53:38Z

@nical ok, understood. I'm rather in favour of the current approach with clear texture names. I think you are safe to re-use the color texture for the Y channel, thus adding only 2 more textures.

nical · 2016-11-04T16:33:39Z

The last commit added a mostly unimplemented ExternalImage primitive batch data type. The bulk of the external image stuff will be implemented by @JerryShih, I just added the minimum to be able to see how to plug the yuv stuff into the renderer.

It would also make sense to add a way to use non-external YUV images, but the render backend seems to be written in a way that it expects at most two source textures for all primitives (color and mask). Making it work with more textures might require a bit of refactoring (not sure, still digging).

glennw · 2016-11-06T21:34:36Z

webrender/res/ps_image.fs.glsl

+#ifdef WR_YUV_IMAGE
+uniform sampler2D sYTexture;
+// TODO: investigate storing the U and V planes in the same texture.
+uniform sampler2D sUTexture;


I think that's fine - we can fix that when it becomes a problem. I can very easily reduce the number of samplers currently in use by combining some of the vertex data textures too, so I don't expect it to be a problem in the near future.

glennw · 2016-11-06T21:36:58Z

webrender/src/renderer.rs

@@ -141,6 +141,8 @@ impl VertexDataTexture {
 }

 const TRANSFORM_FEATURE: &'static [&'static str] = &["TRANSFORM"];
+const YUV_TRANSFORM_FEATURE: &'static [&'static str] = &["TRANSFORM", "YUV_IMAGE"];


I changed how features work in this PR - #527 - once that lands it should be possible to simplify this a bit (take a look at how the text subpx shader is created).

glennw · 2016-11-06T21:37:42Z

webrender/src/renderer.rs

@@ -513,6 +530,22 @@ impl Renderer {
                                                           &mut device,
                                                           options.precache_shaders);

+        let ps_yuv_image = PrimitiveShader {


Should be possible to create as a normal PrimitiveShader once the changes mentioned above land.

glennw · 2016-11-06T21:38:15Z

webrender/src/renderer.rs

+
+                            if has_complex_clip {
+                                // TODO(nical)
+                                unimplemented!();


Perhaps make a println!() warning rather than panic?

glennw · 2016-11-06T21:38:50Z

webrender/src/renderer.rs

+                            }
+
+                            // TODO(nical) needs its own GPU_TAG?
+                            self.gpu_profile.add_marker(GPU_TAG_PRIM_IMAGE);


Yup, let's add a new GPU tag - just so we can see what the shader costs of the YUV conversion are like.

glennw · 2016-11-06T21:46:06Z

@nical This looks like a great start! :) Is there a way to remove the PrimitiveBatchData::External type and treat it as a normal PrimitiveBatchData::Image? It doesn't seem conceptually right for the source of the image to require a different batch type.

The current Image batch type supports both normal images, and also webgl images - if we could extend this to handle external images, I think that would be ideal.

This might require a bit of thought on how to modify the image batch support to handle rgb vs. yuv textures - but seems like a better place to handle the different image source types. Ping me sometime during the week and we discuss?

nical · 2016-11-07T09:03:12Z

Pieces of code with TODO(nical) are not intended to land until the TODO has been resolved.

The main difference between the Image and ExternalImage primitives is that the former refers to texture ids directly and the texture ids are generated in the render backend thread, while the latter adds a level of indirection so that the texture ids are generated on the renderer and the backend thread only sees an external image key. It would certainly make more sense to have PrimitiveBatchData variants correspond to the "type of work to do to render the primitive" rather than how ids are rooted through the system. I was hesitant to make invasive changes, but I'll happily merge the two variants into something that supports both modes.

nical · 2016-11-09T15:23:46Z

Quick update: I rebased this PR on top of PR #534 (had to untangle a mess of commits mixed in random order between the three things I am working on, but hopefully I didn't loose anything in the process).
This makes github a bit unhappy (both this and PR #534 show in the diff now, but I assume it'll get better when the other one lands).

Previously the shader was using only one set of texture coordinates (it was assuming the Y, U and V planes were in their own textures), and moving away from this means the shader ends up being quite different from the image one, so I moved it into its own files. The shader is in a completely broken WIP state right now.

nical · 2016-11-23T13:38:20Z

Update: The PR is now in a landable state (or close, depending on reviews). YUV is handled by a special shader and supports the Rec601 and Rec709 color spaces (the two that gecko's GL compositor appear to support). The color space is specified per-primitive and the right conversion matrix is selected in the vertex shader because it seemed simpler to implement that way, but we could change that to specify the matrix using an uniform instead, and make sure different yuv images using the texture cache don't get batched together.
@kvark regarding your question about the constants and the matrix in the color conversion code: I think that they are separated in order to stay close to how these transformations are expressed in their specifications. My preference is to keep it that way since it makes it easy to check that we are using the right values.

I've tested it without using a reference image so I haven't verified that the colors are 100% correct, but it seems to work and it will be much easier to test once we have video working with webrender in Gecko. There is one bug, however, which is the one described in issue #586 and affects all types of images: If a texture is stretched to fit the size on screen, its texture coordinates end up stretched incorrectly by a small but very noticeable amount, causing the U and V channel (which are typically half the size of the Y channel), to not line up with the Y channel (unless they are all the same size which almost never happens in practice).
Since this bug also affects non-YUV images I think it makes sense to treat it as a separate issue.

nical · 2016-11-23T13:38:44Z

r? @kvark @glennw

glennw · 2016-11-23T21:33:05Z

Reviewed 12 of 12 files at r3.
Review status: all files reviewed at latest revision, 9 unresolved discussions.

webrender/res/prim_shared.glsl, line 613 at r3 (raw file):

    YuvImage image;

    ivec2 uv = get_fetch_uv_2(index);

This should be get_fetch_uv_4

webrender/src/prim_store.rs, line 173 at r3 (raw file):

impl YuvImagePrimitiveGpu {
    pub fn new(size: LayerSize, color_space: YuvColorSpace) -> Self {
        let int_color_space: u8 = unsafe { mem::transmute(color_space) };

Should be able to avoid unsafe here, by doing something like:

let color_space = color_space as u32 as f32;

webrender_traits/src/display_list.rs, line 107 at r3 (raw file):

    pub fn push_yuv_image(&mut self,
                      rect: Rect<f32>,

nit: indentation

Comments from Reviewable

glennw · 2016-11-23T21:34:22Z

@nical Looks great! Added a few minor issues, this is good to merge after they are fixed.

kvark

Looks good! Although I wonder how it worked with using get_fetch_uv_2 instead of get_fetch_uv_4.

nical · 2016-11-24T09:20:43Z

Oh boy! yeah, get_fetch_uv_2 "worked" because I only had one yuv image in my test so the offset was always zero.

YUV images are handled by a dedicated shader which reads the Y, U and V planes from separate texture samplers and computes the rgb values using the Rec601 or Rec709 color spaces.

glennw · 2016-11-24T20:45:39Z

Reviewed 4 of 4 files at r4.
Review status: all files reviewed at latest revision, 9 unresolved discussions.

Comments from Reviewable

glennw · 2016-11-24T20:46:42Z

Review status: all files reviewed at latest revision, 9 unresolved discussions.

Comments from Reviewable

glennw · 2016-11-24T20:47:15Z

@bors-servo r+

bors-servo · 2016-11-24T20:47:17Z

📌 Commit 6c4cf7d has been approved by glennw

bors-servo · 2016-11-24T20:47:19Z

⚡ Test exempted - status

Add a YUV image shader. The YUV image shader is implemented in the image shader using #ifdefs for feature selection since most of the code is identical.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/528)

kvark approved these changes Nov 4, 2016

View reviewed changes

glennw reviewed Nov 6, 2016

View reviewed changes

nical force-pushed the yuv branch 3 times, most recently from 20160c4 to 833bc24 Compare November 9, 2016 15:13

nical force-pushed the yuv branch 5 times, most recently from 37aa244 to fb3fa99 Compare November 23, 2016 13:10

kvark approved these changes Nov 23, 2016

View reviewed changes

Add support for YUV images.

6c4cf7d

YUV images are handled by a dedicated shader which reads the Y, U and V planes from separate texture samplers and computes the rgb values using the Rec601 or Rec709 color spaces.

nical force-pushed the yuv branch from fb3fa99 to 6c4cf7d Compare November 24, 2016 09:29

bors-servo merged commit 6c4cf7d into servo:master Nov 24, 2016

bors-servo mentioned this pull request Nov 24, 2016

Simplify display list construction in the API #592

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a YUV image shader. #528

Add a YUV image shader. #528

nical commented Nov 4, 2016 •

edited by larsbergstrom

Loading

nical commented Nov 4, 2016

kvark left a comment

kvark Nov 4, 2016

glennw Nov 6, 2016

kvark Nov 4, 2016

nical commented Nov 4, 2016

kvark commented Nov 4, 2016

nical commented Nov 4, 2016

glennw Nov 6, 2016

glennw Nov 6, 2016

glennw Nov 6, 2016

glennw Nov 6, 2016

glennw Nov 6, 2016

glennw commented Nov 6, 2016

nical commented Nov 7, 2016

nical commented Nov 9, 2016

nical commented Nov 23, 2016

nical commented Nov 23, 2016

glennw commented Nov 23, 2016

glennw commented Nov 23, 2016

kvark left a comment

nical commented Nov 24, 2016

glennw commented Nov 24, 2016

glennw commented Nov 24, 2016

glennw commented Nov 24, 2016

bors-servo commented Nov 24, 2016

bors-servo commented Nov 24, 2016

Add a YUV image shader. #528

Add a YUV image shader. #528

Conversation

nical commented Nov 4, 2016 • edited by larsbergstrom Loading

nical commented Nov 4, 2016

kvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nical commented Nov 4, 2016

kvark commented Nov 4, 2016

nical commented Nov 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennw commented Nov 6, 2016

nical commented Nov 7, 2016

nical commented Nov 9, 2016

nical commented Nov 23, 2016

nical commented Nov 23, 2016

glennw commented Nov 23, 2016

glennw commented Nov 23, 2016

kvark left a comment

Choose a reason for hiding this comment

nical commented Nov 24, 2016

glennw commented Nov 24, 2016

glennw commented Nov 24, 2016

glennw commented Nov 24, 2016

bors-servo commented Nov 24, 2016

bors-servo commented Nov 24, 2016

nical commented Nov 4, 2016 •

edited by larsbergstrom

Loading