Closed Bug 894891 Opened 7 years ago Closed 7 years ago

Video playback broken on B2G


(Core :: Graphics: Layers, defect)

Gonk (Firefox OS)
Not set





(Reporter: derf, Unassigned)




(3 files, 1 obsolete file)

Symptoms: The content of the video tag just looks like a squashed version of the page containing it.

Tested on a peak using the H.264 playback from <> (WebM appears to work) as well as with getUserMedia() (using the patches in <>).

<> is broken.
<> works.

So this looks like a regression from bug 867460.
Could this be it?
Attachment #777095 - Flags: feedback?(matt.woodrow)
Blocks: 871364
(In reply to Milan Sreckovic [:milan] from comment #1)
> Could this be it?

That doesn't appear to fix the problem.
Given that we know which change caused it, I feel silly mentioning bug 892934, but I will anyway - does peak have 16-bit or 24-bit display?
Hopefully this fixes the incorrect rendering.

We used to set the shader program explicitly, now we compute it based on the RGB{A} format, and the texture target.

The gralloc texture host didn't override GetTextureTarget, so returned the default (GL_TEXTURE_2D).

This would result in us choosing the wrong shader program, one that reads from GL_TEXTURE_2D (which would still have the texture from the previous layer bound to it), rather than reading from GL_TEXTURE_EXTERNAL (which has the video texture bound).

derf: Can you please check if this fixes the issue.
Attachment #777293 - Flags: review?(ncameron)
Attachment #777293 - Flags: feedback?(tterribe)
Attachment #777293 - Flags: review?(ncameron) → review+
We have two different shader programs for GL_TEXTURE_EXTERNAL (with really confusing names).

The only difference is that one applies a texture transform to each pixel in the fragment shader, and is used for android plugins.

With the new code, we don't really know which of these two to use, and we're defaulting to the one that applies the texture transform.

It should work, but it will be a performance regression for b2g video.

One solution would be to add extra logic so that we only pick the one with the texture transform when we actually need.

The better option (imo) is to move the texture transform into the vertex shader instead, since that should be much cheaper.

I've done this by adding it to the default vertex shader. We don't really need it for most cases, but it should be really cheap.

I expect this to fix the b2g video performance regression and hopefully improve performance for android plugins.

Snorp: Can you please test this and make sure that it doesn't break android plugins that require a texture transform.
Attachment #777303 - Flags: feedback?(snorp)
As an added bonus, here's a patch that actually starts up.
Attachment #777303 - Attachment is obsolete: true
Attachment #777303 - Flags: feedback?(snorp)
Attachment #777311 - Flags: feedback?(snorp)
(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> Created attachment 777293 [details] [diff] [review]
> Hopefully fix b2g video

Confirmed that the patch works on master nexus4!
This is a really nice simplification. Thanks.
Attachment #777293 - Flags: feedback?(tterribe) → feedback+
Comment on attachment 777311 [details] [diff] [review]
Remove texture transform from the fragment shader v2

Review of attachment 777311 [details] [diff] [review]:

I tried this and things still seem to work, so looks good to me
Attachment #777311 - Flags: feedback?(snorp) → feedback+
Attachment #777311 - Flags: review?(jmuizelaar)
Comment on attachment 777311 [details] [diff] [review]
Remove texture transform from the fragment shader v2

Review of attachment 777311 [details] [diff] [review]:

::: gfx/layers/opengl/LayerManagerOGLShaders.txt
@@ +123,5 @@
>    finalPosition = finalPosition - uRenderTargetOffset;
> *= finalPosition.w;
>    finalPosition = uMatrixProj * finalPosition;
> +  vTexCoord = (uTextureTransform * vec4(aTexCoord.x, aTexCoord.y, 0.0, 1.0)).xy;

You should add a comment about what the uses of TextureTransform are.
Attachment #777311 - Flags: review?(jmuizelaar) → review+
You need to log in before you can comment on or make changes to this bug.