New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 869704 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 22
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Portrait videos display body background-color during playback

Reported by m...@plexapp.com, Aug 1

Issue description

UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/68.0.3440.75 Safari/537.36

Steps to reproduce the problem:
1. Download the repro (18MB) from: https://drive.google.com/file/d/1wckozxrDbaFhyMUh5Wf1eJHG5VH81njK/view?usp=sharing
2. Or, visit the hosted copy: https://plexinc.github.io/sandbox/matt/bugs/chromium/portrait/
3. Load the page, no further action necessary.

What is the expected behavior?
The video element has a background-color: #000. The body element has a background-color: darkorchid. Only the body's background-color should be visible.

What went wrong?
The body's background-color displays anchored to the top left corner of the video element. Resize your window if it's not visible.

Did this work before? Yes Unknown

Does this work in other browsers? Yes

Chrome version: 68.0.3440.75  Channel: stable
OS Version: OS X 10.13.6
Flash Version: n/a

Labelling as a regression as we'd like to think we'd have caught this in our application in the past. The background-color issue was found when reviewing https://bugs.chromium.org/p/chromium/issues/detail?id=610895.
 
Screen Shot 2018-07-31 at 2.18.13 PM.png
305 KB View Download
For some context the video was recorded by my Nexus 5.
Labels: Needs-Triage-M68 Needs-Bisect
Cc: abdulsyed@chromium.org manoranj...@chromium.org viswa.karala@chromium.org
Labels: -Pri-2 -Needs-Bisect hasbisect-per-revision ReleaseBlock-Stable M-68 RegressedIn-67 Target-69 Target-68 Triaged-ET FoundIn-68 FoundIn-69 OS-Linux Pri-1
Owner: mlamouri@chromium.org
Status: Assigned (was: Unconfirmed)
Able to reproduce the issue on reported version 68.0.3440.75, latest stable# 68.0.3440.84 and the same is not seen on latest canary 70.0.3508.0 using Ubuntu 14.04 & Mac 10.12.6, hence providing reverse bisect info
Note: Issue is not seen on Windows

Reverse Bisect Info:
================
Last Bad build: 69.0.3482.0
First Good build: 69.0.3483.0

You are probably looking for a change made after 572792 (known good), but no later than 572793 (first known bad).
https://chromium.googlesource.com/chromium/src/+log/bb1e9dbfa13bfb83892254b2b4ae75279ca58497..9c99a160694b9381543eec592776bf62ab4f77e1
Suspect: https://chromium.googlesource.com/chromium/src/+/9c99a160694b9381543eec592776bf62ab4f77e1
Reviewed-on: https://chromium-review.googlesource.com/1110670

@Mounir Lamouri: Please confirm the issue and help in re-assigning if it is not related to your change, please help in merging it to M-68 if applicable
Adding ReleaseBlock-Stable as it seems recent break, feel free to remove it if not applicable.

Thanks!
Cc: mlamouri@chromium.org
Labels: Needs-Feedback
Owner: viswa.karala@chromium.org
I'm running 69.0.3497.12 and can't reproduce the issue. Is this reproducible on latest Dev or Canary?
Per comment #3, First Good build: 69.0.3483.0 which means this issue is not seen on M69 Beta version 69.0.3497.23, correct? If it is fixed in M69, pls remove "Target-69" label.
Components: -Blink>Media Internals>Media>Video
Owner: dalecur...@chromium.org
If I understand correctly, it's a M68 only bug. It sounds unlikely that we would merge a fix in the stable branch but given that it doesn't seem to be related to video surface layer, I'm moving this to dalecurtis@ and the videostack team to decide.
Labels: -hasbisect-per-revision Needs-Bisect
Owner: viswa.karala@chromium.org
Seems like it was fixed by turning on SurfaceLayers, which we wouldn't merge. But we should figure out when it broke.

@viswa.karala can you bisect the other way to figure out when this broke?
Labels: -Needs-Bisect -Target-69
Owner: dalecur...@chromium.org
Able to reproduce the issue on reported version 68.0.3440.75, latest stable# 68.0.3440.84 and the same is not seen on Beta# 69.0.3497.23 and on latest chrome# 70.0.3510.0 using Mac 10.12.6 and Ubuntu 14.04

#5: As the issue is not seen on Beta# 69.0.3497.23, hence removing "Target-69" label
#7: On enabling #enable-surfaces-for-videos flag from chrome://flags issue is not seen on any of the build

Without enabling #enable-surfaces-for-videos flag from chrome://flags, issue is seen as mentioned below:
Bisect Info:
=====================
Good Build: 67.0.3382.0
Bad Build: 67.0.3383.0

Reverse Bisect Info:
=====================
Last Bad build: 69.0.3482.0
First Good build: 69.0.3483.0

Note: In the above mentioned bisect ranges for the bad builds, on enabling #enable-surfaces-for-videos flag, issue is not seen. Hence removing Needs-Bisect label.

@dalecurtis: Please find the above mentioned information and let us know if any other information is required from our end.

Thanks!
Cc: ikilpatrick@chromium.org kochi@chromium.org dalecur...@chromium.org
Components: Blink>Layout
Owner: e...@chromium.org
Thanks viswa, here's the change log from when it broke: https://chromium.googlesource.com/chromium/src/+log/67.0.3382.0..67.0.3383.0?pretty=fuller&n=10000

Possible culprits:
cc32ee1f3375cbf3828f45992c6753f4166f4a19 from hayato@
7dde4da2c3a7bb713d3f63c11a35661198133ff3 from kochi@
e355a2a9cde0b850090f700e18649070dd599785 from ikilpatrick@

+Blink>Layout and those folks. There is lots of LayoutNG work going on in this time too. eae@ since you own the LayoutNG bug, can you look over the change log and see if anything stands out. 

I tried to create a version of this test which could work with vp9+opus so we can bisect based on commit, but unfortunately the bug seems only to occur when the h264 version of clip is used. For which we don't have per-commit bisect available (no proprietary codecs in those builds).
Friendly ping to get an update on this issue as we are planning stable respin soon & it is marked as RBS for M68.

Thank You!
 Issue 870242  has been merged into this issue.
Cc: susan.boorgula@chromium.org
 Issue 871707  has been merged into this issue.
@eae : Gentle Ping! Could you please provide any update on this issue as it has been marked as a stable blocker.

Thank You!
Components: -Blink>Layout
Owner: dalecur...@chromium.org
dalecurtis: None of the linked changes touch the LayoutNG code and as LayoutNG isn't on by default it seems quite unlikely to have caused this bug.

There are no layout or paint changes (at all) in the regression range.

Cc: danakj@chromium.org fsam...@chromium.org
Owner: yiyix@chromium.org
Ah, after looking again I was able to create a webm+vp9 version of this test case and use it with our finer-grained bisect script. The key part seems to be that the video has a 90 degree rotation on it. http://xorax.sea/portrait/ is my (internal-only) modified test case.

The culprit is e05ba4c58bde7423528fbb71f61b74543bae919c by yiyix@

$ ./tools/bisect-builds.py -a linux64 -g 546347 -b 546671 -- --disable-features=UseSurfaceLayerForVideo http://127.0.0.1/portrait/index.html
You are probably looking for a change made after 546389 (known good), but no later than 546391 (first known bad).
CHANGELOG URL:
  https://chromium.googlesource.com/chromium/src/+log/340c463f86e06b293061e33e14acb17b77ffbc86..e05ba4c58bde7423528fbb71f61b74543bae919c
Friendly ping to get an update as this bug is still marked as RBS for M68.
Thanks..!

Labels: -ReleaseBlock-Stable -Target-68 -Needs-Triage-M68
This is already in production, so it cannot be a stable release blocker. Removing the label and i am working on it now. 
Labels: ReleaseBlock-Stable M-69
Should probably still be RB-S M69 then.
Status: Started (was: Assigned)
Labels: -M-68
It's fixed on ToT for me. I have attached the videos. Could someone verify it is fixed in canary for them as well? 


chrome m68.mkv
1.9 MB Download
ToT.mkv
2.0 MB Download
Please make sure you're testing with 'Use SurfaceLayer for video' disabled in chrome://flags, it hides the issue.
M69 Stable promotion is coming VERY soon. Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and request a merge into the release branch ASAP. Merge has to happen latest by 4:00 PM PT Friday (08/24/18) in order to make it to next week stable cut. Thank you.
Per bisect provided at #8, this should be fixed in M69. Can someone pls test on latest M69 Beta version 69.0.3497.42?

Bisect Info:
=====================
Good Build: 67.0.3382.0
Bad Build: 67.0.3383.0

Reverse Bisect Info:
=====================
Last Bad build: 69.0.3482.0
First Good build: 69.0.3483.0
@govind: this issue is gone if the flag enable-viz-display-compositor is set to enable; and the flag is set to enable in this cl: https://chromium-review.googlesource.com/c/chromium/src/+/1110670

I have tested this issue with ToT and M69, it works. 

So I don't think this issue needs to mark as a stable release block for M69
yiyix@, please see c#22. That flag is NOT enabled in M69 and is currently disabled due to other regressions. A fix is necessary.
(if you're testing with M69 beta, there is still a finch experiment, so you must disable it in chrome://flags if you end up in the experiment group)
I tried to deep dived to why this issue appeared at the first place. 

The video recording in Android has violet the assumption of SharedQuadState::quad_layer_rect.

The assumption is:
"
  // The rect of the quads' originating layer in the space of the quad rects.
  // Note that the |quad_layer_rect| represents the union of the |rect| of
  // DrawQuads in this SharedQuadState. If it does not hold, then
  // |are_contents_opaque| needs to be set to false.
"

Taking the video recorded in this bug as an example, SharedQuadState::quad_layer_rect has a size of 338*600 and the video draw quad has a size of 600*338. They both has a 90 degree transform. 

Draw occlusion is basically used the size from the SharedQuadState to calculate the size of the video, so it thinks there is 600*338 (after transform) video being display on screen, so it didn't render the anything behind his video draw quads. However, the actual quad is used for drawing, so the video is shown correctly on screen. 





I have review all the usage of SharedQuadState::quad_layer_rect for the YuvVideoDrawQuad. It seems that it is not used any different than any other type of DrawQuad, I don't think we need to violate our assumptions of SharedQuadState in the creation of YuvVideoDrawQuad -- its SharedQuadState::quad_layer_rect is not the union of visible rects of its quads. 
Project Member

Comment 30 by bugdroid1@chromium.org, Aug 22

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/13de6cf8cd4f85994a51d551b8df3b3813b77631

commit 13de6cf8cd4f85994a51d551b8df3b3813b77631
Author: yiyix <yiyix@chromium.org>
Date: Wed Aug 22 19:55:41 2018

fix bug "Portrait videos display body background-color during playback"

The video recording in Android has violet the assumption of
SharedQuadState which states that the union of DrawQuads in a given
SharedQuadState is defined in |ShareQuadState::quad_layer_rect|.
This bug reveals that the VideoDrawQuad is not contained in its
SharedQuadState.

Bug:  869704 

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I158275fbb321de84f7808e82b770d2f0b3cec8bc
Reviewed-on: https://chromium-review.googlesource.com/1184503
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Commit-Queue: Yi Xu <yiyix@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585214}
[modify] https://crrev.com/13de6cf8cd4f85994a51d551b8df3b3813b77631/cc/layers/video_layer_impl.cc

Labels: Merge-Request-69
Project Member

Comment 32 by sheriffbot@chromium.org, Aug 22

Labels: -Merge-Request-69 Merge-Review-69 Hotlist-Merge-Review
This bug requires manual review: We are only 12 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
This bug is regressed in M68. And per reverse bisect provided at #8, this is fixed in M69.

Reverse Bisect Info:
=====================
Last Bad build: 69.0.3482.0
First Good build: 69.0.3483.0

Could you pls justify this merge to M69? 
govind: Please see c#22, this is _not_ fixed in M69 -- it only appears that way because beta still has  the surface layer experiment enabled which will not be enabled for m69.

Mounir, it's probably time to disable the surface layer 50/50 on beta so that we don't accidentally ship any other bugs.
Make sure to force the following feature as Disabled in order to test this regression: chrome://flags/#enable-surfaces-for-videos 

If you are in the Finch group that has the feature enabled, as dalecurtis@ mentioned, you will not see the regression.
Labels: -M-69 -Merge-Review-69 Merge-Rejected-69 Target-70 M-70
Rejecting merge to M69 per offline chat with dalecuris@, this can wait until M70.
For posterity: YT pre-rotates their videos and it seems Vimeo does the same thing, so this should be limited to portrait only videos with a rotation.
Status: Fixed (was: Started)
Labels: TE-Verified-M70 TE-Verified-70.0.3532.0
Able to reproduce the issue on chrome version# 70.0.3530.0(build without fix)
Verified the fix on Ubuntu 14.04 and Mac 10.12.6 using Chrome version #70.0.3532.0 as per the comment #35
Attaching screencast for reference.
Observed "only body's background-colour during video playback"
Hence, the fix is working as expected. 
Adding the verified labels.

Thanks...!!
869704.ogv
2.7 MB View Download
Project Member

Comment 40 by bugdroid1@chromium.org, Aug 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ddc91145857722275e2bd8c4ab7e2c999c96d806

commit ddc91145857722275e2bd8c4ab7e2c999c96d806
Author: yiyix <yiyix@chromium.org>
Date: Mon Aug 27 22:40:13 2018

Add test for "Portrait videos display body background-color during playback"

The fix for bug Portrait videos display body background-color during playback
was landed in https://chromium-review.googlesource.com/c/chromium/src/+/1184503.
Due to its time sensitivity, I didn't get time to write a unittest for it.
Adding unittest here.

Bug:  869704 

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ic4f8bf457e9236f0c048ca018031d49760aad0f1
Reviewed-on: https://chromium-review.googlesource.com/1191528
Reviewed-by: weiliangc <weiliangc@chromium.org>
Commit-Queue: Yi Xu <yiyix@chromium.org>
Cr-Commit-Position: refs/heads/master@{#586458}
[modify] https://crrev.com/ddc91145857722275e2bd8c4ab7e2c999c96d806/cc/layers/video_layer_impl_unittest.cc

Sign in to add a comment