Using Dialog video overlays causes Android window relayouts for every frame update. |
||||||||
Issue descriptionThe AVDASurfaceBundle sends redundant position updates to the AndroidOverlay which results in an unnecessary re-layout of the chrome window for every video frame update. This was at least a ~10% regression in power usage during playback for the buck1080p60_h264.mp4 video on this page: https://storage.googleapis.com/watk/v, in overlay mode. I'm filing a bug in case this needs a merge for 70 that came up here: https://chromium-review.googlesource.com/c/chromium/src/+/1205599. And I'm also curious why this wasn't caught by any perf benchmark?
,
Sep 5
This change landed: https://chromium-review.googlesource.com/c/chromium/src/+/1205599, I'm not sure why crbug isn't updating. liberato@, does this actually need a merge? You mentioned that the change which regressed this already went in M68. I'm not sure if this is sever enough to warrant a merge.
,
Sep 5
Should definitely merge a 10% power regression fix to M70 at the least. Probably worth going to M69 if there are extra stable updates too, given that it's the birthday release, but defer to TPMs on that one.
,
Sep 5
Let is soak overnight in next canary and then merge can be requested.
,
Sep 5
I'm going to request a 70 merge for this tomorrow, after this has been in canary. As for the merge to 69, I think there is a stable re-spin planned but I'm not sure if it'll be possible to merge this tomorrow to make it to the re-spin. +benmason on that.
,
Sep 5
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/00cf3e2aa55772615a5aab9e85f1a43b95241a83 commit 00cf3e2aa55772615a5aab9e85f1a43b95241a83 Author: Khushal <khushalsagar@chromium.org> Date: Wed Sep 05 19:07:41 2018 media/android: Fix unnecessary re-layouts for the video dialog overlay. AVDASurfaceBundle sends redundant position updates to the AndroidOverlay which results in an unnecessary re-layout of the chrome window for every frame. This change fixes it in the AVDASurfaceBundle to avoid redundant IPCs to the browser processes. And also in DialogOverlayCore to avoid any client-side errors from regressing this in the future. R=liberato@chromium.org BUG: 880691 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Change-Id: I17d9264c6d117ed5fc4d1012321cbd66c5cf60d4 Reviewed-on: https://chromium-review.googlesource.com/1205599 Commit-Queue: Khushal <khushalsagar@chromium.org> Reviewed-by: David Trainor <dtrainor@chromium.org> Reviewed-by: Frank Liberato <liberato@chromium.org> Cr-Commit-Position: refs/heads/master@{#588955} [modify] https://crrev.com/00cf3e2aa55772615a5aab9e85f1a43b95241a83/content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayCore.java [modify] https://crrev.com/00cf3e2aa55772615a5aab9e85f1a43b95241a83/content/public/android/junit/src/org/chromium/content/browser/androidoverlay/DialogOverlayCoreTest.java [modify] https://crrev.com/00cf3e2aa55772615a5aab9e85f1a43b95241a83/media/gpu/android/avda_surface_bundle.cc [modify] https://crrev.com/00cf3e2aa55772615a5aab9e85f1a43b95241a83/media/gpu/android/avda_surface_bundle.h
,
Sep 6
Tested this on latest canary and the change is looking good.
,
Sep 7
Your change meets the bar and is auto-approved for M70. Please go ahead and merge the CL to branch 3538 manually. Please contact 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 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5fe674886d784623b5bf5ddadfa49684d850fbdd commit 5fe674886d784623b5bf5ddadfa49684d850fbdd Author: Khushal <khushalsagar@chromium.org> Date: Fri Sep 07 19:12:16 2018 media/android: Fix unnecessary re-layouts for the video dialog overlay. AVDASurfaceBundle sends redundant position updates to the AndroidOverlay which results in an unnecessary re-layout of the chrome window for every frame. This change fixes it in the AVDASurfaceBundle to avoid redundant IPCs to the browser processes. And also in DialogOverlayCore to avoid any client-side errors from regressing this in the future. R=liberato@chromium.org BUG: 880691 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Change-Id: I17d9264c6d117ed5fc4d1012321cbd66c5cf60d4 Reviewed-on: https://chromium-review.googlesource.com/1205599 Commit-Queue: Khushal <khushalsagar@chromium.org> Reviewed-by: David Trainor <dtrainor@chromium.org> Reviewed-by: Frank Liberato <liberato@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#588955}(cherry picked from commit 00cf3e2aa55772615a5aab9e85f1a43b95241a83) Reviewed-on: https://chromium-review.googlesource.com/1214082 Reviewed-by: Khushal <khushalsagar@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#150} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/5fe674886d784623b5bf5ddadfa49684d850fbdd/content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayCore.java [modify] https://crrev.com/5fe674886d784623b5bf5ddadfa49684d850fbdd/content/public/android/junit/src/org/chromium/content/browser/androidoverlay/DialogOverlayCoreTest.java [modify] https://crrev.com/5fe674886d784623b5bf5ddadfa49684d850fbdd/media/gpu/android/avda_surface_bundle.cc [modify] https://crrev.com/5fe674886d784623b5bf5ddadfa49684d850fbdd/media/gpu/android/avda_surface_bundle.h
,
Sep 7
,
Sep 7
Did you chat with Ben about a potential M69 merge?
,
Sep 7
Approved for merge into 69, branch 3497.
,
Sep 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/826551db406486e809b1b5443658acddc0bf8cdd commit 826551db406486e809b1b5443658acddc0bf8cdd Author: Khushal <khushalsagar@chromium.org> Date: Fri Sep 07 20:41:40 2018 media/android: Fix unnecessary re-layouts for the video dialog overlay. AVDASurfaceBundle sends redundant position updates to the AndroidOverlay which results in an unnecessary re-layout of the chrome window for every frame. This change fixes it in the AVDASurfaceBundle to avoid redundant IPCs to the browser processes. And also in DialogOverlayCore to avoid any client-side errors from regressing this in the future. R=liberato@chromium.org BUG: 880691 Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel Change-Id: I17d9264c6d117ed5fc4d1012321cbd66c5cf60d4 Reviewed-on: https://chromium-review.googlesource.com/1205599 Commit-Queue: Khushal <khushalsagar@chromium.org> Reviewed-by: David Trainor <dtrainor@chromium.org> Reviewed-by: Frank Liberato <liberato@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#588955}(cherry picked from commit 00cf3e2aa55772615a5aab9e85f1a43b95241a83) Reviewed-on: https://chromium-review.googlesource.com/1214167 Reviewed-by: Khushal <khushalsagar@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#906} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/826551db406486e809b1b5443658acddc0bf8cdd/content/public/android/java/src/org/chromium/content/browser/androidoverlay/DialogOverlayCore.java [modify] https://crrev.com/826551db406486e809b1b5443658acddc0bf8cdd/content/public/android/junit/src/org/chromium/content/browser/androidoverlay/DialogOverlayCoreTest.java [modify] https://crrev.com/826551db406486e809b1b5443658acddc0bf8cdd/media/gpu/android/avda_surface_bundle.cc [modify] https://crrev.com/826551db406486e809b1b5443658acddc0bf8cdd/media/gpu/android/avda_surface_bundle.h
,
Sep 7
Awesome, thanks!
,
Sep 7
Just FYI, there isn't a respin planned for M69 on Android for now, but if there is one this change will be picked up. |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by khushals...@chromium.org
, Sep 5