Allow setting memory limits on media::DemuxerStream

Allow setting memory limits on media::DemuxerStream

See crbug.com/498482 for details. In short: We need to allow setting
memory usage limits for demuxer streams via public DemuxerStream
interface. This would be useful for unit tests and low-memory platforms
that can't afford default memory limits.
I've removed the old ChunkDemuxer::SetMemoryLimits API and replaced it
with media::DemuxerStream::SetMemoryLimit, since it's trivial to obtain
DemuxerStream* via public media::Demuxer::GetStream interface.

BUG=498482

[servolk, 2015-06-10 00:27:44]

servolk@chromium.org changed reviewers:
+ chcunningham@chromium.org, wolenetz@chromium.org

[servolk, 2015-06-11 02:15:34]

servolk@chromium.org changed reviewers:
+ philipj@opera.com

[servolk, 2015-06-11 02:17:11]

On 2015/06/10 00:28:02, servolk wrote:

PTAL
I believe we should land this CL first, this will make memory limits on demuxer
streams settable through publicly accessible interfaces, instead of internal
media/ interfaces. Then we can start working on providing access to this
functionality from blink.

[philipj_slow, 2015-06-11 08:44:05]

Non-owner LGTM, but I only took a quick peek.

https://codereview.chromium.org/1171263004/diff/80001/media/base/demuxer_stre...
File media/base/demuxer_stream.h (right):

https://codereview.chromium.org/1171263004/diff/80001/media/base/demuxer_stre...
media/base/demuxer_stream.h:94: // Get the maximum amount of data that this
DemuxerStream is allowed to buffer
I suppose document the unit, bytes?

[chcunningham, 2015-06-11 18:56:55]

lgtm

https://codereview.chromium.org/1171263004/diff/80001/media/base/fake_text_tr...
File media/base/fake_text_track_stream.cc (right):

https://codereview.chromium.org/1171263004/diff/80001/media/base/fake_text_tr...
media/base/fake_text_track_stream.cc:48: NOTIMPLEMENTED();
We're having to do this same override boilerplate in a few places. I'm newish to
the team, so this may be the preferred style, but I notice we have some
"default" implementations in demuxer_stream.cc. WDYT about making this a default
there and removing the duplicate overrides? (Matt, please comment on whether
this suggestion is stylistically correct).

[wolenetz, 2015-06-12 00:03:47]

On 2015/06/11 18:56:55, chcunningham wrote:
> lgtm
> 
>
https://codereview.chromium.org/1171263004/diff/80001/media/base/fake_text_tr...
> File media/base/fake_text_track_stream.cc (right):
> 
>
https://codereview.chromium.org/1171263004/diff/80001/media/base/fake_text_tr...
> media/base/fake_text_track_stream.cc:48: NOTIMPLEMENTED();
> We're having to do this same override boilerplate in a few places. I'm newish
to
> the team, so this may be the preferred style, but I notice we have some
> "default" implementations in demuxer_stream.cc. WDYT about making this a
default
> there and removing the duplicate overrides? (Matt, please comment on whether
> this suggestion is stylistically correct).

I'll get to this CR early tomorrow morning. I was in training half of today.

[servolk, 2015-06-12 00:50:48]

https://codereview.chromium.org/1171263004/diff/80001/media/base/demuxer_stre...
File media/base/demuxer_stream.h (right):

https://codereview.chromium.org/1171263004/diff/80001/media/base/demuxer_stre...
media/base/demuxer_stream.h:94: // Get the maximum amount of data that this
DemuxerStream is allowed to buffer
On 2015/06/11 08:44:05, philipj wrote:
> I suppose document the unit, bytes?

Done.

https://codereview.chromium.org/1171263004/diff/80001/media/base/fake_text_tr...
File media/base/fake_text_track_stream.cc (right):

https://codereview.chromium.org/1171263004/diff/80001/media/base/fake_text_tr...
media/base/fake_text_track_stream.cc:48: NOTIMPLEMENTED();
On 2015/06/11 18:56:55, chcunningham wrote:
> We're having to do this same override boilerplate in a few places. I'm newish
to
> the team, so this may be the preferred style, but I notice we have some
> "default" implementations in demuxer_stream.cc. WDYT about making this a
default
> there and removing the duplicate overrides? (Matt, please comment on whether
> this suggestion is stylistically correct).

Yeah, that's a good point. I was pondering this myself. I went with pure virtual
function initially to make it easier for me to catch all the places that might
need to be updated (and so I've added a proper implementation of
Get/SetMemoryLimit for DecryptingDemuxerStream and left a TODO in Mojo code),
but yeah, there are a lot of media::DemuxerStream implementations and some of
them (e.g. all the test/fake/mock implementation) don't care about this. So we
could probably go with a default definition in demuxer_stream.cc for now, we'll
just have to remember to implement this for any actual DemuxerStream
implementations as necessary.

[wolenetz, 2015-06-12 21:31:57]

https://codereview.chromium.org/1171263004/diff/80001/media/base/fake_text_tr...
File media/base/fake_text_track_stream.cc (right):

https://codereview.chromium.org/1171263004/diff/80001/media/base/fake_text_tr...
media/base/fake_text_track_stream.cc:48: NOTIMPLEMENTED();
On 2015/06/12 00:50:48, servolk wrote:
> On 2015/06/11 18:56:55, chcunningham wrote:
> > We're having to do this same override boilerplate in a few places. I'm
newish
> to
> > the team, so this may be the preferred style, but I notice we have some
> > "default" implementations in demuxer_stream.cc. WDYT about making this a
> default
> > there and removing the duplicate overrides? (Matt, please comment on whether
> > this suggestion is stylistically correct).
> 
> Yeah, that's a good point. I was pondering this myself. I went with pure
virtual
> function initially to make it easier for me to catch all the places that might
> need to be updated (and so I've added a proper implementation of
> Get/SetMemoryLimit for DecryptingDemuxerStream and left a TODO in Mojo code),
> but yeah, there are a lot of media::DemuxerStream implementations and some of
> them (e.g. all the test/fake/mock implementation) don't care about this. So we
> could probably go with a default definition in demuxer_stream.cc for now,
we'll
> just have to remember to implement this for any actual DemuxerStream
> implementations as necessary.

I was pondering similarly.
Backing up a bit, ISTM the goal is that eventually all real DemuxerStreams
should have memory buffer limit control that is meaningful.
To discover any new DemuxerStream implementations that might crop-up over time,
ISTM that pure-virtual, non-default in DemuxerStream, with explicit need for
overrides (with TODOs for those that explicitly require implementation) in each
DemuxerStream implementation, while somewhat boilerplate, will ensure the build
won't work for some variant that might use a specialized DemuxerStream that
hasn't been discovered in this CL (prevents dynamic fragility). Test mocks will
need the boilerplate, but IMO, that's a necessary complexity that is warranted
by catching unexpected lack of implementations that might crop up.

https://codereview.chromium.org/1171263004/diff/120001/media/base/demuxer_str...
File media/base/demuxer_stream.h (right):

https://codereview.chromium.org/1171263004/diff/120001/media/base/demuxer_str...
media/base/demuxer_stream.h:94: // Get the data buffer size, in bytes.
nit: s/size/size limit/

https://codereview.chromium.org/1171263004/diff/120001/media/base/demuxer_str...
media/base/demuxer_stream.h:95: virtual int GetMemoryLimit() const;
nit: signed int: is a range including negative values useful for this interface?

https://codereview.chromium.org/1171263004/diff/120001/media/base/demuxer_str...
media/base/demuxer_stream.h:97: // Set the data buffer size, in bytes.
nit ditto (limit)

https://codereview.chromium.org/1171263004/diff/120001/media/base/demuxer_str...
media/base/demuxer_stream.h:98: virtual void SetMemoryLimit(int memory_limit);
nit ditto (int ?)

Also, is a DemuxerStream valid if SetMemoryLimit() has never been called? If
not, should there be some default or ctor arg?
IOW, if the goal is to eventually have memory limit specified and observed for
all real DemuxerStream implementations, would some default not be known at ctor
time? If so, making such default a base class DemuxerStream ctor arg seems good
to me. Somewhat of a nit, not too strong a feeling on this one. WDYT?

[servolk, 2015-06-16 01:30:52]

Ok, I've changed the DemuxerStream interface definitions back to pure virtual
and changed the memory limit type to size_t instead of int. PTAL.

https://codereview.chromium.org/1171263004/diff/120001/media/base/demuxer_str...
File media/base/demuxer_stream.h (right):

https://codereview.chromium.org/1171263004/diff/120001/media/base/demuxer_str...
media/base/demuxer_stream.h:94: // Get the data buffer size, in bytes.
On 2015/06/12 21:31:57, wolenetz wrote:
> nit: s/size/size limit/

Done.

https://codereview.chromium.org/1171263004/diff/120001/media/base/demuxer_str...
media/base/demuxer_stream.h:95: virtual int GetMemoryLimit() const;
On 2015/06/12 21:31:56, wolenetz wrote:
> nit: signed int: is a range including negative values useful for this
interface?

The best type for this is probably size_t, but the old
ChunkDemuxer::SetMemoryLimit API used ints for some reason. I assumed it was for
easier interoperability with blink and/or JS or perhaps negative values were
used to indicate something (seems not to be the case, but I didn't know that
when I started working on this CL).
I'll change the type to size_t and see if any issues pop up.

https://codereview.chromium.org/1171263004/diff/120001/media/base/demuxer_str...
media/base/demuxer_stream.h:97: // Set the data buffer size, in bytes.
On 2015/06/12 21:31:56, wolenetz wrote:
> nit ditto (limit)

Done.

https://codereview.chromium.org/1171263004/diff/120001/media/base/demuxer_str...
media/base/demuxer_stream.h:98: virtual void SetMemoryLimit(int memory_limit);
On 2015/06/12 21:31:56, wolenetz wrote:
> nit ditto (int ?)
> 
> Also, is a DemuxerStream valid if SetMemoryLimit() has never been called? If
> not, should there be some default or ctor arg?
> IOW, if the goal is to eventually have memory limit specified and observed for
> all real DemuxerStream implementations, would some default not be known at
ctor
> time? If so, making such default a base class DemuxerStream ctor arg seems
good
> to me. Somewhat of a nit, not too strong a feeling on this one. WDYT?

Yes, there are reasonable defaults for memory limits
(kSourceBufferAudioMemoryLimit=12Mb/kSourceBufferVideoMemoryLimit=150Mb). So we
can get away without having to pass those in the constructor, having default
values is convenient and doesn't limit our flexibility, since we now provide a
mechanism to change those defaults.

[servolk, 2015-06-16 22:59:06]

On 2015/06/16 01:30:52, servolk wrote:
> Ok, I've changed the DemuxerStream interface definitions back to pure virtual
> and changed the memory limit type to size_t instead of int. PTAL.
> 
>
https://codereview.chromium.org/1171263004/diff/120001/media/base/demuxer_str...
> File media/base/demuxer_stream.h (right):
> 
>
https://codereview.chromium.org/1171263004/diff/120001/media/base/demuxer_str...
> media/base/demuxer_stream.h:94: // Get the data buffer size, in bytes.
> On 2015/06/12 21:31:57, wolenetz wrote:
> > nit: s/size/size limit/
> 
> Done.
> 
>
https://codereview.chromium.org/1171263004/diff/120001/media/base/demuxer_str...
> media/base/demuxer_stream.h:95: virtual int GetMemoryLimit() const;
> On 2015/06/12 21:31:56, wolenetz wrote:
> > nit: signed int: is a range including negative values useful for this
> interface?
> 
> The best type for this is probably size_t, but the old
> ChunkDemuxer::SetMemoryLimit API used ints for some reason. I assumed it was
for
> easier interoperability with blink and/or JS or perhaps negative values were
> used to indicate something (seems not to be the case, but I didn't know that
> when I started working on this CL).
> I'll change the type to size_t and see if any issues pop up.
> 
>
https://codereview.chromium.org/1171263004/diff/120001/media/base/demuxer_str...
> media/base/demuxer_stream.h:97: // Set the data buffer size, in bytes.
> On 2015/06/12 21:31:56, wolenetz wrote:
> > nit ditto (limit)
> 
> Done.
> 
>
https://codereview.chromium.org/1171263004/diff/120001/media/base/demuxer_str...
> media/base/demuxer_stream.h:98: virtual void SetMemoryLimit(int memory_limit);
> On 2015/06/12 21:31:56, wolenetz wrote:
> > nit ditto (int ?)
> > 
> > Also, is a DemuxerStream valid if SetMemoryLimit() has never been called? If
> > not, should there be some default or ctor arg?
> > IOW, if the goal is to eventually have memory limit specified and observed
for
> > all real DemuxerStream implementations, would some default not be known at
> ctor
> > time? If so, making such default a base class DemuxerStream ctor arg seems
> good
> > to me. Somewhat of a nit, not too strong a feeling on this one. WDYT?
> 
> Yes, there are reasonable defaults for memory limits
> (kSourceBufferAudioMemoryLimit=12Mb/kSourceBufferVideoMemoryLimit=150Mb). So
we
> can get away without having to pass those in the constructor, having default
> values is convenient and doesn't limit our flexibility, since we now provide a
> mechanism to change those defaults.

ping, ptal

[wolenetz, 2015-06-16 23:03:06]

On 2015/06/16 22:59:06, servolk wrote:
> On 2015/06/16 01:30:52, servolk wrote:
> > Ok, I've changed the DemuxerStream interface definitions back to pure
virtual
> > and changed the memory limit type to size_t instead of int. PTAL.
> > 
> >
>
https://codereview.chromium.org/1171263004/diff/120001/media/base/demuxer_str...
> > File media/base/demuxer_stream.h (right):
> > 
> >
>
https://codereview.chromium.org/1171263004/diff/120001/media/base/demuxer_str...
> > media/base/demuxer_stream.h:94: // Get the data buffer size, in bytes.
> > On 2015/06/12 21:31:57, wolenetz wrote:
> > > nit: s/size/size limit/
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1171263004/diff/120001/media/base/demuxer_str...
> > media/base/demuxer_stream.h:95: virtual int GetMemoryLimit() const;
> > On 2015/06/12 21:31:56, wolenetz wrote:
> > > nit: signed int: is a range including negative values useful for this
> > interface?
> > 
> > The best type for this is probably size_t, but the old
> > ChunkDemuxer::SetMemoryLimit API used ints for some reason. I assumed it was
> for
> > easier interoperability with blink and/or JS or perhaps negative values were
> > used to indicate something (seems not to be the case, but I didn't know that
> > when I started working on this CL).
> > I'll change the type to size_t and see if any issues pop up.
> > 
> >
>
https://codereview.chromium.org/1171263004/diff/120001/media/base/demuxer_str...
> > media/base/demuxer_stream.h:97: // Set the data buffer size, in bytes.
> > On 2015/06/12 21:31:56, wolenetz wrote:
> > > nit ditto (limit)
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1171263004/diff/120001/media/base/demuxer_str...
> > media/base/demuxer_stream.h:98: virtual void SetMemoryLimit(int
memory_limit);
> > On 2015/06/12 21:31:56, wolenetz wrote:
> > > nit ditto (int ?)
> > > 
> > > Also, is a DemuxerStream valid if SetMemoryLimit() has never been called?
If
> > > not, should there be some default or ctor arg?
> > > IOW, if the goal is to eventually have memory limit specified and observed
> for
> > > all real DemuxerStream implementations, would some default not be known at
> > ctor
> > > time? If so, making such default a base class DemuxerStream ctor arg seems
> > good
> > > to me. Somewhat of a nit, not too strong a feeling on this one. WDYT?
> > 
> > Yes, there are reasonable defaults for memory limits
> > (kSourceBufferAudioMemoryLimit=12Mb/kSourceBufferVideoMemoryLimit=150Mb). So
> we
> > can get away without having to pass those in the constructor, having default
> > values is convenient and doesn't limit our flexibility, since we now provide
a
> > mechanism to change those defaults.
> 
> ping, ptal

Looking now :)

[wolenetz, 2015-06-16 23:17:17]

lgtm % nits. It looks like OWNER reviewer for //chromecast is missing, though.

https://codereview.chromium.org/1171263004/diff/200001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/1171263004/diff/200001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:486: // currently is has a hard-coded limit of
150Mb across all streams (see
nit: s/is/it/

https://codereview.chromium.org/1171263004/diff/200001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:496: // currently is has a hard-coded limit of
150Mb across all streams (see
nit ditto

[servolk, 2015-06-16 23:22:02]

servolk@chromium.org changed reviewers:
+ gunsch@chromium.org, lcwu@chromium.org

[servolk, 2015-06-16 23:23:06]

On 2015/06/16 23:17:17, wolenetz wrote:
> lgtm % nits. It looks like OWNER reviewer for //chromecast is missing, though.
> 
>
https://codereview.chromium.org/1171263004/diff/200001/media/filters/ffmpeg_d...
> File media/filters/ffmpeg_demuxer.cc (right):
> 
>
https://codereview.chromium.org/1171263004/diff/200001/media/filters/ffmpeg_d...
> media/filters/ffmpeg_demuxer.cc:486: // currently is has a hard-coded limit of
> 150Mb across all streams (see
> nit: s/is/it/
> 
>
https://codereview.chromium.org/1171263004/diff/200001/media/filters/ffmpeg_d...
> media/filters/ffmpeg_demuxer.cc:496: // currently is has a hard-coded limit of
> 150Mb across all streams (see
> nit ditto

Thanks Matthew!
+lcwu@,gunsch@ for chromecast/

[servolk, 2015-06-16 23:27:07]

https://codereview.chromium.org/1171263004/diff/200001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/1171263004/diff/200001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:486: // currently is has a hard-coded limit of
150Mb across all streams (see
On 2015/06/16 23:17:17, wolenetz wrote:
> nit: s/is/it/

Done.

https://codereview.chromium.org/1171263004/diff/200001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:496: // currently is has a hard-coded limit of
150Mb across all streams (see
On 2015/06/16 23:17:16, wolenetz wrote:
> nit ditto

Done.

[lcwu1, 2015-06-17 01:32:55]

lgtm

[servolk, 2015-06-17 01:38:21]

The CQ bit was checked by servolk@chromium.org

[servolk, 2015-06-17 01:38:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
chcunningham@chromium.org, philipj@opera.com, wolenetz@chromium.org
Link to the patchset: https://codereview.chromium.org/1171263004/#ps220001
(title: "fixed typo")

[commit-bot: I haz the power, 2015-06-17 01:38:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1171263004/220001

[commit-bot: I haz the power, 2015-06-17 03:29:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-17 03:29:02]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ddorwin, 2015-06-17 21:02:15]

ddorwin@chromium.org changed reviewers:
+ ddorwin@chromium.org

[ddorwin, 2015-06-17 21:02:17]

I would rather that this not be committed until we can work through some of the
issues in these various CLs.

https://codereview.chromium.org/1171263004/diff/220001/media/base/demuxer_str...
File media/base/demuxer_stream.h (right):

https://codereview.chromium.org/1171263004/diff/220001/media/base/demuxer_str...
media/base/demuxer_stream.h:94: // Get the data buffer size limit, in bytes.
Similar to my comments in https://codereview.chromium.org/1182183004/, I don't
think "buffer" is correct.

https://codereview.chromium.org/1171263004/diff/220001/media/base/demuxer_str...
media/base/demuxer_stream.h:95: virtual size_t GetMemoryLimit() const = 0;
Is this ever called? I don't see it being used here. SourceBufferStream is the
only case that needs it, and it is NOT a DemuxerStream. Maybe it is being called
by the new code in https://codereview.chromium.org/1185453004/, but I'm not
convinced that design is desirable.

Should this really be on a demuxer _stream_ rather than the Demuxer? What
behavior would you expect if there were multiple audio streams?

Why does DemuxerStream need to implement this? Having to override this with
NOTIMPLEMENTED in so many places is a sign of a potential design issue. Perhaps
there is another way that would make more sense. Maybe MediaClient.

https://codereview.chromium.org/1171263004/diff/220001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/1171263004/diff/220001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:491: return 0;
What is going to happen when Blink starts using this API?

https://codereview.chromium.org/1171263004/diff/220001/media/filters/source_b...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/1171263004/diff/220001/media/filters/source_b...
media/filters/source_buffer_stream.cc:595: void
SourceBufferStream::GarbageCollectIfNeeded() {
This implementation detail (limits and GC tied to a specific stream) is what
leads to the current methods for sizes. Is that dictated by something like the
spec or just the current implementation? If the latter, is it necessarily the
best or permanent solution?

https://codereview.chromium.org/1171263004/diff/220001/media/filters/source_b...
File media/filters/source_buffer_stream.h (right):

https://codereview.chromium.org/1171263004/diff/220001/media/filters/source_b...
media/filters/source_buffer_stream.h:145: size_t GetMemoryLimit() const {
Memory limit for what? Maximum source buffer size? That's what's currently
implied, but I don't think that's accurate. It appears to be the maximum byte
range cache size or something like that.

[wolenetz, 2015-06-17 21:11:47]

On 2015/06/17 21:02:17, ddorwin wrote:
> I would rather that this not be committed until we can work through some of
the
> issues in these various CLs.
> 
>
https://codereview.chromium.org/1171263004/diff/220001/media/base/demuxer_str...
> File media/base/demuxer_stream.h (right):
> 
>
https://codereview.chromium.org/1171263004/diff/220001/media/base/demuxer_str...
> media/base/demuxer_stream.h:94: // Get the data buffer size limit, in bytes.
> Similar to my comments in https://codereview.chromium.org/1182183004/, I don't
> think "buffer" is correct.
> 
>
https://codereview.chromium.org/1171263004/diff/220001/media/base/demuxer_str...
> media/base/demuxer_stream.h:95: virtual size_t GetMemoryLimit() const = 0;
> Is this ever called? I don't see it being used here. SourceBufferStream is the
> only case that needs it, and it is NOT a DemuxerStream. Maybe it is being
called
> by the new code in https://codereview.chromium.org/1185453004/, but I'm not
> convinced that design is desirable.
> 
> Should this really be on a demuxer _stream_ rather than the Demuxer? What
> behavior would you expect if there were multiple audio streams?
> 
> Why does DemuxerStream need to implement this? Having to override this with
> NOTIMPLEMENTED in so many places is a sign of a potential design issue.
Perhaps
> there is another way that would make more sense. Maybe MediaClient.
> 
>
https://codereview.chromium.org/1171263004/diff/220001/media/filters/ffmpeg_d...
> File media/filters/ffmpeg_demuxer.cc (right):
> 
>
https://codereview.chromium.org/1171263004/diff/220001/media/filters/ffmpeg_d...
> media/filters/ffmpeg_demuxer.cc:491: return 0;
> What is going to happen when Blink starts using this API?
> 
>
https://codereview.chromium.org/1171263004/diff/220001/media/filters/source_b...
> File media/filters/source_buffer_stream.cc (right):
> 
>
https://codereview.chromium.org/1171263004/diff/220001/media/filters/source_b...
> media/filters/source_buffer_stream.cc:595: void
> SourceBufferStream::GarbageCollectIfNeeded() {
> This implementation detail (limits and GC tied to a specific stream) is what
> leads to the current methods for sizes. Is that dictated by something like the
> spec or just the current implementation? If the latter, is it necessarily the
> best or permanent solution?
> 
>
https://codereview.chromium.org/1171263004/diff/220001/media/filters/source_b...
> File media/filters/source_buffer_stream.h (right):
> 
>
https://codereview.chromium.org/1171263004/diff/220001/media/filters/source_b...
> media/filters/source_buffer_stream.h:145: size_t GetMemoryLimit() const {
> Memory limit for what? Maximum source buffer size? That's what's currently
> implied, but I don't think that's accurate. It appears to be the maximum byte
> range cache size or something like that.

From discussion w/ddorwin@, I now have similar concerns. I'll reverse my l*tm
for now.
not lgtm

[wolenetz, 2015-06-17 21:20:41]

On 2015/06/17 21:11:47, wolenetz wrote:
> On 2015/06/17 21:02:17, ddorwin wrote:
> > I would rather that this not be committed until we can work through some of
> the
> > issues in these various CLs.
> > 
> >
>
https://codereview.chromium.org/1171263004/diff/220001/media/base/demuxer_str...
> > File media/base/demuxer_stream.h (right):
> > 
> >
>
https://codereview.chromium.org/1171263004/diff/220001/media/base/demuxer_str...
> > media/base/demuxer_stream.h:94: // Get the data buffer size limit, in bytes.
> > Similar to my comments in https://codereview.chromium.org/1182183004/, I
don't
> > think "buffer" is correct.
> > 
> >
>
https://codereview.chromium.org/1171263004/diff/220001/media/base/demuxer_str...
> > media/base/demuxer_stream.h:95: virtual size_t GetMemoryLimit() const = 0;
> > Is this ever called? I don't see it being used here. SourceBufferStream is
the
> > only case that needs it, and it is NOT a DemuxerStream. Maybe it is being
> called
> > by the new code in https://codereview.chromium.org/1185453004/, but I'm not
> > convinced that design is desirable.
> > 
> > Should this really be on a demuxer _stream_ rather than the Demuxer? What
> > behavior would you expect if there were multiple audio streams?
> > 
> > Why does DemuxerStream need to implement this? Having to override this with
> > NOTIMPLEMENTED in so many places is a sign of a potential design issue.
> Perhaps
> > there is another way that would make more sense. Maybe MediaClient.
> > 
> >
>
https://codereview.chromium.org/1171263004/diff/220001/media/filters/ffmpeg_d...
> > File media/filters/ffmpeg_demuxer.cc (right):
> > 
> >
>
https://codereview.chromium.org/1171263004/diff/220001/media/filters/ffmpeg_d...
> > media/filters/ffmpeg_demuxer.cc:491: return 0;
> > What is going to happen when Blink starts using this API?
> > 
> >
>
https://codereview.chromium.org/1171263004/diff/220001/media/filters/source_b...
> > File media/filters/source_buffer_stream.cc (right):
> > 
> >
>
https://codereview.chromium.org/1171263004/diff/220001/media/filters/source_b...
> > media/filters/source_buffer_stream.cc:595: void
> > SourceBufferStream::GarbageCollectIfNeeded() {
> > This implementation detail (limits and GC tied to a specific stream) is what
> > leads to the current methods for sizes. Is that dictated by something like
the
> > spec or just the current implementation? If the latter, is it necessarily
the
> > best or permanent solution?
> > 
> >
>
https://codereview.chromium.org/1171263004/diff/220001/media/filters/source_b...
> > File media/filters/source_buffer_stream.h (right):
> > 
> >
>
https://codereview.chromium.org/1171263004/diff/220001/media/filters/source_b...
> > media/filters/source_buffer_stream.h:145: size_t GetMemoryLimit() const {
> > Memory limit for what? Maximum source buffer size? That's what's currently
> > implied, but I don't think that's accurate. It appears to be the maximum
byte
> > range cache size or something like that.
> 
> From discussion w/ddorwin@, I now have similar concerns. I'll reverse my l*tm
> for now.
> not lgtm

To add some questions:
Does the existing platform-specific default limit code not provide the
flexibility needed for other low-memory platforms to customize the limits
without this API change?
Also, does Blink really need this (likely to be test-only) API, given that the
GC layout test (see https://codereview.chromium.org/1013923002/#msg12) might
just need some investigation to see why it's taking so long, rather than
forcibly using a test-only API to reduce the limit (and layout test timeout)?
From the FFmpegDemuxerStream TODOs introduced in this CL, it looks like there is
some possibility of needing some limit-get/set API for non-MSE demuxers, but
this CL doesn't clarify that need. Multiple default (or concrete
NOTIMPLEMENTED() override of pure virtual) of the set/get API in all but one of
the DemuxerStream implementations does indeed seem wrong at this time at least.

[servolk, 2015-06-17 22:13:48]

https://codereview.chromium.org/1171263004/diff/220001/media/base/demuxer_str...
File media/base/demuxer_stream.h (right):

https://codereview.chromium.org/1171263004/diff/220001/media/base/demuxer_str...
media/base/demuxer_stream.h:94: // Get the data buffer size limit, in bytes.
On 2015/06/17 21:02:16, ddorwin wrote:
> Similar to my comments in https://codereview.chromium.org/1182183004/, I don't
> think "buffer" is correct.

See the comment in that CL. It's a "buffer" that has the same meaning as in "MSE
SourceBuffer", which is different from e.g. media::DecoderBuffer. AFAIK both
uses of this word are pretty common.

https://codereview.chromium.org/1171263004/diff/220001/media/base/demuxer_str...
media/base/demuxer_stream.h:95: virtual size_t GetMemoryLimit() const = 0;
On 2015/06/17 21:02:16, ddorwin wrote:
> Is this ever called? I don't see it being used here. SourceBufferStream is the
> only case that needs it, and it is NOT a DemuxerStream. Maybe it is being
called
> by the new code in https://codereview.chromium.org/1185453004/, but I'm not
> convinced that design is desirable.
> 
> Should this really be on a demuxer _stream_ rather than the Demuxer? What
> behavior would you expect if there were multiple audio streams?
> 
> Why does DemuxerStream need to implement this? Having to override this with
> NOTIMPLEMENTED in so many places is a sign of a potential design issue.
Perhaps
> there is another way that would make more sense. Maybe MediaClient.

It's currently called in the unit tests (see the changes in
media/filters/chunk_demuxer_unittest.cc in this CL). I'm also planning to call
this from inside CmaRenderer in Chromecast code for adjust memory limits
according to the amount of memory available on the current platform.
Coincidentally that's the reason why I'd prefer this API to be on DemuxerStream,
instead of media::Demuxer. When a renderer is initialized it is given only
media::DemuxerStreamProvider (see media::Renderer::Initialize). So renderers can
easily access DemuxerStreams, but not the full media::Demuxer interface. On
Chromecast we provide a custom renderer, but the use the rest of the media
pipeline as provided by the standard Chromium implementation and we want to be
able to use these APIs.

Re NOTIMPLEMENTED - take a closer look, most of those NOTIMPLs are in mock/fake
objects being used to unit tests. The only two NOTIMPLs that are in actual
implementations are in FFmpegDemuxerStream and in MojoDemuxerStreamAdapter.
These will need to be implemented eventually (and I have some ideas how to do
this for FFmpeg, less clear on how to do this for Mojo).

https://codereview.chromium.org/1171263004/diff/220001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/1171263004/diff/220001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:491: return 0;
On 2015/06/17 21:02:16, ddorwin wrote:
> What is going to happen when Blink starts using this API?

For now we are planning to use this API from Blink level only for unit tests, so
it should be fine.

https://codereview.chromium.org/1171263004/diff/220001/media/filters/source_b...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/1171263004/diff/220001/media/filters/source_b...
media/filters/source_buffer_stream.cc:595: void
SourceBufferStream::GarbageCollectIfNeeded() {
On 2015/06/17 21:02:16, ddorwin wrote:
> This implementation detail (limits and GC tied to a specific stream) is what
> leads to the current methods for sizes. Is that dictated by something like the
> spec or just the current implementation? If the latter, is it necessarily the
> best or permanent solution?

Per MSE spec
(https://w3c.github.io/media-source/#sourcebuffer-coded-frame-eviction) GC
should be done on SourceBuffer level.
I think the question shouldn't be "is it the best or permanent solution". The
question should be "is it better than what we currently have". And the answer to
the latter is "yes, definitely". There are many issues with the current
implementation of GC in MSE (see crbug.com/440173, crbug.com/474806,
crbug.com/421694, etc) and I'm trying to make some steps to improve that.
One step is allowing better control over MSE sizes, so that we can increase
buffer limits for platforms that can afford that and that will help with
triggering GC-related errors less often to our end-users. Another step is MSE
algorithm improvements (e.g. CL https://codereview.chromium.org/1008463002/).
And I hope that through a series of steps like that we will arrive at some
better solution than the current "permanent" solution.

https://codereview.chromium.org/1171263004/diff/220001/media/filters/source_b...
File media/filters/source_buffer_stream.h (right):

https://codereview.chromium.org/1171263004/diff/220001/media/filters/source_b...
media/filters/source_buffer_stream.h:145: size_t GetMemoryLimit() const {
On 2015/06/17 21:02:16, ddorwin wrote:
> Memory limit for what? Maximum source buffer size? That's what's currently
> implied, but I don't think that's accurate. It appears to be the maximum byte
> range cache size or something like that.

Yeah, I guess you can call it "maximum byte cache range size". Note that the
terminology here (the "memory limit") wasn't introduced by me, it was introduced
long time ago by somebody else in a different CL. I'm just renaming the method
to make it's name more consistent with what I'm adding in media::DemuxerStream
interface in this CL. I can rename it to something else, if you prefer, but I
though the old terminology was fine and reasonably understandable.

[ddorwin, 2015-06-20 01:50:26]

https://codereview.chromium.org/1171263004/diff/220001/media/base/demuxer_str...
File media/base/demuxer_stream.h (right):

https://codereview.chromium.org/1171263004/diff/220001/media/base/demuxer_str...
media/base/demuxer_stream.h:94: // Get the data buffer size limit, in bytes.
On 2015/06/17 22:13:48, servolk wrote:
> On 2015/06/17 21:02:16, ddorwin wrote:
> > Similar to my comments in https://codereview.chromium.org/1182183004/, I
don't
> > think "buffer" is correct.
> 
> See the comment in that CL. It's a "buffer" that has the same meaning as in
"MSE
> SourceBuffer", which is different from e.g. media::DecoderBuffer. AFAIK both
> uses of this word are pretty common.

Those are two examples of qualified uses of "buffer"; you have selected one of
them as the meaning of an unqualified use of "buffer." SourceBuffer is actually
the less common of the two since it's just one mode of the media pipeline.
"buffer" is used  in the other sense in many more cases in media/.

I replied in that CL well.

https://codereview.chromium.org/1171263004/diff/220001/media/base/demuxer_str...
media/base/demuxer_stream.h:95: virtual size_t GetMemoryLimit() const = 0;
On 2015/06/17 22:13:48, servolk wrote:
> On 2015/06/17 21:02:16, ddorwin wrote:
> > Is this ever called? I don't see it being used here. SourceBufferStream is
the
> > only case that needs it, and it is NOT a DemuxerStream. Maybe it is being
> called
> > by the new code in https://codereview.chromium.org/1185453004/, but I'm not
> > convinced that design is desirable.
> > 
> > Should this really be on a demuxer _stream_ rather than the Demuxer? What
> > behavior would you expect if there were multiple audio streams?
> > 
> > Why does DemuxerStream need to implement this? Having to override this with
> > NOTIMPLEMENTED in so many places is a sign of a potential design issue.
> Perhaps
> > there is another way that would make more sense. Maybe MediaClient.
> 
> It's currently called in the unit tests (see the changes in
> media/filters/chunk_demuxer_unittest.cc in this CL). I'm also planning to call
> this from inside CmaRenderer in Chromecast code for adjust memory limits
> according to the amount of memory available on the current platform.

Is that the amount of memory _currently_ available or the physical device
characteristics? IOW, will the limits change during runtime or just be set
per-device? If the former, will they depending on how many DemuxerStreams are
active? If the latter, asking MediaClient for an override might be a much
simpler solution.

https://codereview.chromium.org/1171263004/diff/220001/media/filters/ffmpeg_d...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/1171263004/diff/220001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:484: size_t
FFmpegDemuxerStream::GetMemoryLimit() const {
Moving part of the discussion from
https://codereview.chromium.org/1182183004/diff/40001/public/platform/WebMedi...
here because it's not relevant to the interface but should be discussed

On 2015/06/17 21:39:44, servolk wrote:
...
> 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).

I'm curious how similar the memory usage is for these two types of playback and
whether it makes sense to have a uniform interface. In the MSE case, the
application downloaded data and provided it to MSE as SourceBuffers that
Chromium manages. In the .src=, Chromium decides how much data to download. It
could be in the network cache (out of the DemuxerStream's control),
BufferedDataSource, or DemuxerStream. Do you know where the bulk of the memory
usage actually is or whether it varies during playback?

Even if most memory is post-demux, that could theoretically change in the future
or across platforms and may not be something we want to expose via DemuxerStream
or to Blink.

Note that since .src= content is always muxed, separate limits for each stream
[type] (as in https://codereview.chromium.org/1182183004/) would only affect the
post-demux memory usage.

In both cases, I think it is simpler (and likely more effective) to manage each
case directly (i.e. in ChunkDemuxer and BufferedDataSource).

https://codereview.chromium.org/1171263004/diff/220001/media/filters/ffmpeg_d...
media/filters/ffmpeg_demuxer.cc:491: return 0;
On 2015/06/17 22:13:48, servolk wrote:
> On 2015/06/17 21:02:16, ddorwin wrote:
> > What is going to happen when Blink starts using this API?
> 
> For now we are planning to use this API from Blink level only for unit tests,
so
> it should be fine.

I assume you mean layout tests?
There is nothing to prevent other code from relying on this API once you expose
it, especially over to Blink.

If you really don't expect this to be called, you need to ensure this (i.e. it
is only set via the "internal" APIs exposed for layout tests) and use
NOTREACHED() to catch bugs that expose it in other ways.

https://codereview.chromium.org/1171263004/diff/220001/media/filters/source_b...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/1171263004/diff/220001/media/filters/source_b...
media/filters/source_buffer_stream.cc:595: void
SourceBufferStream::GarbageCollectIfNeeded() {
On 2015/06/17 22:13:48, servolk wrote:
> On 2015/06/17 21:02:16, ddorwin wrote:
> > This implementation detail (limits and GC tied to a specific stream) is what
> > leads to the current methods for sizes. Is that dictated by something like
the
> > spec or just the current implementation? If the latter, is it necessarily
the
> > best or permanent solution?
> 
> Per MSE spec
> (https://w3c.github.io/media-source/#sourcebuffer-coded-frame-eviction) GC
> should be done on SourceBuffer level.

Right, and a SourceBuffer can have multiple streams (tracks), right? If so, that
would seem to mean eviction at this level is incorrect. That makes sense - why
would you evict different ranges of audio and video? (I'm not familiar with the
MSE details and could be wrong - I defer to wolenetz on the details of the MSE
spec and implementation.)

If I am right, limits should not be tied to streams for MSE and thus should not
be tied to DemuxerStreams.

> I think the question shouldn't be "is it the best or permanent solution". The
> question should be "is it better than what we currently have". And the answer
to
> the latter is "yes, definitely". There are many issues with the current
> implementation of GC in MSE (see crbug.com/440173, crbug.com/474806,
> crbug.com/421694, etc) and I'm trying to make some steps to improve that.
> One step is allowing better control over MSE sizes, so that we can increase
> buffer limits for platforms that can afford that and that will help with
> triggering GC-related errors less often to our end-users. Another step is MSE
> algorithm improvements (e.g. CL https://codereview.chromium.org/1008463002/).
> And I hope that through a series of steps like that we will arrive at some
> better solution than the current "permanent" solution.

https://codereview.chromium.org/1171263004/diff/220001/media/filters/source_b...
File media/filters/source_buffer_stream.h (right):

https://codereview.chromium.org/1171263004/diff/220001/media/filters/source_b...
media/filters/source_buffer_stream.h:145: size_t GetMemoryLimit() const {
On 2015/06/17 22:13:48, servolk wrote:
> On 2015/06/17 21:02:16, ddorwin wrote:
> > Memory limit for what? Maximum source buffer size? That's what's currently
> > implied, but I don't think that's accurate. It appears to be the maximum
byte
> > range cache size or something like that.
> 
> Yeah, I guess you can call it "maximum byte cache range size". Note that the
> terminology here (the "memory limit") wasn't introduced by me, it was
introduced
> long time ago by somebody else in a different CL. I'm just renaming the method
> to make it's name more consistent with what I'm adding in media::DemuxerStream
> interface in this CL. I can rename it to something else, if you prefer, but I
> though the old terminology was fine and reasonably understandable.

Previously, the name followed the accessor style. Why did you change that?
https://www.chromium.org/developers/coding-style#TOC-Inline-functions

If not following that style, we should definitely be clearer. Even as an
accessor, we might consider updating the naming, though possibly in a separate
CL since we'd need to change all uses of this member.

[ddorwin, 2015-06-20 01:58:26]

The more I think about this and https://codereview.chromium.org/1182183004/, the
more concerned I am about this approach. I think we need to step back and think
about what it is you are trying to accomplish (presumably limiting total memory
usage) and how best to solve that (across SourceBuffers, streams, media
elements, etc.).

[chcunningham, 2016-03-23 04:21:47]

On 2015/06/20 01:58:26, ddorwin wrote:
> The more I think about this and https://codereview.chromium.org/1182183004/,
the
> more concerned I am about this approach. I think we need to step back and
think
> about what it is you are trying to accomplish (presumably limiting total
memory
> usage) and how best to solve that (across SourceBuffers, streams, media
> elements, etc.).

Hey Servolk, Can we close this?