Issue metadata
Sign in to add a comment
|
Portrait videos display body background-color during playback
Reported by
m...@plexapp.com,
Aug 1
|
||||||||||||||||||||||
Issue descriptionUserAgent: 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.
,
Aug 1
,
Aug 2
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!
,
Aug 2
I'm running 69.0.3497.12 and can't reproduce the issue. Is this reproducible on latest Dev or Canary?
,
Aug 2
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.
,
Aug 3
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.
,
Aug 3
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?
,
Aug 3
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!
,
Aug 3
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).
,
Aug 7
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!
,
Aug 8
Issue 870242 has been merged into this issue.
,
Aug 8
,
Aug 13
@eae : Gentle Ping! Could you please provide any update on this issue as it has been marked as a stable blocker. Thank You!
,
Aug 13
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.
,
Aug 13
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
,
Aug 20
Friendly ping to get an update as this bug is still marked as RBS for M68. Thanks..!
,
Aug 20
This is already in production, so it cannot be a stable release blocker. Removing the label and i am working on it now.
,
Aug 20
Should probably still be RB-S M69 then.
,
Aug 20
,
Aug 20
,
Aug 20
It's fixed on ToT for me. I have attached the videos. Could someone verify it is fixed in canary for them as well?
,
Aug 20
Please make sure you're testing with 'Use SurfaceLayer for video' disabled in chrome://flags, it hides the issue.
,
Aug 21
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.
,
Aug 21
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
,
Aug 22
@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
,
Aug 22
yiyix@, please see c#22. That flag is NOT enabled in M69 and is currently disabled due to other regressions. A fix is necessary.
,
Aug 22
(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)
,
Aug 22
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.
,
Aug 22
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.
,
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
,
Aug 22
,
Aug 22
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
,
Aug 22
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?
,
Aug 22
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.
,
Aug 22
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.
,
Aug 22
Rejecting merge to M69 per offline chat with dalecuris@, this can wait until M70.
,
Aug 22
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.
,
Aug 22
,
Aug 24
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...!!
,
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 |
|||||||||||||||||||||||
Comment 1 by m...@plexapp.com
, Aug 1