New issue
Advanced search Search tips

Issue 833564 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug

Blocking:
issue 827396



Sign in to add a comment

Don't report video memory usage if no video frame has been composited.

Project Member Reported by dalecur...@chromium.org, Apr 16 2018

Issue description

The WebMediaPlayerImpl provides an estimate for the memory it thinks is held by the compositor based on the natural size of the video frame when no video memory has been reported. This is normally fine because we always end up decoding the first frame.

However with the advent of the preload=metadata suspend framework, we no longer always have a video frame and should only provide this estimate when we've actually given a frame to the compositor. The fix for this is very simple, just don't include the estimate until a frame has been composited.

This will affect the metrics we collect on beta for the experiment and thus should be merged back to M67.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Apr 16 2018

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

commit c2c5dcb1451ce1b2eaafc20419051776a0fdc881
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Mon Apr 16 23:21:29 2018

Don't report video memory usage estimate when no frames are present.

preload=metadata suspend prevents decoding and rendering of the
first video frame. In these cases the video memory usage is zero,
but WMPI always includes an estimate for the frame held by the
compositor even if no frame is being held.

This artificially inflates the memory used in the preload=metadata
experiment. The fix is to not include this estimate if we've never
painted a frame before.

BUG= 833564 
TEST=new unittest.

Change-Id: I0b8811f04b27e5cc5c4ea5789cbd3d9247720771
Reviewed-on: https://chromium-review.googlesource.com/1014387
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551167}
[modify] https://crrev.com/c2c5dcb1451ce1b2eaafc20419051776a0fdc881/media/blink/webmediaplayer_impl.cc
[modify] https://crrev.com/c2c5dcb1451ce1b2eaafc20419051776a0fdc881/media/blink/webmediaplayer_impl.h
[modify] https://crrev.com/c2c5dcb1451ce1b2eaafc20419051776a0fdc881/media/blink/webmediaplayer_impl_unittest.cc

Labels: Merge-Request-67
+MR-67, will merge after canary cycle, but is a super-minor change so no problems expected.
Project Member

Comment 3 by bugdroid1@chromium.org, Apr 17 2018

Labels: merge-merged-testbranch
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c2c5dcb1451ce1b2eaafc20419051776a0fdc881

commit c2c5dcb1451ce1b2eaafc20419051776a0fdc881
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Mon Apr 16 23:21:29 2018

Don't report video memory usage estimate when no frames are present.

preload=metadata suspend prevents decoding and rendering of the
first video frame. In these cases the video memory usage is zero,
but WMPI always includes an estimate for the frame held by the
compositor even if no frame is being held.

This artificially inflates the memory used in the preload=metadata
experiment. The fix is to not include this estimate if we've never
painted a frame before.

BUG= 833564 
TEST=new unittest.

Change-Id: I0b8811f04b27e5cc5c4ea5789cbd3d9247720771
Reviewed-on: https://chromium-review.googlesource.com/1014387
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551167}
[modify] https://crrev.com/c2c5dcb1451ce1b2eaafc20419051776a0fdc881/media/blink/webmediaplayer_impl.cc
[modify] https://crrev.com/c2c5dcb1451ce1b2eaafc20419051776a0fdc881/media/blink/webmediaplayer_impl.h
[modify] https://crrev.com/c2c5dcb1451ce1b2eaafc20419051776a0fdc881/media/blink/webmediaplayer_impl_unittest.cc

Project Member

Comment 4 by sheriffbot@chromium.org, Apr 18 2018

Labels: -Merge-Request-67 Merge-Approved-67 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M67. Please go ahead and merge the CL to branch 3396 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 5 by gov...@chromium.org, Apr 18 2018

Pls merge your change to M67 branch 3396 ASAP so we can pick it up for next M67 Dev/Beta release.

If already merged to M67 and nothing is pending, pls remove "Merge=Approved-67" label. Thank you.
Project Member

Comment 6 by bugdroid1@chromium.org, Apr 18 2018

Labels: -merge-approved-67 merge-merged-3396
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1539e4030b7d73e6562bcf0e4f47c4146376d7f3

commit 1539e4030b7d73e6562bcf0e4f47c4146376d7f3
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Wed Apr 18 20:02:33 2018

Merge M67: "Don't report video memory usage estimate when no frames are present."

preload=metadata suspend prevents decoding and rendering of the
first video frame. In these cases the video memory usage is zero,
but WMPI always includes an estimate for the frame held by the
compositor even if no frame is being held.

This artificially inflates the memory used in the preload=metadata
experiment. The fix is to not include this estimate if we've never
painted a frame before.

BUG= 833564 
TEST=new unittest.
TBR=dalecurtis@chromium.org

(cherry picked from commit c2c5dcb1451ce1b2eaafc20419051776a0fdc881)

Change-Id: I0b8811f04b27e5cc5c4ea5789cbd3d9247720771
Reviewed-on: https://chromium-review.googlesource.com/1014387
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#551167}
Reviewed-on: https://chromium-review.googlesource.com/1017765
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#95}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/1539e4030b7d73e6562bcf0e4f47c4146376d7f3/media/blink/webmediaplayer_impl.cc
[modify] https://crrev.com/1539e4030b7d73e6562bcf0e4f47c4146376d7f3/media/blink/webmediaplayer_impl.h
[modify] https://crrev.com/1539e4030b7d73e6562bcf0e4f47c4146376d7f3/media/blink/webmediaplayer_impl_unittest.cc

Status: Fixed (was: Started)

Sign in to add a comment