Enforce a limit on the number of WebMediaPlayers per frame. [chromium/src : main]
Chris Hamilton (Gerrit)
Attention is currently required from: Dale Curtis.
Chris Hamilton would like Dale Curtis to review this change.
Enforce a limit on the number of WebMediaPlayers per frame.
These objects are extremely heavy and a common cause of long-tail memory
usage in both browser and renderer processes.
This intervention blocks creation of further WebMediaPlayers once there
are already 75 (desktop) or 40 (mobile) players created in a frame.
These values were chosen as the 99.9th %ile values from instrumentation
counting the number of players. A failed creation will result in a
"Player load failure: error creating media player" message being
presented to the site, and a console warning.
BUG=1144736
Change-Id: I0cb30c9a9ef95981fb4d3e8563d6f5cbfdb30f42
---
M third_party/blink/public/platform/web_media_player.h
M third_party/blink/renderer/modules/modules_initializer.cc
2 files changed, 103 insertions(+), 2 deletions(-)
To view, visit change 2816118. To unsubscribe, or for help writing mail filters, visit settings.
Chris Hamilton (Gerrit)
Attention is currently required from: Dale Curtis.
1 comment:
Patchset:
Dale, can you sanity check this approach before I bother to write tests? I tried to keep the logic as close as possible to the creation point in the renderer (so as not to incur useless IPCs), and to make it easier to integrate with the console for intervention logging. I'm also not sure the LifetimeObserver for WebMediaPlayer is the best approach, but it's certainly the easiest.
To view, visit change 2816118. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Chris Hamilton.
1 comment:
Patchset:
Did my suggestion here https://bugs.chromium.org/p/chromium/issues/detail?id=1144736#c27 not work out? I.e., just have MediaFactory::CreateWebMediaPlayer() return nullptr when RendererWebMediaPlayerDelegate::id_map_.Size() > limit? Just needs a new method on the delegate for getting the size.
To view, visit change 2816118. To unsubscribe, or for help writing mail filters, visit settings.
Chris Hamilton (Gerrit)
Attention is currently required from: Dale Curtis.
1 comment:
Patchset:
Did my suggestion here https://bugs.chromium. […]
/facepalm
For whatever reason, I read that code as occurring on the *browser* side of the connection, and thought that it would be better to move it closer to to the call site in the renderer. Clearly I am an idiot, as I now realize that code is very much running in the renderer. I'll scrap this and do that (next week, I'm on vacation tomorrow)...
To view, visit change 2816118. To unsubscribe, or for help writing mail filters, visit settings.
Chris Hamilton (Gerrit)
1 comment:
Patchset:
Another look Dale? Working on tests now.
To view, visit change 2816118. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Chris Hamilton.
Patch set 3:Code-Review +1
2 comments:
Patchset:
media/ lgtm, thanks!
File content/renderer/media/renderer_webmediaplayer_delegate.h:
Patch Set #3, Line 69:
size_t GetWebMediaPlayerCount() const { return id_map_.size(); }snake_case() for accessors:
https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/c++/c++.md#inline-functions
To view, visit change 2816118. To unsubscribe, or for help writing mail filters, visit settings.
Chris Hamilton (Gerrit)
Attention is currently required from: Kentaro Hara.
Chris Hamilton would like Kentaro Hara to review this change.
Enforce a limit on the number of WebMediaPlayers per frame.
These objects are extremely heavy and a common cause of long-tail memory
usage in both browser and renderer processes.
This intervention blocks creation of further WebMediaPlayers once there
are already 75 (desktop) or 40 (mobile) players created in a frame.
These values were chosen as the 99.9th %ile values from instrumentation
counting the number of players. A failed creation will result in a
"Player load failure: error creating media player" message being
presented to the site, and a console warning.
BUG=1144736
Change-Id: I0cb30c9a9ef95981fb4d3e8563d6f5cbfdb30f42
---
M content/renderer/media/media_factory.cc
M content/renderer/media/media_factory.h
M content/renderer/media/renderer_webmediaplayer_delegate.h
A content/renderer/media/too_many_web_media_players_intervention_browsertest.cc
M content/test/BUILD.gn
A content/test/data/media/too_many_web_media_players_intervention_test.html
M third_party/blink/public/web/web_local_frame.h
M third_party/blink/renderer/core/frame/web_local_frame_impl.cc
M third_party/blink/renderer/core/frame/web_local_frame_impl.h
9 files changed, 155 insertions(+), 2 deletions(-)
Chris Hamilton (Gerrit)
Attention is currently required from: Kentaro Hara.
2 comments:
Patchset:
Dale: Added a browser test. Another look?
+haraken for blink
File content/renderer/media/renderer_webmediaplayer_delegate.h:
Patch Set #3, Line 69:
size_t GetWebMediaPlayerCount() const { return id_map_.size(); }
snake_case() for accessors: […]
I was always under the impression that this was only true if the accessor mirrors the name of the variable being accessed, which is not true in this case.
Looking quickly at occurrences of const member functions that simply return foo.size():
Function has snake-style name: 1258 occurrences [1]
Function does not have snake-style name: 519 occurrences [2]So it appears snake-style is preferred more generally, not just in the case where variable name and accessor names are mirrored. TIL.
Done.
To view, visit change 2816118. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Chris Hamilton, Kentaro Hara.
Patch set 5:Code-Review +1
3 comments:
Patchset:
That test is likely to end up flaky on the bots, they don't do well with this many videos generally.
File content/test/data/media/too_many_web_media_players_intervention_test.html:
Patch Set #5, Line 19:
await v.play();I wouldn't play them since that'll eat a lot of resource unnecessarily.
Patch Set #5, Line 26:
<video width="320" height="240" controls>You may want to use a small data:// or blob:// URL instead of URL based loading for all tags since that can be resource heavy. fetch() => blob would also work.
Since we don't care if these actually play or not, just that the tags are loaded, you can provide a poster image and set preload=metadata to ensure we only do the minimal possible loading for each video.
To view, visit change 2816118. To unsubscribe, or for help writing mail filters, visit settings.
Chris Hamilton (Gerrit)
Attention is currently required from: Dale Curtis, Kentaro Hara.
3 comments:
Patchset:
That test is likely to end up flaky on the bots, they don't do well with this many videos generally.
Ack. Some tests are failing because of "excessive output". I'll inject a testing seam that lets me set the cap much lower.
I've done this via a command-line flag, but it feels dirty. Any idea what is the best practice for configuring a renderer for test-only behavior from a browser process?
File content/test/data/media/too_many_web_media_players_intervention_test.html:
Patch Set #5, Line 19:
await v.play();I wouldn't play them since that'll eat a lot of resource unnecessarily.
Done
Patch Set #5, Line 26:
<video width="320" height="240" controls>
You may want to use a small data:// or blob:// URL instead of URL based loading for all tags since t […]
Thanks for the pointers, I know basically nothing of these APIs :)
To view, visit change 2816118. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Chris Hamilton, Kentaro Hara.
Patch set 6:Code-Review +1
1 comment:
File content/public/common/content_switches.cc:
Patch Set #6, Line 542:
"limit-web-media-players-for-testing";I think I'd just call this --web-media-player-limit <###> so if we run into issues we can have folks debug by setting the limit higher.
To view, visit change 2816118. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Chris Hamilton, Kentaro Hara.
1 comment:
File content/test/data/media/too_many_web_media_players_intervention_test.html:
Patch Set #6, Line 24:
poster="data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7">Oh for poster you can just use an image file. I meant for the video src you should use a blob or data url.
To view, visit change 2816118. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
File content/test/data/media/too_many_web_media_players_intervention_test.html:
Patch Set #6, Line 24:
poster="data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7">
Oh for poster you can just use an image file. […]
(Though it may not matter with a much lower limit)
To view, visit change 2816118. To unsubscribe, or for help writing mail filters, visit settings.
Kentaro Hara (Gerrit)
Attention is currently required from: Chris Hamilton.
Patchset:
blink LGTM
To view, visit change 2816118. To unsubscribe, or for help writing mail filters, visit settings.
Chris Hamilton (Gerrit)
Attention is currently required from: Nasko Oskov.
Chris Hamilton would like Nasko Oskov to review this change.
Enforce a limit on the number of WebMediaPlayers per frame.
These objects are extremely heavy and a common cause of long-tail memory
usage in both browser and renderer processes.
This intervention blocks creation of further WebMediaPlayers once there
are already 75 (desktop) or 40 (mobile) players created in a frame.
These values were chosen as the 99.9th %ile values from instrumentation
counting the number of players. A failed creation will result in a
"Player load failure: error creating media player" message being
presented to the site, and a console warning.
BUG=1144736
Change-Id: I0cb30c9a9ef95981fb4d3e8563d6f5cbfdb30f42
---
M content/browser/renderer_host/render_process_host_impl.cc
M content/public/common/content_switches.cc
M content/public/common/content_switches.h
M content/renderer/media/media_factory.cc
M content/renderer/media/renderer_webmediaplayer_delegate.h
A content/renderer/media/too_many_web_media_players_intervention_browsertest.cc
M content/test/BUILD.gn
A content/test/data/media/too_many_web_media_players_intervention_test.html
M third_party/blink/public/web/web_local_frame.h
M third_party/blink/renderer/core/frame/web_local_frame_impl.cc
M third_party/blink/renderer/core/frame/web_local_frame_impl.h
11 files changed, 171 insertions(+), 2 deletions(-)
Chris Hamilton (Gerrit)
Attention is currently required from: Nasko Oskov.
3 comments:
Patchset:
Thanks! Refactored the flag to make it non-test-only.
+nasko@ for content/ changes.
File content/public/common/content_switches.cc:
Patch Set #6, Line 542:
"limit-web-media-players-for-testing";
I think I'd just call this --web-media-player-limit <###> so if we run into issues we can have folks […]
Done.
File content/test/data/media/too_many_web_media_players_intervention_test.html:
Patch Set #6, Line 24:
poster="data:image/gif;base64,R0lGODlhAQABAIAAAAAAAP///yH5BAEAAAAALAAAAAABAAEAAAIBRAA7">
(Though it may not matter with a much lower limit)
Ack
To view, visit change 2816118. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Chris Hamilton, Nasko Oskov.
Patch set 7:Code-Review +1
2 comments:
File content/renderer/media/media_factory.cc:
Patch Set #7, Line 120:
static constexpr size_t kDefaultMaxWebMediaPlayers =Drop static here and below. Not needed in anonymous namespace.
Patch Set #7, Line 129:
static size_t max_web_media_players = 0;Since this is on the render thread it's probably fine, but generally prefer:
static const size_t kMaxWebMediaPlayers = []() {
...
return limit;
};
return kMaxWebMediaPlayers;to ensure thread safe static initialization I think.
To view, visit change 2816118. To unsubscribe, or for help writing mail filters, visit settings.
Chris Hamilton (Gerrit)
Attention is currently required from: Nasko Oskov.
2 comments:
File content/renderer/media/media_factory.cc:
Patch Set #7, Line 120:
static constexpr size_t kDefaultMaxWebMediaPlayers =Drop static here and below. Not needed in anonymous namespace.
Oops, cut and paste error when moving this from the public header.
Patch Set #7, Line 129:
static size_t max_web_media_players = 0;
Since this is on the render thread it's probably fine, but generally prefer: […]
Good point... I was assuming this would only run on the main thread, but if Blink ever becomes multi-threaded (long term place) it indeed does need to be thread safe.
Done.
To view, visit change 2816118. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Chris Hamilton, Nasko Oskov.
Patch set 8:Code-Review +1
1 comment:
Patchset:
lgtm, thanks!
To view, visit change 2816118. To unsubscribe, or for help writing mail filters, visit settings.
Nasko Oskov (Gerrit)
Attention is currently required from: Chris Hamilton.
Patchset:
Non-media //content/ changes LGTM
To view, visit change 2816118. To unsubscribe, or for help writing mail filters, visit settings.
Chris Hamilton (Gerrit)
1 comment:
Patchset:
Thanks all, committing!
To view, visit change 2816118. To unsubscribe, or for help writing mail filters, visit settings.
Chris Hamilton (Gerrit)
Patch set 8:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Enforce a limit on the number of WebMediaPlayers per frame.
These objects are extremely heavy and a common cause of long-tail memory
usage in both browser and renderer processes.
This intervention blocks creation of further WebMediaPlayers once there
are already 75 (desktop) or 40 (mobile) players created in a frame.
These values were chosen as the 99.9th %ile values from instrumentation
counting the number of players. A failed creation will result in a
"Player load failure: error creating media player" message being
presented to the site, and a console warning.
BUG=1144736
Change-Id: I0cb30c9a9ef95981fb4d3e8563d6f5cbfdb30f42
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2816118
Reviewed-by: Dale Curtis <dalec...@chromium.org>
Reviewed-by: Nasko Oskov <na...@chromium.org>
Reviewed-by: Kentaro Hara <har...@chromium.org>
Commit-Queue: Chris Hamilton <chr...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#872232}
---
M content/browser/renderer_host/render_process_host_impl.cc
M content/public/common/content_switches.cc
M content/public/common/content_switches.h
M content/renderer/media/media_factory.cc
M content/renderer/media/renderer_webmediaplayer_delegate.h
A content/renderer/media/too_many_web_media_players_intervention_browsertest.cc
M content/test/BUILD.gn
A content/test/data/media/too_many_web_media_players_intervention_test.html
M third_party/blink/public/web/web_local_frame.h
M third_party/blink/renderer/core/frame/web_local_frame_impl.cc
M third_party/blink/renderer/core/frame/web_local_frame_impl.h
11 files changed, 169 insertions(+), 2 deletions(-)
Chris Hamilton (Gerrit)
3 comments:
Patchset:
/facepalm […]
Done
Patchset:
Ack. Some tests are failing because of "excessive output". […]
Done
Patchset:
(Closing out some open comments.)
To view, visit change 2816118. To unsubscribe, or for help writing mail filters, visit settings.
John Warburton (Gerrit)
1 comment:
Patchset:
Hello, I'm just a user and web programmer, but is there a way of disabling this limit? It's broken a small app used here for helping to de-duplicate tracks in a music database. About 230 players are loaded up in a simple table, each linked to an MP3 file, so possible duplicate pieces of music can be auditioned.
Thank you for looking at this.
jo...@johnwarburton.net
To view, visit change 2816118. To unsubscribe, or for help writing mail filters, visit settings.
Dale Curtis (Gerrit)
Attention is currently required from: Chris Hamilton.
1 comment:
Patchset:
Hello, I'm just a user and web programmer, but is there a way of disabling this limit? It's broken a […]
You should just need to mark the <audio> tags with preload=none to avoid creating all the WebMediaPlayers before use. With preload=none they'll only be created when played. Unless you expect users to start more than 75 concurrently on desktop or 40+ on Android that should solve your problem.
To view, visit change 2816118. To unsubscribe, or for help writing mail filters, visit settings.
John Warburton (Gerrit)
Attention is currently required from: Chris Hamilton.
Thank you very much for this suggestion.
It sounds entirely practical and I'll try it tomorrow.
With best wishes,
JW
Riley Dutton
Joseph Anderson
Minh Son TRAN (Gerrit)
Attention is currently required from: Chris Hamilton.
2 comments:
Patchset:
You should just need to mark the <audio> tags with preload=none to avoid creating all the WebMediaPl […]
Dear Dale Curtis,
I have the same problem as John Warburton but with video track. We host up to 35 cameras concurrently in a video conference, and they eat up very quickly the limit 40.
Everything worked well with the previous Chrome Release. The Version 92 just drops the cameras.Is it possible to increase or disable this feature?
Many thanksDear Dale Curtis,
I have the same problem as John Warburton but with video track. We host up to 35 cameras concurrently in a video conference, and they eat up very quickly the limit 40.
Everything worked well with the previous Chrome Release. The Version 92 just drops the cameras.Is it possible to increase or disable this feature?
Many thanks
To view, visit change 2816118. To unsubscribe, or for help writing mail filters, visit settings.
Mattie Ruth Kramer Backman (Gerrit)
Attention is currently required from: Chris Hamilton.
1 comment:
Patchset:
Hi!
I am a developer at Daily (http://daily.co). We provide a WebRTC API and hosting service. This change significantly impacts the capabilities we provide on Chrome. As such, I would like to vote to consider reverting this change. We have customers with a wide variety of use cases that suddenly no longer work. For instance, calls with > 37 simultaneous participants, all sharing audio and video, large gridded views of participants (>75), and clubhouse-like mobile pages with >40 audio-only participants.I believe this not only limits several legitimate WebRTC use-cases, but is a regression against the great strides which browsers, especially Chrome, have made over the last few years. It is true that UIs can work around the limitation to some degree with better accounting of elements in use (removing srcObject on muted participants or combining audio tracks into one element, for instance). However, developers are painted into a corner with how they implement their UIs. Also, we have experienced issues with things like combined audio tracks introducing crackle.
If this limitation is to stay, then I'd like to request three things.
1. A fix for https://bugs.chromium.org/p/chromium/issues/detail?id=1232455&q=webmediaplayer&can=2 which can easily trip developers up and increase their chance of hitting this limitation despite only having a few media elements on the DOM.
2. A way to dynamically query the number of WebMediaPlayers so that we can detect when we may hit this limitation prior to doing so and react accordingly.
3. API to dynamically increase or remove the limitation so that we can chose to push chrome's limits based on use-case. `preload=none` has been mentioned as a work-around but it does not work with `autoplay` and does not actually change the limit.Thank you for your consideration!
To view, visit change 2816118. To unsubscribe, or for help writing mail filters, visit settings.
Philipp Hancke (Gerrit)
Patchset:
commenting on old changes ends up being confusing, I've filed https://bugs.chromium.org/p/chromium/issues/detail?id=1232649 for discussion.
To view, visit change 2816118. To unsubscribe, or for help writing mail filters, visit settings.
Chris Hamilton (Gerrit)
1 comment:
Patchset:
Dear Dale Curtis, […]
Resolving this line of comments. Please prefer to comment on the related bug at https://bugs.chromium.org/p/chromium/issues/detail?id=1232649
To view, visit change 2816118. To unsubscribe, or for help writing mail filters, visit settings.
전상인 (Gerrit)
1 comment:
Patchset:
Hello I'm a web developer, struggling with this issue for days.
In my website, there are pages with multiple <audio> elements (sometimes more than 100, not fixed). All of them are set to 'preload=none'. An end-user plays the audio consecutively, one by one. Sometimes one or two audios might overlap, but definitely not 75 players would be played at the same time. (I'm sure about this because the website is an in-house product)
The user does this task for hours, navigating from a page to another, which also has multiple <audio> sources. They get the error ([Intervention] Blocked attempt to create a web media player as there are too many webmediaplayers already in existence) once in a while, usually in a 40min ~ 1hour period.
Can't find a reason.. any help or suggestions are welcomed. Thanks.
To view, visit change 2816118. To unsubscribe, or for help writing mail filters, visit settings.