Closed
Bug 1265625
Opened 8 years ago
Closed 8 years ago
First frame is upside down in some video.
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: jhlin, Assigned: pchang)
References
Details
Attachments
(3 files)
4.70 MB,
application/zip
|
Details | |
58 bytes,
text/x-review-board-request
|
jrmuizel
:
review+
ritu
:
approval-mozilla-aurora+
|
Details |
58 bytes,
text/x-review-board-request
|
jrmuizel
:
review+
ritu
:
approval-mozilla-aurora+
|
Details |
Play this link -- http://people.mozilla.org/~jolin/mozilla%20employee%20recruiting%20video%20.mp4 in Fennec. The first frame is upside down.
Reporter | ||
Comment 1•8 years ago
|
||
In LayerScope the texture image of the upside down frame(#11)looks fine.
Comment 2•8 years ago
|
||
It might be better to check SurfaceTextureSource::GetTextureTransform().
Comment 3•8 years ago
|
||
Peter, Per discussing offline, this is a very important bug. Can someone from your team help check it?
Flags: needinfo?(howareyou322)
Assignee | ||
Comment 4•8 years ago
|
||
Checking
Assignee: nobody → howareyou322
Flags: needinfo?(howareyou322)
Assignee | ||
Comment 5•8 years ago
|
||
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
Assignee | ||
Comment 6•8 years ago
|
||
After beginner video frame, the source transform becomes correct then I can see the video frame with right orientation.
Assignee | ||
Comment 7•8 years ago
|
||
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
Updated•8 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 9•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/51557/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/51557/
Attachment #8750628 -
Flags: review?(jmuizelaar)
Attachment #8750629 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 10•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/51559/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/51559/
Assignee | ||
Comment 11•8 years ago
|
||
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•8 years ago
|
||
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 13•8 years ago
|
||
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+
Assignee | ||
Comment 14•8 years ago
|
||
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
Assignee | ||
Comment 15•8 years ago
|
||
Don't know why reviewboard doesn't send out review again. Add needinfo for comment 14.
Flags: needinfo?(jmuizelaar)
Assignee | ||
Updated•8 years ago
|
Attachment #8750628 -
Flags: review?(jmuizelaar)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(jmuizelaar)
Updated•8 years ago
|
Attachment #8750628 -
Flags: review?(jmuizelaar) → review+
Comment 16•8 years ago
|
||
Comment on attachment 8750628 [details] MozReview Request: Bug 1265625 - Remove workaround for fennec, r?jrmuizel https://reviewboard.mozilla.org/r/51557/#review49541
Comment 17•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/09f4d5bad354 https://hg.mozilla.org/integration/mozilla-inbound/rev/dd33f84a62e6
Comment 18•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/09f4d5bad354 https://hg.mozilla.org/mozilla-central/rev/dd33f84a62e6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Updated•8 years ago
|
status-firefox47:
--- → affected
status-firefox48:
--- → affected
Comment 19•8 years ago
|
||
Peter, This impact user experience seriously. Could you help uplift your patch to firefox 48? Thanks!
Flags: needinfo?(howareyou322)
Comment 20•8 years ago
|
||
we don't have much time for firefox47, so set firefox47 wontfix.
Assignee | ||
Comment 21•8 years ago
|
||
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?
Assignee | ||
Comment 22•8 years ago
|
||
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?
Comment 23•8 years ago
|
||
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)
Comment 26•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/c79fc3fa7ea3 https://hg.mozilla.org/releases/mozilla-aurora/rev/138121c338c7
Comment 27•8 years ago
|
||
I have tested it on my HTC M9+ with nightly and this issue is gone!
Reporter | ||
Comment 28•8 years ago
|
||
(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.
Description
•