VideoSurfaceLayer: enable feature only when video goes in Picture-in-Picture |
||||||||
Issue descriptionBecause VideoSurfaceLayer still has performance regressions, we need to break the dependency between this and Picture-in-Picture. The feature isn't ready to be used for all videos but is good enough to be used for Picture-in-Picture. The plan is to have the compositing method switch when Picture-in-Picture is triggered on a video. This is a temporary change until we can fix the perf regressions and launch the feature for all videos.
,
Sep 17
,
Sep 24
Friendly ping! Could you please provide any update on this issue as it has been marked as a stable blocker. Thank You!
,
Sep 26
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/69db58ff1cadcd6dd30a572850466324c23a3d60 commit 69db58ff1cadcd6dd30a572850466324c23a3d60 Author: liberato@chromium.org <liberato@chromium.org> Date: Wed Sep 26 20:27:56 2018 Enable SurfaceLayerForVideo for PIP only. This CL adds a new flag, UseSurfaceLayerForVideoPIP, which will switch to SurfaceLayer only when entering PIP mode. The player does not switch back when exiting PIP. When doing this, the VideoFrameCompositor, VFS, etc. all run on the compositor impl thread rather than a dedicated media thread. This is to simplify the switchover. This CL also changes the default from Use...Video to Use...PIP. This is based on https://chromium-review.googlesource.com/c/chromium/src/+/1219948 Bug: 883931 Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel Change-Id: Ibb612d83df721712929aeab42aba790e32c1a8ea Reviewed-on: https://chromium-review.googlesource.com/1220562 Reviewed-by: Fady Samuel <fsamuel@chromium.org> Reviewed-by: François Doray <fdoray@chromium.org> Reviewed-by: Bo <boliu@chromium.org> Reviewed-by: Kenneth Russell <kbr@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Mounir Lamouri <mlamouri@chromium.org> Reviewed-by: Dale Curtis <dalecurtis@chromium.org> Commit-Queue: Frank Liberato <liberato@chromium.org> Cr-Commit-Position: refs/heads/master@{#594459} [modify] https://crrev.com/69db58ff1cadcd6dd30a572850466324c23a3d60/android_webview/lib/aw_main_delegate.cc [modify] https://crrev.com/69db58ff1cadcd6dd30a572850466324c23a3d60/content/renderer/media/media_factory.cc [modify] https://crrev.com/69db58ff1cadcd6dd30a572850466324c23a3d60/content/renderer/media/media_factory.h [modify] https://crrev.com/69db58ff1cadcd6dd30a572850466324c23a3d60/content/renderer/render_view_impl.cc [modify] https://crrev.com/69db58ff1cadcd6dd30a572850466324c23a3d60/media/base/media_switches.cc [modify] https://crrev.com/69db58ff1cadcd6dd30a572850466324c23a3d60/media/base/media_switches.h [modify] https://crrev.com/69db58ff1cadcd6dd30a572850466324c23a3d60/media/blink/video_frame_compositor.cc [modify] https://crrev.com/69db58ff1cadcd6dd30a572850466324c23a3d60/media/blink/webmediaplayer_impl.cc [modify] https://crrev.com/69db58ff1cadcd6dd30a572850466324c23a3d60/media/blink/webmediaplayer_impl.h [modify] https://crrev.com/69db58ff1cadcd6dd30a572850466324c23a3d60/media/blink/webmediaplayer_impl_unittest.cc [modify] https://crrev.com/69db58ff1cadcd6dd30a572850466324c23a3d60/media/blink/webmediaplayer_params.cc [modify] https://crrev.com/69db58ff1cadcd6dd30a572850466324c23a3d60/media/blink/webmediaplayer_params.h [modify] https://crrev.com/69db58ff1cadcd6dd30a572850466324c23a3d60/third_party/blink/public/platform/web_video_frame_submitter.h [modify] https://crrev.com/69db58ff1cadcd6dd30a572850466324c23a3d60/third_party/blink/renderer/platform/exported/web_video_frame_submitter.cc [modify] https://crrev.com/69db58ff1cadcd6dd30a572850466324c23a3d60/third_party/blink/renderer/platform/graphics/video_frame_resource_provider.cc [modify] https://crrev.com/69db58ff1cadcd6dd30a572850466324c23a3d60/third_party/blink/renderer/platform/graphics/video_frame_resource_provider.h [modify] https://crrev.com/69db58ff1cadcd6dd30a572850466324c23a3d60/third_party/blink/renderer/platform/graphics/video_frame_submitter_test.cc
,
Sep 26
,
Sep 26
This bug requires manual review: M70 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 27
how safe is this merge? Seems like a fairly large code change.
,
Sep 28
let's see how it does in dev, then.
,
Oct 1
[bulk edit] - This issue is marked as a stable blocker for M70. We are two weeks away from M70 Stable. Please take a look urgently!
,
Oct 1
I realized we're pushing dev release to Thursday. How safe is this merge overall? Is it well tested locally?
,
Oct 2
it's well tested locally. i think it's fairly safe. most of it is "do exactly what we're doing now", and the changes should happen only when the user enters picture-in-picture (which is new, so probably not too many folks will at first anyway).
,
Oct 2
Ok great - approving this merge to M70. Branch:3538
,
Oct 2
thanks. there's a merge conflict, so i'll merge manually.
,
Oct 2
conflicts were trivial. tested locally and merged.
,
Oct 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/368cbff1c8cf118af97ea73f7396f8edc2ff66ae commit 368cbff1c8cf118af97ea73f7396f8edc2ff66ae Author: liberato@chromium.org <liberato@chromium.org> Date: Tue Oct 02 18:16:25 2018 [M70] Enable SurfaceLayerForVideo for PIP only. Merged from: https://chromium-review.googlesource.com/1220562 This CL adds a new flag, UseSurfaceLayerForVideoPIP, which will switch to SurfaceLayer only when entering PIP mode. The player does not switch back when exiting PIP. When doing this, the VideoFrameCompositor, VFS, etc. all run on the compositor impl thread rather than a dedicated media thread. This is to simplify the switchover. This CL also changes the default from Use...Video to Use...PIP. This is based on https://chromium-review.googlesource.com/c/chromium/src/+/1219948 Bug: 883931 Nopresubmit: true Notry: true Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel Change-Id: I43fbab358d0ff052bca386d07ed8aaf2f53bf187 Reviewed-on: https://chromium-review.googlesource.com/c/1257147 Reviewed-by: Frank Liberato <liberato@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#822} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/368cbff1c8cf118af97ea73f7396f8edc2ff66ae/android_webview/lib/aw_main_delegate.cc [modify] https://crrev.com/368cbff1c8cf118af97ea73f7396f8edc2ff66ae/content/renderer/media/media_factory.cc [modify] https://crrev.com/368cbff1c8cf118af97ea73f7396f8edc2ff66ae/content/renderer/media/media_factory.h [modify] https://crrev.com/368cbff1c8cf118af97ea73f7396f8edc2ff66ae/content/renderer/render_view_impl.cc [modify] https://crrev.com/368cbff1c8cf118af97ea73f7396f8edc2ff66ae/media/base/media_switches.cc [modify] https://crrev.com/368cbff1c8cf118af97ea73f7396f8edc2ff66ae/media/base/media_switches.h [modify] https://crrev.com/368cbff1c8cf118af97ea73f7396f8edc2ff66ae/media/blink/video_frame_compositor.cc [modify] https://crrev.com/368cbff1c8cf118af97ea73f7396f8edc2ff66ae/media/blink/webmediaplayer_impl.cc [modify] https://crrev.com/368cbff1c8cf118af97ea73f7396f8edc2ff66ae/media/blink/webmediaplayer_impl.h [modify] https://crrev.com/368cbff1c8cf118af97ea73f7396f8edc2ff66ae/media/blink/webmediaplayer_impl_unittest.cc [modify] https://crrev.com/368cbff1c8cf118af97ea73f7396f8edc2ff66ae/media/blink/webmediaplayer_params.cc [modify] https://crrev.com/368cbff1c8cf118af97ea73f7396f8edc2ff66ae/media/blink/webmediaplayer_params.h [modify] https://crrev.com/368cbff1c8cf118af97ea73f7396f8edc2ff66ae/third_party/blink/public/platform/web_video_frame_submitter.h [modify] https://crrev.com/368cbff1c8cf118af97ea73f7396f8edc2ff66ae/third_party/blink/renderer/platform/exported/web_video_frame_submitter.cc [modify] https://crrev.com/368cbff1c8cf118af97ea73f7396f8edc2ff66ae/third_party/blink/renderer/platform/graphics/video_frame_resource_provider.cc [modify] https://crrev.com/368cbff1c8cf118af97ea73f7396f8edc2ff66ae/third_party/blink/renderer/platform/graphics/video_frame_resource_provider.h [modify] https://crrev.com/368cbff1c8cf118af97ea73f7396f8edc2ff66ae/third_party/blink/renderer/platform/graphics/video_frame_submitter_test.cc
,
Oct 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/368cbff1c8cf118af97ea73f7396f8edc2ff66ae Commit: 368cbff1c8cf118af97ea73f7396f8edc2ff66ae Author: liberato@chromium.org Commiter: liberato@chromium.org Date: 2018-10-02 18:16:25 +0000 UTC [M70] Enable SurfaceLayerForVideo for PIP only. Merged from: https://chromium-review.googlesource.com/1220562 This CL adds a new flag, UseSurfaceLayerForVideoPIP, which will switch to SurfaceLayer only when entering PIP mode. The player does not switch back when exiting PIP. When doing this, the VideoFrameCompositor, VFS, etc. all run on the compositor impl thread rather than a dedicated media thread. This is to simplify the switchover. This CL also changes the default from Use...Video to Use...PIP. This is based on https://chromium-review.googlesource.com/c/chromium/src/+/1219948 Bug: 883931 Nopresubmit: true Notry: true Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel Change-Id: I43fbab358d0ff052bca386d07ed8aaf2f53bf187 Reviewed-on: https://chromium-review.googlesource.com/c/1257147 Reviewed-by: Frank Liberato <liberato@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#822} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
,
Oct 5
Checking in on this as it's marked RBS for M70. Is there any more work to be done with this issue? Thanks.
,
Oct 8
sorry, was ooo. |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by enne@chromium.org
, Sep 13