Encrypted Media: Support config change in DecryptingDemuxerStream.

Encrypted Media: Support config change in DecryptingDemuxerStream.

The DecryptingDemuxerStream was returning kAborted upon config change, which
indirectly caused this crash. Add config change support in this CL.

Also, expect the new stream config is still valid and still encrypted. This is ensured by the upstream demuxer. One reason for this change is that we don't have a good way to report error through the read callnack. Returning kAborted would potentially introduce the same crash. As a result, related (invalid/unencrypted config) tests are removed.

For now, DecryptingXxxDecoders won't introduce the same crash because they report decoder error
immediately when config change is encountered. In a later CL, config change support will be added to these decoders.

BUG=167811
TEST=Updated unittests.


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

[xhwang, 2013-01-03 01:14:09]

This CL adds config change support in DecryptingDemuxerStream (DDS) and fixes
the crash. I'll add config change support in DecryptingVideoDecoder (DVD) and
DecryptingAudioDecoder (DAD) in a later CL.

PTAL!

[acolwell GONE FROM CHROMIUM, 2013-01-03 01:39:39]

LGTM % nits

https://codereview.chromium.org/11722008/diff/1/media/filters/decrypting_demu...
File media/filters/decrypting_demuxer_stream.cc (right):

https://codereview.chromium.org/11722008/diff/1/media/filters/decrypting_demu...
media/filters/decrypting_demuxer_stream.cc:25: static bool
IsStreamValidAndEncrytped(
nit: s/Encrytped/Encrypted/

https://codereview.chromium.org/11722008/diff/1/media/filters/decrypting_demu...
media/filters/decrypting_demuxer_stream.cc:207: // TODO(xhwang): If needed,
supported switching from encrypted stream to
nit: s/supported/support

[ddorwin, 2013-01-03 01:45:36]

LGTM % questions.

https://codereview.chromium.org/11722008/diff/1/media/filters/decrypting_demu...
File media/filters/decrypting_demuxer_stream.cc (right):

https://codereview.chromium.org/11722008/diff/1/media/filters/decrypting_demu...
media/filters/decrypting_demuxer_stream.cc:158: SetDecoderConfig();
Why was/is this done here instead of in DoInitialize()?

https://codereview.chromium.org/11722008/diff/1/media/filters/decrypting_demu...
media/filters/decrypting_demuxer_stream.cc:207: // TODO(xhwang): If needed,
supported switching from encrypted stream to
Does this TODO belong in SourceBufferStream instead? It seems you have two close
locations that could change where you might need this comment in a couple
decryptor files. Or, we can just remove it (and maybe file a bug).

https://codereview.chromium.org/11722008/diff/1/media/filters/decrypting_demu...
media/filters/decrypting_demuxer_stream.cc:213:
base::ResetAndReturn(&read_cb_).Run(kConfigChanged, NULL);
Where does the config change take place? Does the actual decoder pick up the
change by calling the accessor? Might be worth commenting what's expected to
happen to complete the process.

[xhwang, 2013-01-03 17:13:54]

https://codereview.chromium.org/11722008/diff/1/media/filters/decrypting_demu...
File media/filters/decrypting_demuxer_stream.cc (right):

https://codereview.chromium.org/11722008/diff/1/media/filters/decrypting_demu...
media/filters/decrypting_demuxer_stream.cc:25: static bool
IsStreamValidAndEncrytped(
On 2013/01/03 01:39:40, acolwell wrote:
> nit: s/Encrytped/Encrypted/

Done.

https://codereview.chromium.org/11722008/diff/1/media/filters/decrypting_demu...
media/filters/decrypting_demuxer_stream.cc:158: SetDecoderConfig();
On 2013/01/03 01:45:36, ddorwin wrote:
> Why was/is this done here instead of in DoInitialize()?

Good question. I don't see why not. Fixed.

https://codereview.chromium.org/11722008/diff/1/media/filters/decrypting_demu...
media/filters/decrypting_demuxer_stream.cc:207: // TODO(xhwang): If needed,
supported switching from encrypted stream to
On 2013/01/03 01:45:36, ddorwin wrote:
> Does this TODO belong in SourceBufferStream instead? It seems you have two
close
> locations that could change where you might need this comment in a couple
> decryptor files. Or, we can just remove it (and maybe file a bug).

Removed as I don't see any use case for that.

https://codereview.chromium.org/11722008/diff/1/media/filters/decrypting_demu...
media/filters/decrypting_demuxer_stream.cc:213:
base::ResetAndReturn(&read_cb_).Run(kConfigChanged, NULL);
On 2013/01/03 01:45:36, ddorwin wrote:
> Where does the config change take place? Does the actual decoder pick up the
> change by calling the accessor? Might be worth commenting what's expected to
> happen to complete the process.

Added comment. The decoder will get notified of kConfigChange, and it will call
xxx_decoder_config() to get the new config, which is updated on line 211.

[ddorwin, 2013-01-03 17:48:29]

minor stuff

https://codereview.chromium.org/11722008/diff/5002/media/filters/decrypting_d...
File media/filters/decrypting_demuxer_stream.cc (right):

https://codereview.chromium.org/11722008/diff/5002/media/filters/decrypting_d...
media/filters/decrypting_demuxer_stream.cc:136:
DCHECK(IsStreamValidAndEncrypted(stream));
WDYT about moving this inside SetDecoderConfig() now that they are always called
together?

https://codereview.chromium.org/11722008/diff/5002/media/filters/decrypting_d...
media/filters/decrypting_demuxer_stream.cc:209: // Update the decoder config
with the new config.
This comment doesn't really add anything that the function name doesn't provide.
I would suggest something similar to your response to my comment: "Update the
decoder config, which the decoder will use when it is notified of
kConfigChanged."

https://codereview.chromium.org/11722008/diff/5002/media/filters/decrypting_d...
media/filters/decrypting_demuxer_stream.cc:210: SetDecoderConfig();
SetDecoderConfigFromDemuxerStream? Should Set be Update?
I know this is long, but it's kind of odd to have a Set function that has no
parameters.
Alternatively, you could pass demuxer_stream_ as a parameter. That leaves the
function generic and makes it clear at the call site what value is being used.
It also makes it similar to the IsStreamValidAndEncrypted() calls that precede
it.

[xhwang, 2013-01-03 18:16:04]

https://codereview.chromium.org/11722008/diff/5002/media/filters/decrypting_d...
File media/filters/decrypting_demuxer_stream.cc (right):

https://codereview.chromium.org/11722008/diff/5002/media/filters/decrypting_d...
media/filters/decrypting_demuxer_stream.cc:136:
DCHECK(IsStreamValidAndEncrypted(stream));
On 2013/01/03 17:48:29, ddorwin wrote:
> WDYT about moving this inside SetDecoderConfig() now that they are always
called
> together?

Done.

https://codereview.chromium.org/11722008/diff/5002/media/filters/decrypting_d...
media/filters/decrypting_demuxer_stream.cc:209: // Update the decoder config
with the new config.
On 2013/01/03 17:48:29, ddorwin wrote:
> This comment doesn't really add anything that the function name doesn't
provide.
> I would suggest something similar to your response to my comment: "Update the
> decoder config, which the decoder will use when it is notified of
> kConfigChanged."

Done.

https://codereview.chromium.org/11722008/diff/5002/media/filters/decrypting_d...
media/filters/decrypting_demuxer_stream.cc:210: SetDecoderConfig();
On 2013/01/03 17:48:29, ddorwin wrote:
> SetDecoderConfigFromDemuxerStream? Should Set be Update?
> I know this is long, but it's kind of odd to have a Set function that has no
> parameters.
> Alternatively, you could pass demuxer_stream_ as a parameter. That leaves the
> function generic and makes it clear at the call site what value is being used.
> It also makes it similar to the IsStreamValidAndEncrypted() calls that precede
> it.

Done.

[commit-bot: I haz the power, 2013-01-03 18:29:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xhwang@chromium.org/11722008/10001

[commit-bot: I haz the power, 2013-01-03 20:57:23]

Change committed as 175007