New issue
Advanced search Search tips

Issue 821961 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Jul 20
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug-Regression

Blocked on:
issue 828478
issue 829941

Blocking:
issue 833461


Participants' hotlists:
Modern-Media-Controls


Sign in to add a comment

1.5%-651.2% regression in media.desktop at 542476:542658

Project Member Reported by jrumm...@chromium.org, Mar 14 2018

Issue description

See the link to graphs below.
 
Project Member

Comment 1 by 42576172...@developer.gserviceaccount.com, Mar 14 2018

All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=821961

(For debugging:) Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?sid=4057930187cc8f1bb72707b05533f66551456ffa23cb01e4826ebfd10959afd7


Bot(s) for this bug's original alert(s):

chromium-rel-mac11-air
chromium-rel-mac11-pro
chromium-rel-mac12
chromium-rel-win7-gpu-intel
chromium-rel-win7-gpu-nvidia
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Mar 14 2018

Cc: mlamouri@chromium.org isherman@chromium.org dalecur...@chromium.org steimel@chromium.org
Owner: steimel@chromium.org
Status: Assigned (was: Untriaged)
📍 Found a significant difference after 1 commit.
https://pinpoint-dot-chromeperf.appspot.com/job/12a3a17e440000

Add fieldtrial testing config for new media controls by steimel@chromium.org
https://chromium.googlesource.com/chromium/src/+/fdfc5ae4154a67e0539b9d0a77b3c5e2da38f40b

Understanding performance regressions:
  http://g.co/ChromePerformanceRegressions
Cc: beccahughes@chromium.org
Components: Blink>Media>Controls
Project Member

Comment 6 by 42576172...@developer.gserviceaccount.com, Mar 29 2018

📍 Couldn't reproduce a difference.
https://pinpoint-dot-chromeperf.appspot.com/job/101583bb440000
Labels: -Pri-2 Pri-1
Owner: beccahughes@chromium.org
Project Member

Comment 9 by 42576172...@developer.gserviceaccount.com, Mar 29 2018

📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/11f2c47b440000
Project Member

Comment 11 by 42576172...@developer.gserviceaccount.com, Mar 29 2018

📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/14e89eb7440000
Project Member

Comment 13 by 42576172...@developer.gserviceaccount.com, Mar 31 2018

📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/14809a37440000
Project Member

Comment 14 by bugdroid1@chromium.org, Apr 2 2018

Project Member

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

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

commit 5f02ed9270f274ed7dd1cc6a861bcd6eb2d65892
Author: Becca Hughes <beccahughes@chromium.org>
Date: Tue Apr 03 16:18:08 2018

Media Controls: Only update slider positions when needed

The slider positions are constantly updated even when they have
not changed. This changes them to only be updated when changed.

BUG= 821961 ,821414

Change-Id: Ia978e130d51915bce121913db7f66caccbd9e636
Reviewed-on: https://chromium-review.googlesource.com/991075
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Commit-Queue: Becca Hughes <beccahughes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547727}
[modify] https://crrev.com/5f02ed9270f274ed7dd1cc6a861bcd6eb2d65892/third_party/WebKit/Source/modules/media_controls/elements/MediaControlSliderElement.cpp

😿 Pinpoint job stopped with an error.
https://pinpoint-dot-chromeperf.appspot.com/job/11c9f3ef440000
😿 Pinpoint job stopped with an error.
https://pinpoint-dot-chromeperf.appspot.com/job/14ab05bf440000
Project Member

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

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

commit 07b346a00facb25e8458e18b5f6546a059d81c1c
Author: Becca Hughes <beccahughes@chromium.org>
Date: Tue Apr 03 17:19:57 2018

Media Controls: Clean up the DOM

Do not create the volume slider on the new media controls
as we don't need it. Also, avoids adding elements to the
DOM that are permanently hidden.

BUG= 821961 ,821414

Change-Id: Ia0e334b6079450d4a3b084371629f6b6ba37176c
Reviewed-on: https://chromium-review.googlesource.com/991293
Commit-Queue: Becca Hughes <beccahughes@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547743}
[modify] https://crrev.com/07b346a00facb25e8458e18b5f6546a059d81c1c/third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp
[modify] https://crrev.com/07b346a00facb25e8458e18b5f6546a059d81c1c/third_party/WebKit/Source/modules/media_controls/MediaControlsWindowEventListener.cpp
[modify] https://crrev.com/07b346a00facb25e8458e18b5f6546a059d81c1c/third_party/WebKit/Source/modules/media_controls/MediaControlsWindowEventListener.h

Blockedon: 828478
📍 Couldn't reproduce a difference.
https://pinpoint-dot-chromeperf.appspot.com/job/148dc3b0c40000
Project Member

Comment 24 by bugdroid1@chromium.org, Apr 5 2018

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

commit 16d83bf550e6d1580ef68c646db29a1d04878278
Author: Becca Hughes <beccahughes@chromium.org>
Date: Thu Apr 05 19:25:15 2018

Media Controls: Move to different layer

will-change will force the media controls to use a different
layer. According to Pinpoint this will reduce CC memory usage
by 18% and GPU memory usage by 6%.

BUG= 821961 ,821414

Change-Id: I13c62f79577dfef253acf5f3b89cecaf0c5c1ad3
Reviewed-on: https://chromium-review.googlesource.com/996309
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Commit-Queue: Becca Hughes <beccahughes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548524}
[modify] https://crrev.com/16d83bf550e6d1580ef68c646db29a1d04878278/third_party/WebKit/Source/modules/media_controls/resources/modernMediaControls.css

Project Member

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

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

commit 041244a9de147288c4e99b6201cda3417710e29f
Author: Becca Hughes <beccahughes@chromium.org>
Date: Fri Apr 06 03:03:42 2018

Media Controls: Create scrubbing message on demand

The scrubbing message has lots of child DOM elements. This
creates/prunes them when the visiblity of the scrubbing message changes.

BUG= 821961 ,821414

Change-Id: I4f0b2dc2c276387be1867ff6459d594dc97d3545
Reviewed-on: https://chromium-review.googlesource.com/990480
Commit-Queue: Becca Hughes <beccahughes@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Tommy Steimel <steimel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548649}
[modify] https://crrev.com/041244a9de147288c4e99b6201cda3417710e29f/third_party/WebKit/Source/modules/BUILD.gn
[modify] https://crrev.com/041244a9de147288c4e99b6201cda3417710e29f/third_party/WebKit/Source/modules/media_controls/elements/MediaControlElementBase.h
[modify] https://crrev.com/041244a9de147288c4e99b6201cda3417710e29f/third_party/WebKit/Source/modules/media_controls/elements/MediaControlScrubbingMessageElement.cpp
[modify] https://crrev.com/041244a9de147288c4e99b6201cda3417710e29f/third_party/WebKit/Source/modules/media_controls/elements/MediaControlScrubbingMessageElement.h
[add] https://crrev.com/041244a9de147288c4e99b6201cda3417710e29f/third_party/WebKit/Source/modules/media_controls/elements/MediaControlScrubbingMessageElementTest.cpp

Project Member

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

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

commit 0631e18f16653c8e262079e7ee7300e70ec2b9a3
Author: Becca Hughes <beccahughes@chromium.org>
Date: Fri Apr 06 17:19:29 2018

Media Controls: Remove manual paint invalidation

The media controls were performing manual paint invalidation
from when we used to have MediaControlPainter. Now we use
CSS this is handled for us automatically.

According to manual testing this reduces the time painting
from 153 ms to 49ms.

BUG= 821961 ,821414

Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_layout_ng
Change-Id: Ie864f914c7ced0031710d81a7a702c8e3697a312
Reviewed-on: https://chromium-review.googlesource.com/998834
Commit-Queue: Becca Hughes <beccahughes@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548832}
[modify] https://crrev.com/0631e18f16653c8e262079e7ee7300e70ec2b9a3/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[modify] https://crrev.com/0631e18f16653c8e262079e7ee7300e70ec2b9a3/third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
[delete] https://crrev.com/4cb700f0379ba40cb6c69b95cc1022d4e7a633c7/third_party/WebKit/LayoutTests/paint/invalidation/video-mute-repaint-expected.txt
[delete] https://crrev.com/4cb700f0379ba40cb6c69b95cc1022d4e7a633c7/third_party/WebKit/LayoutTests/paint/invalidation/video-mute-repaint.html
[delete] https://crrev.com/4cb700f0379ba40cb6c69b95cc1022d4e7a633c7/third_party/WebKit/LayoutTests/paint/invalidation/video-unmute-repaint-expected.txt
[delete] https://crrev.com/4cb700f0379ba40cb6c69b95cc1022d4e7a633c7/third_party/WebKit/LayoutTests/paint/invalidation/video-unmute-repaint.html
[modify] https://crrev.com/0631e18f16653c8e262079e7ee7300e70ec2b9a3/third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp
[modify] https://crrev.com/0631e18f16653c8e262079e7ee7300e70ec2b9a3/third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.h
[modify] https://crrev.com/0631e18f16653c8e262079e7ee7300e70ec2b9a3/third_party/WebKit/Source/modules/media_controls/elements/MediaControlElementBase.cpp

Blockedon: 829941
Project Member

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

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

commit a17997b912ab317b9e73ca786523a03759b6725d
Author: Becca Hughes <beccahughes@chromium.org>
Date: Fri Apr 06 18:40:54 2018

Media Controls: Avoid style recalculation

When generating the media controls DOM we were recalculating
styles each time we called 'AppendChild'. This moves us
to using 'ParserAppendChild' which does not do this.

According to manual testing this reduces the time calculating
styles from 230ms to 139ms.

BUG= 821961 ,821414

Change-Id: I0d673ddc6e12ff81a311315466be735a7673c1bd
Reviewed-on: https://chromium-review.googlesource.com/998187
Commit-Queue: Becca Hughes <beccahughes@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548872}
[modify] https://crrev.com/a17997b912ab317b9e73ca786523a03759b6725d/third_party/WebKit/Source/modules/media_controls/MediaControlsImpl.cpp
[modify] https://crrev.com/a17997b912ab317b9e73ca786523a03759b6725d/third_party/WebKit/Source/modules/media_controls/elements/MediaControlElementsHelper.cpp
[modify] https://crrev.com/a17997b912ab317b9e73ca786523a03759b6725d/third_party/WebKit/Source/modules/media_controls/elements/MediaControlInputElement.cpp
[modify] https://crrev.com/a17997b912ab317b9e73ca786523a03759b6725d/third_party/WebKit/Source/modules/media_controls/elements/MediaControlLoadingPanelElement.cpp
[modify] https://crrev.com/a17997b912ab317b9e73ca786523a03759b6725d/third_party/WebKit/Source/modules/media_controls/elements/MediaControlOverlayPlayButtonElement.cpp
[modify] https://crrev.com/a17997b912ab317b9e73ca786523a03759b6725d/third_party/WebKit/Source/modules/media_controls/elements/MediaControlScrubbingMessageElement.cpp
[modify] https://crrev.com/a17997b912ab317b9e73ca786523a03759b6725d/third_party/WebKit/Source/modules/media_controls/elements/MediaControlTextTrackListElement.cpp
[modify] https://crrev.com/a17997b912ab317b9e73ca786523a03759b6725d/third_party/WebKit/Source/modules/media_controls/elements/MediaControlTimelineElement.cpp

Project Member

Comment 30 by bugdroid1@chromium.org, Apr 9 2018

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

commit 138363ffd7c05f6152572cfc7e0869ff4484d743
Author: Becca Hughes <beccahughes@chromium.org>
Date: Mon Apr 09 20:44:40 2018

Media Controls: Re-create the play button

As per steimel@'s comment in crrev.com/c/991293 we should always
create the play button as we use it when the video element is
very small.

BUG= 821961 ,821414

Change-Id: Icec781d0cd49529c72dd21c88d54727e81236890
Reviewed-on: https://chromium-review.googlesource.com/1000722
Commit-Queue: Becca Hughes <beccahughes@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549266}
[modify] https://crrev.com/138363ffd7c05f6152572cfc7e0869ff4484d743/third_party/blink/renderer/modules/media_controls/media_controls_impl.cc

Status: Started (was: Assigned)
Project Member

Comment 32 by bugdroid1@chromium.org, Apr 10 2018

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

commit 5ad3a0af487c043c920283d1bc76b857c4f03ca6
Author: Becca Hughes <beccahughes@chromium.org>
Date: Tue Apr 10 10:27:15 2018

Revert "Media Controls: Move to different layer"

This reverts commit 16d83bf550e6d1580ef68c646db29a1d04878278.

Reason for revert: Performance issues

Original change's description:
> Media Controls: Move to different layer
>
> will-change will force the media controls to use a different
> layer. According to Pinpoint this will reduce CC memory usage
> by 18% and GPU memory usage by 6%.
>
> BUG= 821961 ,821414
>
> Change-Id: I13c62f79577dfef253acf5f3b89cecaf0c5c1ad3
> Reviewed-on: https://chromium-review.googlesource.com/996309
> Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
> Commit-Queue: Becca Hughes <beccahughes@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#548524}

TBR=mlamouri@chromium.org,beccahughes@chromium.org


Bug:  821961 , 821414
Change-Id: I5df8f67e4e779dc673ca9cf89221c734c5a2235c
Reviewed-on: https://chromium-review.googlesource.com/1003332
Commit-Queue: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Becca Hughes <beccahughes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549475}
[modify] https://crrev.com/5ad3a0af487c043c920283d1bc76b857c4f03ca6/third_party/blink/renderer/modules/media_controls/resources/modernMediaControls.css

Project Member

Comment 33 by bugdroid1@chromium.org, Apr 11 2018

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

commit b5d01e75a40d842fa2575225a194e16396c34dc4
Author: Becca Hughes <beccahughes@chromium.org>
Date: Wed Apr 11 01:42:27 2018

Media Controls: Do not update the time if we are hidden

If the panel is hidden then we should not update the current time
unnecessarily.

BUG= 821961 ,821414

Change-Id: I5c3fed448c52edfb14570d5e2e164c56e4747672
Reviewed-on: https://chromium-review.googlesource.com/1006068
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Commit-Queue: Becca Hughes <beccahughes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549720}
[modify] https://crrev.com/b5d01e75a40d842fa2575225a194e16396c34dc4/third_party/blink/renderer/modules/media_controls/media_controls_impl.cc

Blocking: 833461
Status: WontFix (was: Started)
Most of these are fixed, but the remainder are to be expected with the added complexity of the new controls.

Sign in to add a comment