Allow setting memory limits through WebMediaPlayer interface (Blink)

public/platform/WebMediaPlayer.h

Index: public/platform/WebMediaPlayer.h
diff --git a/public/platform/WebMediaPlayer.h b/public/platform/WebMediaPlayer.h
index c18942a62fa8b5e4f2cb09fd4c8f45348bed0861..32fcc045dec156d70829a1a375ba870bf0c3da72 100644
--- a/public/platform/WebMediaPlayer.h
+++ b/public/platform/WebMediaPlayer.h
@@ -147,6 +147,12 @@ public:
     virtual unsigned audioDecodedByteCount() const = 0;
     virtual unsigned videoDecodedByteCount() const = 0;
 
+    // Getting/setting buffer sizes is currently only implemented for MSE.

[ddorwin, 2015/06/17 20:31:35]

Is there a CL showing how this will be used? Would it make sense to have these
methods on an MSE interface instead?

[servolk, 2015/06/17 21:39:44]

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.

[ddorwin, 2015/06/20 01:56:40]

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.

+    virtual size_t getAudioBufferSize() const = 0;

[ddorwin, 2015/06/17 20:31:35]

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...

[servolk, 2015/06/17 21:39:44]

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.

[ddorwin, 2015/06/20 01:56:40]

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.

+    virtual void setAudioBufferSize(size_t) = 0;

[ddorwin, 2015/06/17 20:31:35]

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.

[servolk, 2015/06/17 21:39:44]

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.

[ddorwin, 2015/06/20 01:56:40]

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.

+    virtual size_t getVideoBufferSize() const = 0;

[ddorwin, 2015/06/17 20:31:35]

"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.)

[servolk, 2015/06/17 21:39:44]

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.

+    virtual void setVideoBufferSize(size_t) = 0;
+
     virtual void paint(WebCanvas*, const WebRect&, unsigned char alpha, SkXfermode::Mode) = 0;
     // Do a GPU-GPU textures copy if possible.
     virtual bool copyVideoTextureToPlatformTexture(WebGraphicsContext3D*, unsigned texture, unsigned internalFormat, unsigned type, bool premultiplyAlpha, bool flipY) { return false; }