Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1182183004:
Allow setting memory limits through WebMediaPlayer interface (Blink)
|
Created:
5 years, 6 months ago by servolk Modified:
5 years, 5 months ago CC:
blink-reviews, mlamouri+watch-blink_chromium.org, feature-media-reviews_chromium.org, dglazkov+blink, philipj_slow, eric.carlson_apple.com Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionDepends on https://codereview.chromium.org/1171263004/
Implementation: https://codereview.chromium.org/1185453004/
BUG=498482
Patch Set 1 #Patch Set 2 : size_t #
Total comments: 2
Patch Set 3 : Added a note about get/set buffer size working only for MSE for now #
Total comments: 11
Messages
Total messages: 10 (2 generated)
servolk@chromium.org changed reviewers: + philipj@opera.com
https://codereview.chromium.org/1182183004/diff/20001/public/platform/WebMedi... File public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/1182183004/diff/20001/public/platform/WebMedi... public/platform/WebMediaPlayer.h:150: virtual size_t getAudioBufferSize() const = 0; Can you document that these are for MSE and nothing else?
https://codereview.chromium.org/1182183004/diff/20001/public/platform/WebMedi... File public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/1182183004/diff/20001/public/platform/WebMedi... public/platform/WebMediaPlayer.h:150: virtual size_t getAudioBufferSize() const = 0; On 2015/06/17 07:51:12, philipj wrote: > Can you document that these are for MSE and nothing else? Done. Eventually we might make this work for other source types (e.g. direct URL playback), but that will require some additional work in FFmpegDemuxer and is not high-priority feature right now).
lgtm
ddorwin@chromium.org changed reviewers: + ddorwin@chromium.org
I realize the current Chromium implementation has separate limits per audio and video stream (and supports a single stream of each), but is that the best solution and something we want to codify in this API? Is there a CL showing usage and why this should be in WMP instead of an MSE-specific interface? https://codereview.chromium.org/1182183004/diff/40001/public/platform/WebMedi... File public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/1182183004/diff/40001/public/platform/WebMedi... public/platform/WebMediaPlayer.h:150: // Getting/setting buffer sizes is currently only implemented for MSE. Is there a CL showing how this will be used? Would it make sense to have these methods on an MSE interface instead? https://codereview.chromium.org/1182183004/diff/40001/public/platform/WebMedi... public/platform/WebMediaPlayer.h:151: virtual size_t getAudioBufferSize() const = 0; I don't think these are "buffer" sizes. IMO, a buffer is a single entity. This is more a cache size or max memory usage, right? See also https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/sour... https://codereview.chromium.org/1182183004/diff/40001/public/platform/WebMedi... public/platform/WebMediaPlayer.h:152: virtual void setAudioBufferSize(size_t) = 0; Does a per-player value make sense? Perhaps for Cast (one tab, usually one video element), but what about other platforms where there might be more than one video per page or multiple tabs playing video or audio? Any assumptions made about the platform when implementing custom sizes may not make sense when there are two or more elements. https://codereview.chromium.org/1182183004/diff/40001/public/platform/WebMedi... public/platform/WebMediaPlayer.h:153: virtual size_t getVideoBufferSize() const = 0; "Audio" and "Video" restricts the Blink API to one stream of each type unless we state that this is the total for each type of stream. What about embedded text tracks? Note that "audio" and "video" appear very rarely in this interface. Do we need separate audio and video sizes? How does the platform know better than the app how to allocate the total available RAM? (I realize the current implementation does this, but if we're going to extend it, perhaps we should reevaluate it.)
https://codereview.chromium.org/1182183004/diff/40001/public/platform/WebMedi... File public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/1182183004/diff/40001/public/platform/WebMedi... public/platform/WebMediaPlayer.h:150: // Getting/setting buffer sizes is currently only implemented for MSE. On 2015/06/17 20:31:35, ddorwin wrote: > Is there a CL showing how this will be used? Would it make sense to have these > methods on an MSE interface instead? Please see the CL given in the description (https://codereview.chromium.org/1171263004/ and crbug.com/498482) for more details/context about why we need better control of memory limits. I think these methods would be useful in WMP interface, since it would allow a uniform interface for controlling memory usage for both MSE and direct URL playback (not implemented in FFmpegDemuxer yet, but will be in the future). This should allow us to reduce running times of blink-level tests for MSE garbage collection (see https://codereview.chromium.org/1013923002/), because with current default buffer sizes (min 12Mb for audio streams) it takes up to 20 seconds for SB.appendStream to append enough data to the source buffer to trigger GC. https://codereview.chromium.org/1182183004/diff/40001/public/platform/WebMedi... public/platform/WebMediaPlayer.h:151: virtual size_t getAudioBufferSize() const = 0; On 2015/06/17 20:31:35, ddorwin wrote: > I don't think these are "buffer" sizes. IMO, a buffer is a single entity. This > is more a cache size or max memory usage, right? > > See also > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/sour... The word "buffer" indeed has a different meaning for people that work on the lower levels of media pipeline. But on blink level the word "buffer" means "memory set aside for storing media data", as in "SourceBuffer". And this API essentially allows you to set the memory limit for the corresponding SourceBuffer. https://codereview.chromium.org/1182183004/diff/40001/public/platform/WebMedi... public/platform/WebMediaPlayer.h:152: virtual void setAudioBufferSize(size_t) = 0; On 2015/06/17 20:31:35, ddorwin wrote: > Does a per-player value make sense? Perhaps for Cast (one tab, usually one video > element), but what about other platforms where there might be more than one > video per page or multiple tabs playing video or audio? > > Any assumptions made about the platform when implementing custom sizes may not > make sense when there are two or more elements. I think per-player setting gives us maximum flexibility while still keeping API reasonably simple. We could, of course, have a similar set of methods for setting global defaults, but what's the point of that if we could have a more flexible API with approximately the same code footprint? If there's more than one video per page that's totally fine. In fact I think this design (per-player as opposed to global setting) would help us. Since if there are multiple video elements the browser can be smart and, for example, set larger memory limits for currently playing/visible media element and reduce memory limits for media elements that are in the invisible area of the web page, in the paused state or in an inactive browser tab. https://codereview.chromium.org/1182183004/diff/40001/public/platform/WebMedi... public/platform/WebMediaPlayer.h:153: virtual size_t getVideoBufferSize() const = 0; On 2015/06/17 20:31:35, ddorwin wrote: > "Audio" and "Video" restricts the Blink API to one stream of each type unless we > state that this is the total for each type of stream. What about embedded text > tracks? > > Note that "audio" and "video" appear very rarely in this interface. > > > Do we need separate audio and video sizes? How does the platform know better > than the app how to allocate the total available RAM? (I realize the current > implementation does this, but if we're going to extend it, perhaps we should > reevaluate it.) As far as I know memory usage by embedded text tracks is rarely a concern and we can live with default memory limits for that. I agree with you point about this API being limited to 1 audio and/or video stream, but there are a lot of places in media/ that make the same assumption. For example, Audio/VideoRenderer::Initialize methods take just one input DemuxerStream parameter. media::DemuxerStreamProvider::GetStream(AUDIO) also returns only 1 stream instead of collection/iterator. All these places would need to be changed to support more than 1 audio/video streams, so I thought it would be ok to make the same assumption here. Besides there's an easy way to deal with this here - if there are multiple audio and/or video streams, then setAudioBufferSize will enforce memory limit for all audio streams, setVideoBufferSize will enforce memory limit for all video streams. I don't think we'll ever need more flexibility than this.
https://codereview.chromium.org/1182183004/diff/40001/public/platform/WebMedi... File public/platform/WebMediaPlayer.h (right): https://codereview.chromium.org/1182183004/diff/40001/public/platform/WebMedi... public/platform/WebMediaPlayer.h:150: // Getting/setting buffer sizes is currently only implemented for MSE. On 2015/06/17 21:39:44, servolk wrote: > On 2015/06/17 20:31:35, ddorwin wrote: > > Is there a CL showing how this will be used? Would it make sense to have these > > methods on an MSE interface instead? > > Please see the CL given in the description > (https://codereview.chromium.org/1171263004/ and crbug.com/498482) for more > details/context about why we need better control of memory limits. > I think these methods would be useful in WMP interface, since it would allow a > uniform interface for controlling memory usage for both MSE and direct URL > playback (not implemented in FFmpegDemuxer yet, but will be in the future). This > should allow us to reduce running times of blink-level tests for MSE garbage > collection (see https://codereview.chromium.org/1013923002/), because with > current default buffer sizes (min 12Mb for audio streams) it takes up to 20 > seconds for SB.appendStream to append enough data to the source buffer to > trigger GC. Whether we need a uniform interface for controlling memory usage is irrelevant to whether we need to expose this to Blink. We can have a uniform interface in Chromium without this change. (As mentioned in https://codereview.chromium.org/1171263004/, I'm not sure we can have or want a uniform interface.) As mentioned elsewhere, the test time is likely a separate problem that we should investigate and fix. We shouldn't add an interface to work around that problem. If we *really* need the ability to modify these values for testing, hopefully there are other ways to set such values globally in Chromium. (Each test runs in a separate instance of the content shell.) As mentioned in https://codereview.chromium.org/1171263004/, I'm not sure a) we can have separate audio/video values in the .src= case or b) the MSE implementation should be garbage collecting streams separately. That would make this API, which separates audio and video, problematic. https://codereview.chromium.org/1182183004/diff/40001/public/platform/WebMedi... public/platform/WebMediaPlayer.h:151: virtual size_t getAudioBufferSize() const = 0; On 2015/06/17 21:39:44, servolk wrote: > On 2015/06/17 20:31:35, ddorwin wrote: > > I don't think these are "buffer" sizes. IMO, a buffer is a single entity. This > > is more a cache size or max memory usage, right? > > > > See also > > > https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/sour... > > The word "buffer" indeed has a different meaning for people that work on the > lower levels of media pipeline. But on blink level the word "buffer" means > "memory set aside for storing media data", as in "SourceBuffer". And this API > essentially allows you to set the memory limit for the corresponding > SourceBuffer. SourceBuffer is a very specific thing with a spec definition. I don't think that means that "buffer" should be assumed to be similar. There is too much ambiguity here. I think "audio buffer" is most likely to be interpreted as being related to the audio output device. AudioSourceBufferSize would be better. However, since this is likely MSE-specific, it should probably just be moved there. Also, this sets the SourceBuffer*Stream* limit, which I'm not sure is correct. In addition, since there can be multiple SourceBufferStreams, the actual memory usage of a given media element (or MediaSource), even just for a single stream type, could be much greater than this limit. That's not clear from this API. https://codereview.chromium.org/1182183004/diff/40001/public/platform/WebMedi... public/platform/WebMediaPlayer.h:152: virtual void setAudioBufferSize(size_t) = 0; On 2015/06/17 21:39:44, servolk wrote: > On 2015/06/17 20:31:35, ddorwin wrote: > > Does a per-player value make sense? Perhaps for Cast (one tab, usually one > video > > element), but what about other platforms where there might be more than one > > video per page or multiple tabs playing video or audio? > > > > Any assumptions made about the platform when implementing custom sizes may not > > make sense when there are two or more elements. > > I think per-player setting gives us maximum flexibility while still keeping API > reasonably simple. We could, of course, have a similar set of methods for > setting global defaults, but what's the point of that if we could have a more > flexible API with approximately the same code footprint? > If there's more than one video per page that's totally fine. In fact I think > this design (per-player as opposed to global setting) would help us. Since if > there are multiple video elements the browser can be smart and, for example, set > larger memory limits for currently playing/visible media element and reduce > memory limits for media elements that are in the invisible area of the web page, > in the paused state or in an inactive browser tab. Flexibility often means complexity and can easily become YAGNI. Let's not add APIs and complexity for which we don't have a real need. "The browser" is Chromium. Chromium should make these decisions, not Blink.
Based on the concerns here and in the implementation (https://codereview.chromium.org/1171263004/), not lgtm. |
