Add SupportsConfigChanges() to DemuxerStream.

Add SupportsConfigChanges() to DemuxerStream.

This signals to the client whether or not it is expected to be able to handle midstream configuration changes.

For instance, in the AudioRendererImpl, we must take a slower path to support config changes. So using this will enable us to pick the faster path when config change handling is unneeded.

BUG=347270
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259484

[rileya (GONE FROM CHROMIUM), 2014-03-24 18:54:44]

To support audio config changes seamlessly we need to resample and remix audio
immediately as we get it. However, when we don't have to worry about config
changes (i.e. we're not using MSE) we'd needlessly take a performance hit doing
it upfront, rather than letting it happen in AudioRendererMixer.

This will allow the AudioRendererImpl to decide whether or not to do this
conversion step, depending on whether we're expecting config changes or not.

[DaleCurtis, 2014-03-24 19:00:59]

I think the idea is sound, but wait for scherkus@ to comment before going wild
and adding the method to every demuxer stream.

https://codereview.chromium.org/208263018/diff/1/media/base/demuxer_stream.h
File media/base/demuxer_stream.h (right):

https://codereview.chromium.org/208263018/diff/1/media/base/demuxer_stream.h#...
media/base/demuxer_stream.h:72: virtual bool SupportsConfigChanges();
Unfortunately, this should be = 0; and each implementation will need to define
this method.

[scherkus (not reviewing), 2014-03-25 17:33:53]

can I get a quick explanation as to how this will be used by downstream clients?

do we know for certain that config changes aren't possible with FFmpegDemuxer?
if it is possible, then we might not need this

[rileya (GONE FROM CHROMIUM), 2014-03-25 18:04:11]

On 2014/03/25 17:33:53, scherkus wrote:
> can I get a quick explanation as to how this will be used by downstream
clients?
> 
> do we know for certain that config changes aren't possible with FFmpegDemuxer?
> if it is possible, then we might not need this

To support audio config changes without tearing everything down (and probably
inducing delays/sound skips), we need to remix/resample the audio to a common
format immediately as we get it from the DecoderStream. This is slower than
letting it happen later on, in the AudioRendererMixer.

So this will be used to only do the costly audio conversion upfront in
AudioRendererImpl if we could possibly see config changes (and using MSE is the
only time that is the case afaik). No sense making all other cases slower for a
feature that only MSE uses.

As far as I can tell FFmpegDemuxer will never report a config change currently.
In fact, we guard against midstream config changes by error-ing out if we see
them in FFmpegDecoder. ChunkDemuxer (and possibly DecryptingDemuxerStream) are
the only Demuxers that currently signal config changes.

[scherkus (not reviewing), 2014-03-25 21:14:47]

On 2014/03/25 18:04:11, rileya wrote:
> On 2014/03/25 17:33:53, scherkus wrote:
> > can I get a quick explanation as to how this will be used by downstream
> clients?
> > 
> > do we know for certain that config changes aren't possible with
FFmpegDemuxer?
> > if it is possible, then we might not need this
> 
> To support audio config changes without tearing everything down (and probably
> inducing delays/sound skips), we need to remix/resample the audio to a common
> format immediately as we get it from the DecoderStream. This is slower than
> letting it happen later on, in the AudioRendererMixer.
> 
> So this will be used to only do the costly audio conversion upfront in
> AudioRendererImpl if we could possibly see config changes (and using MSE is
the
> only time that is the case afaik). No sense making all other cases slower for
a
> feature that only MSE uses.
> 
> As far as I can tell FFmpegDemuxer will never report a config change
currently.
> In fact, we guard against midstream config changes by error-ing out if we see
> them in FFmpegDecoder. ChunkDemuxer (and possibly DecryptingDemuxerStream) are
> the only Demuxers that currently signal config changes.

alright -- can you summarize that in the CL description?

[scherkus (not reviewing), 2014-03-25 21:14:52]

https://codereview.chromium.org/208263018/diff/1/media/base/demuxer_stream.h
File media/base/demuxer_stream.h (right):

https://codereview.chromium.org/208263018/diff/1/media/base/demuxer_stream.h#...
media/base/demuxer_stream.h:71: // Whether or not this DemuxerStream allows
midstream configuration changes.
I know EnableBitsreamConverter() is sorely lacking in documentation, but we can
certainly make this better

the question I have is an API consumer what does this mean for me? how will a
DemuxerStream operate differently when this returns true vs. false? (e.g., will
video_decoder_config() change?)

[rileya (GONE FROM CHROMIUM), 2014-03-25 21:45:06]

Updated CL description, added a better comment to SupportsConfigChanges() and
made it pure virtual + added impls for various DemuxerStream subclasses. PTAL.

https://codereview.chromium.org/208263018/diff/1/media/base/demuxer_stream.h
File media/base/demuxer_stream.h (right):

https://codereview.chromium.org/208263018/diff/1/media/base/demuxer_stream.h#...
media/base/demuxer_stream.h:71: // Whether or not this DemuxerStream allows
midstream configuration changes.
On 2014/03/25 21:14:53, scherkus wrote:
> I know EnableBitsreamConverter() is sorely lacking in documentation, but we
can
> certainly make this better
> 
> the question I have is an API consumer what does this mean for me? how will a
> DemuxerStream operate differently when this returns true vs. false? (e.g.,
will
> video_decoder_config() change?)

Added more detail.

https://codereview.chromium.org/208263018/diff/1/media/base/demuxer_stream.h#...
media/base/demuxer_stream.h:72: virtual bool SupportsConfigChanges();
On 2014/03/24 19:01:00, DaleCurtis wrote:
> Unfortunately, this should be = 0; and each implementation will need to define
> this method.

Done.

[DaleCurtis, 2014-03-25 21:58:03]

lgtm

[scherkus (not reviewing), 2014-03-25 22:16:47]

some q's

https://codereview.chromium.org/208263018/diff/200002/media/filters/decoder_s...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/208263018/diff/200002/media/filters/decoder_s...
media/filters/decoder_stream.cc:373: DCHECK(stream_->SupportsConfigChanges());
\o/

https://codereview.chromium.org/208263018/diff/200002/media/filters/decryptin...
File media/filters/decrypting_demuxer_stream.cc (right):

https://codereview.chromium.org/208263018/diff/200002/media/filters/decryptin...
media/filters/decrypting_demuxer_stream.cc:167: return demuxer_stream_ ?
demuxer_stream_->SupportsConfigChanges() : true;
line 161 doesn't make this assumption nor do other methods

my $0.02: try to avoid optional / unsure code and find out whether we can stick
with the much simpler "demuxer_stream->SCC();"

https://codereview.chromium.org/208263018/diff/200002/media/filters/fake_demu...
File media/filters/fake_demuxer_stream.cc (right):

https://codereview.chromium.org/208263018/diff/200002/media/filters/fake_demu...
media/filters/fake_demuxer_stream.cc:89: return true;
is this needed for some tests?

do we need some tests to cover the false behaviour?

[rileya (GONE FROM CHROMIUM), 2014-03-25 22:43:19]

https://codereview.chromium.org/208263018/diff/200002/media/filters/decryptin...
File media/filters/decrypting_demuxer_stream.cc (right):

https://codereview.chromium.org/208263018/diff/200002/media/filters/decryptin...
media/filters/decrypting_demuxer_stream.cc:167: return demuxer_stream_ ?
demuxer_stream_->SupportsConfigChanges() : true;
On 2014/03/25 22:16:47, scherkus wrote:
> line 161 doesn't make this assumption nor do other methods
> 
> my $0.02: try to avoid optional / unsure code and find out whether we can
stick
> with the much simpler "demuxer_stream->SCC();"

Good point, removed that behavior. Looks like the only time we create a DDS
outside of the unit tests, we Initialize() it immediately. I figured a DCHECK
couldn't hurt anyway though.

https://codereview.chromium.org/208263018/diff/200002/media/filters/fake_demu...
File media/filters/fake_demuxer_stream.cc (right):

https://codereview.chromium.org/208263018/diff/200002/media/filters/fake_demu...
media/filters/fake_demuxer_stream.cc:89: return true;
On 2014/03/25 22:16:47, scherkus wrote:
> is this needed for some tests?
> 
> do we need some tests to cover the false behaviour?

Yeah, it's used by the tests. I made it return true or false depending on
whether the FDS was initialized with multiple configs. I also added a test for
the 'false' behavior.

[scherkus (not reviewing), 2014-03-25 23:17:58]

lgtm w/ nit

https://codereview.chromium.org/208263018/diff/300001/media/filters/decryptin...
File media/filters/decrypting_demuxer_stream.cc (right):

https://codereview.chromium.org/208263018/diff/300001/media/filters/decryptin...
media/filters/decrypting_demuxer_stream.cc:165: DCHECK(demuxer_stream_);
we tend to avoid these DCHECK()s as null pointer dereferences have their own
built-in DCHECK-like behaviour that even works in release mode (i.e., they
crash)

(also our code would be littered with 1000s of DCHECK()s for valid pointers! the
only time we tend to use them is if the pointer isn't immediately dereferenced
[e.g., pass a pointer into a ctor that requires it to be non-null, but you only
use the pointer sometime way after the ctor completes])

[rileya (GONE FROM CHROMIUM), 2014-03-25 23:34:32]

On 2014/03/25 23:17:58, scherkus wrote:
> lgtm w/ nit
> 
>
https://codereview.chromium.org/208263018/diff/300001/media/filters/decryptin...
> File media/filters/decrypting_demuxer_stream.cc (right):
> 
>
https://codereview.chromium.org/208263018/diff/300001/media/filters/decryptin...
> media/filters/decrypting_demuxer_stream.cc:165: DCHECK(demuxer_stream_);
> we tend to avoid these DCHECK()s as null pointer dereferences have their own
> built-in DCHECK-like behaviour that even works in release mode (i.e., they
> crash)
> 
> (also our code would be littered with 1000s of DCHECK()s for valid pointers!
the
> only time we tend to use them is if the pointer isn't immediately dereferenced
> [e.g., pass a pointer into a ctor that requires it to be non-null, but you
only
> use the pointer sometime way after the ctor completes])

Okie doke, sounds good, removed it.

[rileya (GONE FROM CHROMIUM), 2014-03-25 23:34:41]

The CQ bit was checked by rileya@chromium.org

[commit-bot: I haz the power, 2014-03-25 23:37:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rileya@chromium.org/208263018/320001

[commit-bot: I haz the power, 2014-03-26 04:37:27]

Change committed as 259484