Closed Bug 1265625 Opened 8 years ago Closed 8 years ago

First frame is upside down in some video.

Categories

(Core :: Graphics: Layers, defect)

ARM
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 --- wontfix
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: jhlin, Assigned: pchang)

References

Details

Attachments

(3 files)

Play this link -- http://people.mozilla.org/~jolin/mozilla%20employee%20recruiting%20video%20.mp4 in Fennec. The first frame is upside down.
Attached file Layerscope dump
In LayerScope the texture image of the upside down frame(#11)looks fine.
It might be better to check SurfaceTextureSource::GetTextureTransform().
Peter,
Per discussing offline, this is a very important bug. Can someone from your team help check it?
Flags: needinfo?(howareyou322)
Checking
Assignee: nobody → howareyou322
Flags: needinfo?(howareyou322)
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
After beginner video frame, the source transform becomes correct then I can see the video frame with right orientation.
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
Peter, 
Thank you for helping this bug!
Severity: normal → critical
Status: NEW → ASSIGNED
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 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?
Attachment #8750628 - Flags: review?(jmuizelaar)
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
Attachment #8750629 - Flags: review?(jmuizelaar) → review+
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
Don't know why reviewboard doesn't send out review again. Add needinfo for comment 14.
Flags: needinfo?(jmuizelaar)
Attachment #8750628 - Flags: review?(jmuizelaar)
Flags: needinfo?(jmuizelaar)
Attachment #8750628 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8750628 [details]
MozReview Request: Bug 1265625 - Remove workaround for fennec, r?jrmuizel

https://reviewboard.mozilla.org/r/51557/#review49541
https://hg.mozilla.org/mozilla-central/rev/09f4d5bad354
https://hg.mozilla.org/mozilla-central/rev/dd33f84a62e6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Peter, 
This impact user experience seriously. Could you help uplift your patch to firefox 48?
Thanks!
Flags: needinfo?(howareyou322)
we don't have much time for firefox47, so set firefox47 wontfix.
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
Flags: needinfo?(howareyou322)
Attachment #8750628 - Flags: approval-mozilla-aurora?
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
Attachment #8750629 - Flags: approval-mozilla-aurora?
ritu,
May we have your help to approve the patches in comment 21 and 22?
Thanks.
Flags: needinfo?(rkothari)
Comment on attachment 8750628 [details]
MozReview Request: Bug 1265625 - Remove workaround for fennec, r?jrmuizel

This seems like a bad regression, Aurora48+
Flags: needinfo?(rkothari)
Attachment #8750628 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8750629 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hello John, could you please verify this issue is fixed as expected on a latest Nightly? Thanks!
Flags: needinfo?(jolin)
I have tested it on my HTC M9+ with nightly and this issue is gone!
(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.
Flags: needinfo?(jolin)
You need to log in before you can comment on or make changes to this bug.