New issue
Advanced search Search tips

Issue 750313 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug

Blocking:
issue 746182



Sign in to add a comment

Pass Rotational information to the SurfaceLayerBridge's Surface.

Project Member Reported by lethalantidote@chromium.org, Jul 28 2017

Issue description

By having the SurfaceLayerBridge create the Surface instead of the WebMediaPlayerImpl creating a VideoLayer, we lose the opportunity to pass in rotational information to the cc layer. We must figure out a way to do this, and have it updatable.

 
Blocking: 746182
Cc: mlamouri@chromium.org
Labels: -Pri-2 M-65 Pri-1
Status: Started (was: Assigned)
I have an idea on how to do this, sending the information from the webmediaplayerimpl to the submitter, but in order to test, I would need a site that doesnt have default orientation. Do you know of any such sites, Mounir? 
I think https://cs.chromium.org/chromium/src/media/test/data/bear_rotate_90.mp4?sq=package:chromium is a 90 degrees rotated video. There are others with different rotation angles.
The bear videos not seem to work on my local clean build without surfaces enabled. I'll see if there is a bug already filled about this. In the mean time, do you have any other suggested videos?
Project Member

Comment 6 by bugdroid1@chromium.org, Feb 7 2018

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

commit 7b8b5b6e24112a724f8ee5ee790a3aeb700442cf
Author: CJ DiMeglio <lethalantidote@chromium.org>
Date: Wed Feb 07 01:58:42 2018

Updates rotation information for video surfaces.

This CL allows video surfaces to utilize rotation information provided in the metadata.
Providing this information is important because it ensures that videos are played in the
correct orientation.

In order to accomplish this, we wait to enable submission of frames until after metadata
has loaded. This CL also gets rid of the use of the |submitter_| variable in
VideoFrameCompositor, only using it for ownership purposes, simplifying the code.

Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I06eb182be69d326f97b9ae9613cd196db55621a8
Bug:  750313 
Reviewed-on: https://chromium-review.googlesource.com/865462
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Frank Liberato <liberato@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: CJ DiMeglio <lethalantidote@chromium.org>
Cr-Commit-Position: refs/heads/master@{#534875}
[modify] https://crrev.com/7b8b5b6e24112a724f8ee5ee790a3aeb700442cf/media/blink/video_frame_compositor.cc
[modify] https://crrev.com/7b8b5b6e24112a724f8ee5ee790a3aeb700442cf/media/blink/video_frame_compositor.h
[modify] https://crrev.com/7b8b5b6e24112a724f8ee5ee790a3aeb700442cf/media/blink/video_frame_compositor_unittest.cc
[modify] https://crrev.com/7b8b5b6e24112a724f8ee5ee790a3aeb700442cf/media/blink/webmediaplayer_impl.cc
[modify] https://crrev.com/7b8b5b6e24112a724f8ee5ee790a3aeb700442cf/media/blink/webmediaplayer_impl_unittest.cc
[modify] https://crrev.com/7b8b5b6e24112a724f8ee5ee790a3aeb700442cf/third_party/WebKit/LayoutTests/TestExpectations
[add] https://crrev.com/7b8b5b6e24112a724f8ee5ee790a3aeb700442cf/third_party/WebKit/LayoutTests/media/content/bear_rotate_0.mp4
[add] https://crrev.com/7b8b5b6e24112a724f8ee5ee790a3aeb700442cf/third_party/WebKit/LayoutTests/media/content/bear_rotate_90.mp4
[add] https://crrev.com/7b8b5b6e24112a724f8ee5ee790a3aeb700442cf/third_party/WebKit/LayoutTests/media/video-rotation.html
[modify] https://crrev.com/7b8b5b6e24112a724f8ee5ee790a3aeb700442cf/third_party/WebKit/Source/platform/graphics/VideoFrameResourceProvider.cpp
[modify] https://crrev.com/7b8b5b6e24112a724f8ee5ee790a3aeb700442cf/third_party/WebKit/Source/platform/graphics/VideoFrameResourceProvider.h
[modify] https://crrev.com/7b8b5b6e24112a724f8ee5ee790a3aeb700442cf/third_party/WebKit/Source/platform/graphics/VideoFrameSubmitter.cpp
[modify] https://crrev.com/7b8b5b6e24112a724f8ee5ee790a3aeb700442cf/third_party/WebKit/Source/platform/graphics/VideoFrameSubmitter.h
[modify] https://crrev.com/7b8b5b6e24112a724f8ee5ee790a3aeb700442cf/third_party/WebKit/Source/platform/graphics/VideoFrameSubmitterTest.cpp
[modify] https://crrev.com/7b8b5b6e24112a724f8ee5ee790a3aeb700442cf/third_party/WebKit/public/platform/DEPS
[modify] https://crrev.com/7b8b5b6e24112a724f8ee5ee790a3aeb700442cf/third_party/WebKit/public/platform/WebVideoFrameSubmitter.h

Status: Fixed (was: Started)
Project Member

Comment 8 by bugdroid1@chromium.org, Mar 26 2018

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

commit a5dfe3d896f59f570a4f8ddb60941eeaea93f698
Author: CJ DiMeglio <lethalantidote@chromium.org>
Date: Mon Mar 26 19:07:59 2018

Update expectations for video-rotation.html to Failure Pass

Because propriety codecs are not supported on some test
configurations, video-rotation.html is updated to be
Failure Pass instead of just Skipped, as it gives us better
code coverage.

Bug:  750313 
Change-Id: Ia26c4cc0fdab21adb308b228a95c234577060af8
Reviewed-on: https://chromium-review.googlesource.com/958262
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Commit-Queue: CJ DiMeglio <lethalantidote@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545859}
[modify] https://crrev.com/a5dfe3d896f59f570a4f8ddb60941eeaea93f698/third_party/WebKit/LayoutTests/TestExpectations
[add] https://crrev.com/a5dfe3d896f59f570a4f8ddb60941eeaea93f698/third_party/WebKit/LayoutTests/media/video-rotation-expected.png
[add] https://crrev.com/a5dfe3d896f59f570a4f8ddb60941eeaea93f698/third_party/WebKit/LayoutTests/media/video-rotation-expected.txt
[modify] https://crrev.com/a5dfe3d896f59f570a4f8ddb60941eeaea93f698/third_party/WebKit/LayoutTests/media/video-rotation.html

Project Member

Comment 9 by bugdroid1@chromium.org, Mar 26 2018

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

commit f12a79beee138bfcf011f1046f4cacbc35d6e490
Author: Dominic Battré <battre@chromium.org>
Date: Mon Mar 26 20:51:12 2018

Revert "Update expectations for video-rotation.html to Failure Pass"

This reverts commit a5dfe3d896f59f570a4f8ddb60941eeaea93f698.

Reason for revert: Fails on Mac builders (instead of crashing).

Original change's description:
> Update expectations for video-rotation.html to Failure Pass
> 
> Because propriety codecs are not supported on some test
> configurations, video-rotation.html is updated to be
> Failure Pass instead of just Skipped, as it gives us better
> code coverage.
> 
> Bug:  750313 
> Change-Id: Ia26c4cc0fdab21adb308b228a95c234577060af8
> Reviewed-on: https://chromium-review.googlesource.com/958262
> Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
> Commit-Queue: CJ DiMeglio <lethalantidote@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#545859}

TBR=wez@chromium.org,ccameron@chromium.org,mlamouri@chromium.org,liberato@chromium.org,lethalantidote@chromium.org

Change-Id: I2dd1a6ee472a215c7139510c0ec08f0aef1bd4fc
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  750313 
Reviewed-on: https://chromium-review.googlesource.com/980755
Reviewed-by: Dominic Battré <battre@chromium.org>
Commit-Queue: Dominic Battré <battre@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545871}
[modify] https://crrev.com/f12a79beee138bfcf011f1046f4cacbc35d6e490/third_party/WebKit/LayoutTests/TestExpectations
[delete] https://crrev.com/37c3a715e4b8c6732b7197799c1bb498d01853cf/third_party/WebKit/LayoutTests/media/video-rotation-expected.png
[delete] https://crrev.com/37c3a715e4b8c6732b7197799c1bb498d01853cf/third_party/WebKit/LayoutTests/media/video-rotation-expected.txt
[modify] https://crrev.com/f12a79beee138bfcf011f1046f4cacbc35d6e490/third_party/WebKit/LayoutTests/media/video-rotation.html

Reverting the last change as it fails on Mac builders instead of crashing.

Text diff:
--- /b/s/w/io6uCBls/layout-test-results/media/video-rotation-expected.txt
+++ /b/s/w/io6uCBls/layout-test-results/media/video-rotation-actual.txt
@@ -3,8 +3,8 @@
 layer at (0,0) size 800x600
   LayoutBlockFlow {HTML} at (0,0) size 800x600
     LayoutBlockFlow {BODY} at (8,8) size 784x584
-      LayoutText {#text} at (320,225) size 4x19
-        text run at (320,225) width 4: " "
+      LayoutText {#text} at (320,226) size 4x18
+        text run at (320,226) width 4: " "
       LayoutText {#text} at (0,0) size 0x0
 layer at (8,8) size 320x240
   LayoutVideo {VIDEO} at (0,0) size 320x240

Image diff is also different (shows an empty image):
https://test-results.appspot.com/data/layout_results/WebKit_Mac10_11/30927/layout-test-results/results.html

Here is stderr:
12:41:43.785 39629 worker/2 media/video-rotation.html output stderr lines:
12:41:43.785 39629   [40847:38651:0326/124143.699450:ERROR:render_media_log.cc(30)] MediaEvent: MEDIA_ERROR_LOG_ENTRY {"error":"FFmpegDemuxer: no supported streams"}
12:41:43.785 39629   [40847:38651:0326/124143.699557:ERROR:render_media_log.cc(30)] MediaEvent: MEDIA_ERROR_LOG_ENTRY {"error":"FFmpegDemuxer: no supported streams"}
12:41:43.785 39629   [40847:779:0326/124143.699624:ERROR:render_media_log.cc(30)] MediaEvent: PIPELINE_ERROR DEMUXER_ERROR_NO_SUPPORTED_STREAMS
12:41:43.785 39629   [40847:779:0326/124143.699793:ERROR:render_media_log.cc(30)] MediaEvent: PIPELINE_ERROR DEMUXER_ERROR_NO_SUPPORTED_STREAMS
12:41:43.844 39629 "/b/s/w/ir/out/Release/image_diff --diff /b/s/w/it2Vwx_4/tmpmYX67u/actual.png /b/s/w/it2Vwx_4/tmpmYX67u/expected.png /b/s/w/it2Vwx_4/tmpmYX67u/diff.png" took 0.06s
@battre, the commit you reverted intentionally set it to Failure|Pass for that reason? I.e. that bot doesn't and can't have proprietary codecs enabled (but others do) and we have no means by which to distinguish these, so fail|pass is as good as we can do currently.
OIC, the CL description didn't indicate it but the TestExpectations for Mac added crashed when it should be in the same group as the others; I.e. the line should just be

bug media/video-rotation.html [ Fail Pass ]
dalecurtis@, it was set to CRASH on Mac because some bots hit a completely unrelated DCHECK for which a bug was opened. I think we should probably do [ Fail Crash ] or even [ Fail Pass Crash ] (to be safe).

Sign in to add a comment