1265625 - First frame is upside down in some video.

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[John Lin [:jhlin][:jolin]]

Play this link -- http://people.mozilla.org/~jolin/mozilla%20employee%20recruiting%20video%20.mp4 in Fennec. The first frame is upside down.

Comment 1

[John Lin [:jhlin][:jolin]]

Attachment: Layerscope dump

In LayerScope the texture image of the upside down frame(#11)looks fine.

Comment 2

[Sotaro Ikeda [:sotaro]]

It might be better to check SurfaceTextureSource::GetTextureTransform().

Comment 3

[Blake Wu [:bwu][:blakewu]]

Peter,
Per discussing offline, this is a very important bug. Can someone from your team help check it?

Comment 4

[Peter Chang[:pchang]]

Checking

Comment 5

[Peter Chang[:pchang]]

The up-side down video frame is caused by the source's transform was incorrect. And the transform was set from getTransformMatrix of SurfaceTextureAPI[1].

[1]https://dxr.mozilla.org/mozilla-central/source/gfx/gl/AndroidSurfaceTexture.cpp#244

Comment 6

[Peter Chang[:pchang]]

After beginner video frame, the source transform becomes correct then I can see the video frame with right orientation.

Comment 7

[Peter Chang[:pchang]]

For Android surfaceTexture, the correct transform will be calculated inside UpdateTexImage[1]. Therefore, we should get the transformMatrix after BindTexture calls(which calls updateTexImage internally).

[1]http://androidxref.com/4.4.4_r1/xref/frameworks/native/libs/gui/GLConsumer.cpp#160
[2]https://dxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/CompositorOGL.cpp?case=true&from=CompositorOGL.cpp#1266

Comment 8

[Blake Wu [:bwu][:blakewu]]

Peter, 
Thank you for helping this bug!

Comment 9

[Peter Chang[:pchang]]

Attachment: MozReview Request: Bug 1265625 - Remove workaround for fennec, r?jrmuizel

Review commit: https://reviewboard.mozilla.org/r/51557/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51557/

Comment 10

[Peter Chang[:pchang]]

Attachment: MozReview Request: Bug 1265625 - Call BindTexture to get correct transform for Android Surface Texture, r?jrmuizel

Review commit: https://reviewboard.mozilla.org/r/51559/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51559/

Comment 11

[Peter Chang[:pchang]]

https://reviewboard.mozilla.org/r/51559/#review48321

::: gfx/layers/opengl/CompositorOGL.cpp:1248
(Diff revision 1)
>  
>        source->AsSourceOGL()->BindTexture(LOCAL_GL_TEXTURE0, filter);
>  
>        program->SetTextureUnit(0);
> +
> +      Matrix4x4 textureTransform = source->AsSourceOGL()->GetTextureTransform();

For Fennec, the correct transform is calculated inside SurfaceTextureSource::BindTexture[1]. Therefore, I need to change the calling sequences here.


https://dxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/TextureHostOGL.cpp#417

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8750628 [details]
MozReview Request: Bug 1265625 - Remove workaround for fennec, r?jrmuizel

https://reviewboard.mozilla.org/r/51557/#review48471

::: gfx/layers/opengl/CompositorOGL.cpp
(Diff revision 1)
> -        // On Android we encounter small resampling errors in what should be
> -        // pixel-aligned compositing operations. This works around them. This
> -        // code should not be needed!
> -        filter = gfx::Filter::POINT;
> -      }
> -#endif

Why do we need this?

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8750629 [details]
MozReview Request: Bug 1265625 - Call BindTexture to get correct transform for Android Surface Texture, r?jrmuizel

https://reviewboard.mozilla.org/r/51559/#review48477

Comment 14

[Peter Chang[:pchang]]

https://reviewboard.mozilla.org/r/51557/#review48471

> Why do we need this?

I'm sorry that I forgot to explain this part. Before I tried to move this workaround inside SurfaceTextureSource::BindTexture[1] in order to changing the call sequence between bindtexture and settransform[2]. But I couldn't move aTransform from Compositor::DrawQuad into SurfaceTextureSource::BindTexture. This workaround came from bug 938316 which helped some reftest failures on fennec. So I just removed this workaround and apply my 2nd patch to check the try result and I didn't see the reftest failures. Therefore, I decided to remove this workaround.


[1]https://dxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/TextureHostOGL.cpp#402
[2]https://dxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/CompositorOGL.cpp#1255

Comment 15

[Peter Chang[:pchang]]

Don't know why reviewboard doesn't send out review again. Add needinfo for comment 14.

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8750628 [details]
MozReview Request: Bug 1265625 - Remove workaround for fennec, r?jrmuizel

https://reviewboard.mozilla.org/r/51557/#review49541

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/09f4d5bad354
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd33f84a62e6

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/09f4d5bad354
https://hg.mozilla.org/mozilla-central/rev/dd33f84a62e6

Comment 19

[Blake Wu [:bwu][:blakewu]]

Peter, 
This impact user experience seriously. Could you help uplift your patch to firefox 48?
Thanks!

Comment 20

[Blake Wu [:bwu][:blakewu]]

we don't have much time for firefox47, so set firefox47 wontfix.

Comment 21

[Peter Chang[:pchang]]

Comment on attachment 8750628 [details]
MozReview Request: Bug 1265625 - Remove workaround for fennec, r?jrmuizel

Approval Request Comment
[Feature/regressing bug #]:none
[User impact if declined]: Bad user experience because users might see the upside down video frame in the beginning
[Describe test coverage new/current, TreeHerder]:got positive try result 
[Risks and why]: only change the calling sequence, should be low risk
[String/UUID change made/needed]:none

Comment 22

[Peter Chang[:pchang]]

Comment on attachment 8750629 [details]
MozReview Request: Bug 1265625 - Call BindTexture to get correct transform for Android Surface Texture, r?jrmuizel

Approval Request Comment
[Feature/regressing bug #]:none
[User impact if declined]: Bad user experience because users might see the upside down video frame in the beginning
[Describe test coverage new/current, TreeHerder]:got positive try result 
[Risks and why]: only change the calling sequence, should be low risk
[String/UUID change made/needed]:none

Comment 23

[Blake Wu [:bwu][:blakewu]]

ritu,
May we have your help to approve the patches in comment 21 and 22?
Thanks.

Comment 24

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8750628 [details]
MozReview Request: Bug 1265625 - Remove workaround for fennec, r?jrmuizel

This seems like a bad regression, Aurora48+

Comment 25

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello John, could you please verify this issue is fixed as expected on a latest Nightly? Thanks!

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c79fc3fa7ea3
https://hg.mozilla.org/releases/mozilla-aurora/rev/138121c338c7

Comment 27

[Blake Wu [:bwu][:blakewu]]

I have tested it on my HTC M9+ with nightly and this issue is gone!

Comment 28

[John Lin [:jhlin][:jolin]]

(In reply to Ritu Kothari (:ritu) from comment #25)
> Hello John, could you please verify this issue is fixed as expected on a
> latest Nightly? Thanks!

 Yes it is. Also tried Chase's test page and there is no more flicker.