media: Support better decoder switching

media: Support better decoder switching

Today on reinitialization error, or decode error of the first buffer,
DecoderStream will fall back to decoders left in the DecoderSelector.
Since we will have less and less decoders in the DecoderSelector, the
ability to switch decoders is limited.

This CL updates DecoderSelector so that it takes a callback to create
a list of decoders, instead of taking the list of decoders directly.
This allows the DecoderSelector to select a decoder that has been
tried or selected before, such that upon decoder reinitialization error
(e.g. switching from clear to encrypted config), or upon decode error of
the first buffer, we always have the full list of decoders to select
from.

Two mechanisms are added to avoid trying the same failing decoder again:

1. Blacklist

When SelectDecoder() is called due to reinitialization failure, or
decoding failure of the first buffer, the existing decoder is listed as
the "blacklisted decoder" such that DecoderSelector will not even try
it.

There is one exception though. When DecryptingDemuxerStream is selected,
since the input steam is changed (encrypted -> clear), the blacklisted
decoder is ignored. For example, FFmpegVideoDecoder is slected first for
a clear stream. Then on a config change, the stream becomes encrypted so
FFmpegVideoDecoder fails to reinitialize and we need to select a new
decoder. After DecryptingDemuxerStream is selected, we should still be
able to use FFmpegVideoDecoder to decode the decrypted stream.

2. Fall back at most once

If fallback has already happened and decode of the first buffer failed
again, we don't try to fallback again. This is to avoid the infinite
loop of "select decoder 1 -> decode error -> select decoder 2 -> decode
error -> select decoder 1".

BUG=695595
TEST=Updated/Added unittests.

Review-Url: https://codereview.chromium.org/2837613004
Cr-Commit-Position: refs/heads/master@{#469579}
Committed: https://chromium.googlesource.com/chromium/src/+/1492be9e0d583b0110f22d7e5f2ded139611f8bd

[xhwang, 2017-04-24 19:25:41]

xhwang@chromium.org changed reviewers:
+ dalecurtis@chromium.org, sandersd@chromium.org

[xhwang, 2017-04-24 19:25:41]

dalecurtis / sandersd: I still need to update the tests. But before that, please
do a high level review of the CL to make sure I am on the right path. If it
looks good to you, I'll polish the CL (where needed), update existing tests, and
add new test cases.

[DaleCurtis, 2017-04-24 19:39:40]

High level sgtm. I need to think about the blacklist a little more, but I think
for our scoped use cases just a single entry should work out okay.

https://codereview.chromium.org/2837613004/diff/1/media/filters/decoder_selec...
File media/filters/decoder_selector.cc (right):

https://codereview.chromium.org/2837613004/diff/1/media/filters/decoder_selec...
media/filters/decoder_selector.cc:204: // When |decrypted_stream_| is selected,
the |config_| has changed so ignore
Can you explain this comment a bit more? The code above seems to prevent vending
if blacklist match, so how are we hitting this point?

[DaleCurtis, 2017-04-24 19:41:05]

https://codereview.chromium.org/2837613004/diff/1/media/filters/decoder_selec...
File media/filters/decoder_selector.cc (right):

https://codereview.chromium.org/2837613004/diff/1/media/filters/decoder_selec...
media/filters/decoder_selector.cc:204: // When |decrypted_stream_| is selected,
the |config_| has changed so ignore
On 2017/04/24 at 19:39:39, DaleCurtis wrote:
> Can you explain this comment a bit more? The code above seems to prevent
vending if blacklist match, so how are we hitting this point?

Ah just saw your entry in the CL description now. Should this bypass be in the
DecoderStream instead? It seems more informed about making this decision.

[sandersd (OOO until July 31), 2017-04-24 20:55:15]

Basic structure seems sound, but I am hoping for a more explicit handling of the
selection state so that future selection algorithm changes will be simple and
clear.

https://codereview.chromium.org/2837613004/diff/1/media/filters/decoder_selec...
File media/filters/decoder_selector.cc (right):

https://codereview.chromium.org/2837613004/diff/1/media/filters/decoder_selec...
media/filters/decoder_selector.cc:71: const std::string& blacklisted_decoder,
I worry that blacklisting a single decoder may be insufficient in some cases.
(Example: same decoder available via GpuVideoDecoder and MojoVideoDecoder.) The
cases I can come up with are all temporary and shouldn't be shipped to users,
but I'm not convinced the behavior is obvious in all the possible cases.

Perhaps the selection state can be reified and handled more explicitly?

https://codereview.chromium.org/2837613004/diff/1/media/filters/decoder_selec...
media/filters/decoder_selector.cc:125: if (decoder_->GetDisplayName() ==
blacklisted_decoder_) {
This implies some new semantics for the DisplayName of a decoder, which should
be documented. (Specifically, it now needs to be stable and defined before
Initialize().)

I'm actually not sure what should be done for MojoVideoDecoder here. I do want
to proxy the remote decoder name for logging purposes, but depending on exactly
how that is implemented it may only be reasonable after Initialize().

https://codereview.chromium.org/2837613004/diff/1/media/filters/decoder_selec...
File media/filters/decoder_selector.h (right):

https://codereview.chromium.org/2837613004/diff/1/media/filters/decoder_selec...
media/filters/decoder_selector.h:43: using CreateDecodersCB =
base::RepeatingCallback<ScopedVector<Decoder>()>;
Is now perhaps a good time to replace this ScopedVector?

[xhwang, 2017-04-25 00:43:30]

comments-only

https://codereview.chromium.org/2837613004/diff/1/media/filters/decoder_selec...
File media/filters/decoder_selector.cc (right):

https://codereview.chromium.org/2837613004/diff/1/media/filters/decoder_selec...
media/filters/decoder_selector.cc:71: const std::string& blacklisted_decoder,
On 2017/04/24 20:55:14, sandersd wrote:
> I worry that blacklisting a single decoder may be insufficient in some cases.
> (Example: same decoder available via GpuVideoDecoder and MojoVideoDecoder.)
The
> cases I can come up with are all temporary and shouldn't be shipped to users,
> but I'm not convinced the behavior is obvious in all the possible cases.

We "could" host the same decoder via GVD and MVD, but I don't see why we want to
do that in production... That being said, it should be trivial to extend the
"blacklisted_decoder" to a "blacklisted_decoder_list".
 
> Perhaps the selection state can be reified and handled more explicitly?

Could you please elaborate a bit more on what "selection state" you are
referring to?

https://codereview.chromium.org/2837613004/diff/1/media/filters/decoder_selec...
media/filters/decoder_selector.cc:125: if (decoder_->GetDisplayName() ==
blacklisted_decoder_) {
On 2017/04/24 20:55:14, sandersd wrote:
> This implies some new semantics for the DisplayName of a decoder, which should
> be documented. (Specifically, it now needs to be stable and defined before
> Initialize().)

Good point. Will do!

> I'm actually not sure what should be done for MojoVideoDecoder here. I do want
> to proxy the remote decoder name for logging purposes, but depending on
exactly
> how that is implemented it may only be reasonable after Initialize().

This could be useful in the decode-error-fallback case when MojoVideoDecoder
hosts multiple decoders. But in that case, MojoVideoDecoder can have it's own
fallback logic when decode-error happens, such that if it really fails (even
after trying fallback), putting it in the blacklist is still the right choice.

https://codereview.chromium.org/2837613004/diff/1/media/filters/decoder_selec...
media/filters/decoder_selector.cc:204: // When |decrypted_stream_| is selected,
the |config_| has changed so ignore
On 2017/04/24 19:41:04, DaleCurtis wrote:
> On 2017/04/24 at 19:39:39, DaleCurtis wrote:
> > Can you explain this comment a bit more? The code above seems to prevent
> vending if blacklist match, so how are we hitting this point?
> 
> Ah just saw your entry in the CL description now. Should this bypass be in the
> DecoderStream instead? It seems more informed about making this decision.

From my understanding, DecoderStream doesn't know that a DecryptingDemuxerStream
could be selected in DecoderSelector. So  there's no easy path for DecoderStream
to bypass this. Am I missing something here? :)

https://codereview.chromium.org/2837613004/diff/1/media/filters/decoder_selec...
File media/filters/decoder_selector.h (right):

https://codereview.chromium.org/2837613004/diff/1/media/filters/decoder_selec...
media/filters/decoder_selector.h:43: using CreateDecodersCB =
base::RepeatingCallback<ScopedVector<Decoder>()>;
On 2017/04/24 20:55:14, sandersd wrote:
> Is now perhaps a good time to replace this ScopedVector?

Agreed, and this is the tracking bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=588249

But I don't think this should be in this CL. Could be done in a follow up CL.

[sandersd (OOO until July 31), 2017-04-27 18:45:31]

Summary of our offline conversation is that xhwang@ will update the
documentation for GetDisplayName(), and we will leave the other refactoring I
have requested as future (OKR-planned) work.

[xhwang, 2017-05-03 08:29:00]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 08:29:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 09:03:00]

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

[commit-bot: I haz the power, 2017-05-03 09:03:01]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[xhwang, 2017-05-03 18:53:48]

Patchset #2 (id:20001) has been deleted

[xhwang, 2017-05-03 18:53:56]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 18:54:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 19:24:34]

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

[commit-bot: I haz the power, 2017-05-03 19:24:35]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[xhwang, 2017-05-03 20:32:23]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 20:33:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 22:20:51]

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

[commit-bot: I haz the power, 2017-05-03 22:20:52]

Dry run: This issue passed the CQ dry run.

[xhwang, 2017-05-03 23:14:20]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 23:15:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xhwang, 2017-05-03 23:15:41]

Description was changed from

==========
media: Support switching decoders

This CL updates DecoderSelector so that it takes a callback to create
a list of decoders, instead of taking the list of decoders directly.
This allows the DecoderSelector to select a decoder that has been
tried or selected before, such that upon decoder reinitialization error
(e.g. switching from clear to encrypted config), or upon decode error of
the first buffer, we always have the full list of decoders to select
from.

Two mechanisms are added to avoid trying the same failing decoder again:

1. Black list

When SelectDecoder() is called due to reinitialization failure, or
decoding failure of the first buffer, the existing decoder is listed as
the "blacklisted decoder" such that DecoderSelector will not even try
it.

There is one exception though. When DecryptingDemuxerStream is selected,
since the input steam is changed (encrypted -> clear), the blacklisted
decoder is ignored. For example, FFmpegVideoDecoder is slected first for
a clear stream. Then on a config change, the stream becomes encrypted so
FFmpegVideoDecoder fails to reinitialize and we need to select a new
decoder. After DecryptingDemuxerStream is selected, we should still be
able to use FFmpegVideoDecoder to decode the decrypted stream.

2. Fallback at most once

If fallback has already happened and decode of the first buffer failed
again, we don't try to fallback again. This is to avoid the infinate
loop of "select decoder 1 -> decode error -> select decoder 2 -> decode
error -> select decoder 1".

BUG=695595
TEST=Updated/Added unittests.
==========

to

==========
media: Support switching decoders

This CL updates DecoderSelector so that it takes a callback to create
a list of decoders, instead of taking the list of decoders directly.
This allows the DecoderSelector to select a decoder that has been
tried or selected before, such that upon decoder reinitialization error
(e.g. switching from clear to encrypted config), or upon decode error of
the first buffer, we always have the full list of decoders to select
from.

Two mechanisms are added to avoid trying the same failing decoder again:

1. Black list

When SelectDecoder() is called due to reinitialization failure, or
decoding failure of the first buffer, the existing decoder is listed as
the "blacklisted decoder" such that DecoderSelector will not even try
it.

There is one exception though. When DecryptingDemuxerStream is selected,
since the input steam is changed (encrypted -> clear), the blacklisted
decoder is ignored. For example, FFmpegVideoDecoder is slected first for
a clear stream. Then on a config change, the stream becomes encrypted so
FFmpegVideoDecoder fails to reinitialize and we need to select a new
decoder. After DecryptingDemuxerStream is selected, we should still be
able to use FFmpegVideoDecoder to decode the decrypted stream.

2. Fallback at most once

If fallback has already happened and decode of the first buffer failed
again, we don't try to fallback again. This is to avoid the infinite
loop of "select decoder 1 -> decode error -> select decoder 2 -> decode
error -> select decoder 1".

BUG=695595
TEST=Updated/Added unittests.
==========

[xhwang, 2017-05-03 23:16:55]

I updated the existing tests and added new test cases. This feature is also
covered by existing pipeline integration tests and browser tests.

PTAL!

[commit-bot: I haz the power, 2017-05-04 00:54:50]

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

[commit-bot: I haz the power, 2017-05-04 00:54:51]

Dry run: This issue passed the CQ dry run.

[DaleCurtis, 2017-05-04 02:37:24]

Overall idea lg2m, but won't have time to dig in thoroughly before OOO so defer
to sandersd@.

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

https://codereview.chromium.org/2837613004/diff/80001/media/filters/decoder_s...
media/filters/decoder_stream.cc:510: is_on_fallback_decoder_ = false;
Name is a bit off since we clear it here. Not sure of a better one though,

[xhwang, 2017-05-04 05:20:34]

xhwang@chromium.org changed reviewers:
+ watk@chromium.org

[xhwang, 2017-05-04 05:20:35]

watk: Please also take a look given Dale is going to be OOO soon, and that you
are very familiar with the fallback logic.

[sandersd (OOO until July 31), 2017-05-04 20:27:59]

Please update the documentation for GetDisplayName(), and add a TODO for
renaming it.

[watk, 2017-05-04 20:43:48]

Description was changed from

==========
media: Support switching decoders

This CL updates DecoderSelector so that it takes a callback to create
a list of decoders, instead of taking the list of decoders directly.
This allows the DecoderSelector to select a decoder that has been
tried or selected before, such that upon decoder reinitialization error
(e.g. switching from clear to encrypted config), or upon decode error of
the first buffer, we always have the full list of decoders to select
from.

Two mechanisms are added to avoid trying the same failing decoder again:

1. Black list

When SelectDecoder() is called due to reinitialization failure, or
decoding failure of the first buffer, the existing decoder is listed as
the "blacklisted decoder" such that DecoderSelector will not even try
it.

There is one exception though. When DecryptingDemuxerStream is selected,
since the input steam is changed (encrypted -> clear), the blacklisted
decoder is ignored. For example, FFmpegVideoDecoder is slected first for
a clear stream. Then on a config change, the stream becomes encrypted so
FFmpegVideoDecoder fails to reinitialize and we need to select a new
decoder. After DecryptingDemuxerStream is selected, we should still be
able to use FFmpegVideoDecoder to decode the decrypted stream.

2. Fallback at most once

If fallback has already happened and decode of the first buffer failed
again, we don't try to fallback again. This is to avoid the infinite
loop of "select decoder 1 -> decode error -> select decoder 2 -> decode
error -> select decoder 1".

BUG=695595
TEST=Updated/Added unittests.
==========

to

==========
media: Support switching decoders

This CL updates DecoderSelector so that it takes a callback to create
a list of decoders, instead of taking the list of decoders directly.
This allows the DecoderSelector to select a decoder that has been
tried or selected before, such that upon decoder reinitialization error
(e.g. switching from clear to encrypted config), or upon decode error of
the first buffer, we always have the full list of decoders to select
from.

Two mechanisms are added to avoid trying the same failing decoder again:

1. Blacklist

When SelectDecoder() is called due to reinitialization failure, or
decoding failure of the first buffer, the existing decoder is listed as
the "blacklisted decoder" such that DecoderSelector will not even try
it.

There is one exception though. When DecryptingDemuxerStream is selected,
since the input steam is changed (encrypted -> clear), the blacklisted
decoder is ignored. For example, FFmpegVideoDecoder is slected first for
a clear stream. Then on a config change, the stream becomes encrypted so
FFmpegVideoDecoder fails to reinitialize and we need to select a new
decoder. After DecryptingDemuxerStream is selected, we should still be
able to use FFmpegVideoDecoder to decode the decrypted stream.

2. Fallback at most once

If fallback has already happened and decode of the first buffer failed
again, we don't try to fallback again. This is to avoid the infinite
loop of "select decoder 1 -> decode error -> select decoder 2 -> decode
error -> select decoder 1".

BUG=695595
TEST=Updated/Added unittests.
==========

[xhwang, 2017-05-04 20:56:48]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 20:57:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xhwang, 2017-05-04 20:57:23]

On 2017/05/04 20:27:59, sandersd wrote:
> Please update the documentation for GetDisplayName(), and add a TODO for
> renaming it.

Oops, I forgot about this. Sorry about that.

Done.

[commit-bot: I haz the power, 2017-05-04 21:03:17]

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

[commit-bot: I haz the power, 2017-05-04 21:03:18]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[watk, 2017-05-04 21:34:04]

Description was changed from

==========
media: Support switching decoders

This CL updates DecoderSelector so that it takes a callback to create
a list of decoders, instead of taking the list of decoders directly.
This allows the DecoderSelector to select a decoder that has been
tried or selected before, such that upon decoder reinitialization error
(e.g. switching from clear to encrypted config), or upon decode error of
the first buffer, we always have the full list of decoders to select
from.

Two mechanisms are added to avoid trying the same failing decoder again:

1. Blacklist

When SelectDecoder() is called due to reinitialization failure, or
decoding failure of the first buffer, the existing decoder is listed as
the "blacklisted decoder" such that DecoderSelector will not even try
it.

There is one exception though. When DecryptingDemuxerStream is selected,
since the input steam is changed (encrypted -> clear), the blacklisted
decoder is ignored. For example, FFmpegVideoDecoder is slected first for
a clear stream. Then on a config change, the stream becomes encrypted so
FFmpegVideoDecoder fails to reinitialize and we need to select a new
decoder. After DecryptingDemuxerStream is selected, we should still be
able to use FFmpegVideoDecoder to decode the decrypted stream.

2. Fallback at most once

If fallback has already happened and decode of the first buffer failed
again, we don't try to fallback again. This is to avoid the infinite
loop of "select decoder 1 -> decode error -> select decoder 2 -> decode
error -> select decoder 1".

BUG=695595
TEST=Updated/Added unittests.
==========

to

==========
media: Support switching decoders

This CL updates DecoderSelector so that it takes a callback to create
a list of decoders, instead of taking the list of decoders directly.
This allows the DecoderSelector to select a decoder that has been
tried or selected before, such that upon decoder reinitialization error
(e.g. switching from clear to encrypted config), or upon decode error of
the first buffer, we always have the full list of decoders to select
from.

Two mechanisms are added to avoid trying the same failing decoder again:

1. Blacklist

When SelectDecoder() is called due to reinitialization failure, or
decoding failure of the first buffer, the existing decoder is listed as
the "blacklisted decoder" such that DecoderSelector will not even try
it.

There is one exception though. When DecryptingDemuxerStream is selected,
since the input steam is changed (encrypted -> clear), the blacklisted
decoder is ignored. For example, FFmpegVideoDecoder is slected first for
a clear stream. Then on a config change, the stream becomes encrypted so
FFmpegVideoDecoder fails to reinitialize and we need to select a new
decoder. After DecryptingDemuxerStream is selected, we should still be
able to use FFmpegVideoDecoder to decode the decrypted stream.

2. Fall back at most once

If fallback has already happened and decode of the first buffer failed
again, we don't try to fallback again. This is to avoid the infinite
loop of "select decoder 1 -> decode error -> select decoder 2 -> decode
error -> select decoder 1".

BUG=695595
TEST=Updated/Added unittests.
==========

[watk, 2017-05-04 21:35:33]

Looks correct to me! I mostly had nits.

Could you please update the commit summary to be a little more descriptive
(because we already supported switching decoders).

https://codereview.chromium.org/2837613004/diff/80001/media/base/audio_decode...
File media/base/audio_decoder_config.h (right):

https://codereview.chromium.org/2837613004/diff/80001/media/base/audio_decode...
media/base/audio_decoder_config.h:91: // useful for decryptors that decrypts an
encrypted stream to a clear stream.
If you want to fix this while you're at it :) s/decrypts/decrypt

https://codereview.chromium.org/2837613004/diff/80001/media/base/mock_filters.h
File media/base/mock_filters.h (right):

https://codereview.chromium.org/2837613004/diff/80001/media/base/mock_filters...
media/base/mock_filters.h:182: explicit MockVideoDecoder(const std::string&
decoder_name = "");
You can default the name to "MockVideoDecoder" here instead of in
GetDisplayName()?

https://codereview.chromium.org/2837613004/diff/80001/media/base/video_decode...
File media/base/video_decoder_config.h (right):

https://codereview.chromium.org/2837613004/diff/80001/media/base/video_decode...
media/base/video_decoder_config.h:123: // useful for decryptors that decrypts an
encrypted stream to a clear stream.
as above, s/decrypts/decrypt.

https://codereview.chromium.org/2837613004/diff/80001/media/filters/video_dec...
File media/filters/video_decoder_selector_unittest.cc (right):

https://codereview.chromium.org/2837613004/diff/80001/media/filters/video_dec...
media/filters/video_decoder_selector_unittest.cc:238:
TEST_F(VideoDecoderSelectorTest, ClearStream_BlackListedDecoder) {
Here and another one below: s/BlackListed/Blacklisted/

https://codereview.chromium.org/2837613004/diff/100001/media/filters/audio_de...
File media/filters/audio_decoder_selector_unittest.cc (right):

https://codereview.chromium.org/2837613004/diff/100001/media/filters/audio_de...
media/filters/audio_decoder_selector_unittest.cc:184: // Tests for clear
streams. CDM will not be used for clear streams so
s/CDM/The CDM/

https://codereview.chromium.org/2837613004/diff/100001/media/filters/audio_de...
media/filters/audio_decoder_selector_unittest.cc:411: // DecryptingAudioDecoder
is blacklisted so we'll fallback to use
s/fallback/fall back

https://codereview.chromium.org/2837613004/diff/100001/media/filters/decoder_...
File media/filters/decoder_selector.cc (right):

https://codereview.chromium.org/2837613004/diff/100001/media/filters/decoder_...
media/filters/decoder_selector.cc:127: DVLOG(1) << __func__ << ": Decryping
decoder is blacklisted.";
decrypting

https://codereview.chromium.org/2837613004/diff/100001/media/filters/decoder_...
media/filters/decoder_selector.cc:253: decoders_.clear();
Potentially, instead of saving decoders_ and making sure we clear it in every
case, we could thread the decoder vector through the InitializeDone callbacks
(remaining_decoders or something).

Actually, not worth doing if you make the change where we use a factory that
creates one decoder at a time.

https://codereview.chromium.org/2837613004/diff/100001/media/filters/decoder_...
File media/filters/decoder_selector.h (right):

https://codereview.chromium.org/2837613004/diff/100001/media/filters/decoder_...
media/filters/decoder_selector.h:43: // TODO(xhwang): Use a DecoderFactory to
create decoder one by one as needed,
s/decoder/decoders/

https://codereview.chromium.org/2837613004/diff/100001/media/filters/decoder_...
media/filters/decoder_selector.h:44: // instead of creating a list of decoders
at once.
s/at once/all at once/

https://codereview.chromium.org/2837613004/diff/100001/media/filters/decoder_...
media/filters/decoder_selector.h:75: //    select from the decoders following
the decoder that was last returned.
Needs updating? And document blacklisted_decoder?

https://codereview.chromium.org/2837613004/diff/100001/media/filters/decoder_...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/2837613004/diff/100001/media/filters/decoder_...
media/filters/decoder_stream.cc:438: // buffer, and we have not falled back
before.
s/and we have not falled/and if we have not fallen/

https://codereview.chromium.org/2837613004/diff/100001/media/filters/decoder_...
media/filters/decoder_stream.cc:439: if (!decoder_produced_a_frame_ &&
!is_on_fallback_decoder_) {
I'm not sure why we can't fall back again? Maybe a comment would help. I
expected it to work like this:

The blacklist is a list (more than one) and its lifetime is the same as the
config: config changes imply the blacklist should be reset.

https://codereview.chromium.org/2837613004/diff/100001/media/filters/decoder_...
File media/filters/decoder_stream.h (right):

https://codereview.chromium.org/2837613004/diff/100001/media/filters/decoder_...
media/filters/decoder_stream.h:217: // Whether we are on a fallback decoder.
Used to make sure we at most fallback
What about instead of "on a fallback decoder" we say "using a fallback decoder"?
s/is_on_fallback_decoder_/using_fallback_decoder_/

s/we at most fallback once/we fall back once at most/

I think this is also a confusing name because after the fallback decoder
produces a frame we reset it to false. But it's still a fallback decoder?

https://codereview.chromium.org/2837613004/diff/100001/media/filters/decoder_...
media/filters/decoder_stream.h:218: // once, to prevent issues like fallback
infinitely.
s/fallback infinitely/infinite fallback/

https://codereview.chromium.org/2837613004/diff/100001/media/filters/fake_vid...
File media/filters/fake_video_decoder.cc (right):

https://codereview.chromium.org/2837613004/diff/100001/media/filters/fake_vid...
media/filters/fake_video_decoder.cc:60: DVLOG(1) << __func__ << "(" <<
decoder_name_ << ")";
nit: this log format is inconsistent with the two above.

[xhwang, 2017-05-04 23:38:36]

Description was changed from

==========
media: Support switching decoders

This CL updates DecoderSelector so that it takes a callback to create
a list of decoders, instead of taking the list of decoders directly.
This allows the DecoderSelector to select a decoder that has been
tried or selected before, such that upon decoder reinitialization error
(e.g. switching from clear to encrypted config), or upon decode error of
the first buffer, we always have the full list of decoders to select
from.

Two mechanisms are added to avoid trying the same failing decoder again:

1. Blacklist

When SelectDecoder() is called due to reinitialization failure, or
decoding failure of the first buffer, the existing decoder is listed as
the "blacklisted decoder" such that DecoderSelector will not even try
it.

There is one exception though. When DecryptingDemuxerStream is selected,
since the input steam is changed (encrypted -> clear), the blacklisted
decoder is ignored. For example, FFmpegVideoDecoder is slected first for
a clear stream. Then on a config change, the stream becomes encrypted so
FFmpegVideoDecoder fails to reinitialize and we need to select a new
decoder. After DecryptingDemuxerStream is selected, we should still be
able to use FFmpegVideoDecoder to decode the decrypted stream.

2. Fall back at most once

If fallback has already happened and decode of the first buffer failed
again, we don't try to fallback again. This is to avoid the infinite
loop of "select decoder 1 -> decode error -> select decoder 2 -> decode
error -> select decoder 1".

BUG=695595
TEST=Updated/Added unittests.
==========

to

==========
media: Support better decoder switching

Today on reinitialization error, or decode error of the first buffer,
DecoderStream will fall back to decoders left in the DecoderSelector.
Since we will have less and less decoders in the DecoderSelector, the
ability to switch decoders is limited.

This CL updates DecoderSelector so that it takes a callback to create
a list of decoders, instead of taking the list of decoders directly.
This allows the DecoderSelector to select a decoder that has been
tried or selected before, such that upon decoder reinitialization error
(e.g. switching from clear to encrypted config), or upon decode error of
the first buffer, we always have the full list of decoders to select
from.

Two mechanisms are added to avoid trying the same failing decoder again:

1. Blacklist

When SelectDecoder() is called due to reinitialization failure, or
decoding failure of the first buffer, the existing decoder is listed as
the "blacklisted decoder" such that DecoderSelector will not even try
it.

There is one exception though. When DecryptingDemuxerStream is selected,
since the input steam is changed (encrypted -> clear), the blacklisted
decoder is ignored. For example, FFmpegVideoDecoder is slected first for
a clear stream. Then on a config change, the stream becomes encrypted so
FFmpegVideoDecoder fails to reinitialize and we need to select a new
decoder. After DecryptingDemuxerStream is selected, we should still be
able to use FFmpegVideoDecoder to decode the decrypted stream.

2. Fall back at most once

If fallback has already happened and decode of the first buffer failed
again, we don't try to fallback again. This is to avoid the infinite
loop of "select decoder 1 -> decode error -> select decoder 2 -> decode
error -> select decoder 1".

BUG=695595
TEST=Updated/Added unittests.
==========

[xhwang, 2017-05-04 23:49:31]

rebase only

[xhwang, 2017-05-04 23:52:28]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 23:53:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xhwang, 2017-05-04 23:53:14]

watk: I addressed all the comments, rebased, and did some small change in
decoder_stream.*. PTAL again!

https://codereview.chromium.org/2837613004/diff/100001/media/filters/audio_de...
File media/filters/audio_decoder_selector_unittest.cc (right):

https://codereview.chromium.org/2837613004/diff/100001/media/filters/audio_de...
media/filters/audio_decoder_selector_unittest.cc:184: // Tests for clear
streams. CDM will not be used for clear streams so
On 2017/05/04 21:35:33, watk wrote:
> s/CDM/The CDM/

Done.

https://codereview.chromium.org/2837613004/diff/100001/media/filters/audio_de...
media/filters/audio_decoder_selector_unittest.cc:411: // DecryptingAudioDecoder
is blacklisted so we'll fallback to use
On 2017/05/04 21:35:33, watk wrote:
> s/fallback/fall back

Done.

https://codereview.chromium.org/2837613004/diff/100001/media/filters/decoder_...
File media/filters/decoder_selector.cc (right):

https://codereview.chromium.org/2837613004/diff/100001/media/filters/decoder_...
media/filters/decoder_selector.cc:127: DVLOG(1) << __func__ << ": Decryping
decoder is blacklisted.";
On 2017/05/04 21:35:33, watk wrote:
> decrypting

Done.

https://codereview.chromium.org/2837613004/diff/100001/media/filters/decoder_...
media/filters/decoder_selector.cc:253: decoders_.clear();
On 2017/05/04 21:35:33, watk wrote:
> Potentially, instead of saving decoders_ and making sure we clear it in every
> case, we could thread the decoder vector through the InitializeDone callbacks
> (remaining_decoders or something).
> 
> Actually, not worth doing if you make the change where we use a factory that
> creates one decoder at a time.

Acknowledged.

https://codereview.chromium.org/2837613004/diff/100001/media/filters/decoder_...
File media/filters/decoder_selector.h (right):

https://codereview.chromium.org/2837613004/diff/100001/media/filters/decoder_...
media/filters/decoder_selector.h:43: // TODO(xhwang): Use a DecoderFactory to
create decoder one by one as needed,
On 2017/05/04 21:35:33, watk wrote:
> s/decoder/decoders/

Done.

https://codereview.chromium.org/2837613004/diff/100001/media/filters/decoder_...
media/filters/decoder_selector.h:44: // instead of creating a list of decoders
at once.
On 2017/05/04 21:35:33, watk wrote:
> s/at once/all at once/

Done.

https://codereview.chromium.org/2837613004/diff/100001/media/filters/decoder_...
media/filters/decoder_selector.h:75: //    select from the decoders following
the decoder that was last returned.
On 2017/05/04 21:35:33, watk wrote:
> Needs updating? And document blacklisted_decoder?

Good catch. Done.

https://codereview.chromium.org/2837613004/diff/100001/media/filters/decoder_...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/2837613004/diff/100001/media/filters/decoder_...
media/filters/decoder_stream.cc:438: // buffer, and we have not falled back
before.
On 2017/05/04 21:35:33, watk wrote:
> s/and we have not falled/and if we have not fallen/

Done.

https://codereview.chromium.org/2837613004/diff/100001/media/filters/decoder_...
media/filters/decoder_stream.cc:439: if (!decoder_produced_a_frame_ &&
!is_on_fallback_decoder_) {
On 2017/05/04 21:35:33, watk wrote:
> I'm not sure why we can't fall back again? Maybe a comment would help. I
> expected it to work like this:
> 
> The blacklist is a list (more than one) and its lifetime is the same as the
> config: config changes imply the blacklist should be reset.

Added comment in the .h file.

https://codereview.chromium.org/2837613004/diff/100001/media/filters/decoder_...
File media/filters/decoder_stream.h (right):

https://codereview.chromium.org/2837613004/diff/100001/media/filters/decoder_...
media/filters/decoder_stream.h:217: // Whether we are on a fallback decoder.
Used to make sure we at most fallback
On 2017/05/04 21:35:33, watk wrote:
> What about instead of "on a fallback decoder" we say "using a fallback
decoder"?
> s/is_on_fallback_decoder_/using_fallback_decoder_/
> 
> s/we at most fallback once/we fall back once at most/
> 
> I think this is also a confusing name because after the fallback decoder
> produces a frame we reset it to false. But it's still a fallback decoder? 

How about has_fallen_back_once_on_decode_error_ ?

I choose this name to make sure we are talking about fallback on decode error,
not reinit error.

Also, I dropped the resetting code. Previously I thought we call fallback on
decode error again after reinit, but it's not the case (controlled by
decoder_produced_a_frame_). We can revisit this later. I have a TODO in the test
about this.

https://codereview.chromium.org/2837613004/diff/100001/media/filters/decoder_...
media/filters/decoder_stream.h:218: // once, to prevent issues like fallback
infinitely.
On 2017/05/04 21:35:33, watk wrote:
> s/fallback infinitely/infinite fallback/

Done.

https://codereview.chromium.org/2837613004/diff/100001/media/filters/fake_vid...
File media/filters/fake_video_decoder.cc (right):

https://codereview.chromium.org/2837613004/diff/100001/media/filters/fake_vid...
media/filters/fake_video_decoder.cc:60: DVLOG(1) << __func__ << "(" <<
decoder_name_ << ")";
On 2017/05/04 21:35:33, watk wrote:
> nit: this log format is inconsistent with the two above.

Done.

[watk, 2017-05-04 23:58:36]

thanks lgtm

https://codereview.chromium.org/2837613004/diff/120001/media/filters/decoder_...
File media/filters/decoder_stream.h (right):

https://codereview.chromium.org/2837613004/diff/120001/media/filters/decoder_...
media/filters/decoder_stream.h:225: bool has_fallen_back_once_on_decode_error_;
+1

https://codereview.chromium.org/2837613004/diff/160001/media/base/mock_filters.h
File media/base/mock_filters.h (right):

https://codereview.chromium.org/2837613004/diff/160001/media/base/mock_filter...
media/base/mock_filters.h:182: explicit MockVideoDecoder(const std::string&
decoder_name = "");
repeating: You could put the default name here instead of "" and defaulting in
GetDisplayName().

[xhwang, 2017-05-05 00:07:47]

sandersd: PTAL again!

https://codereview.chromium.org/2837613004/diff/160001/media/base/mock_filters.h
File media/base/mock_filters.h (right):

https://codereview.chromium.org/2837613004/diff/160001/media/base/mock_filter...
media/base/mock_filters.h:182: explicit MockVideoDecoder(const std::string&
decoder_name = "");
On 2017/05/04 23:58:36, watk wrote:
> repeating: You could put the default name here instead of "" and defaulting in
> GetDisplayName().

Done.

[xhwang, 2017-05-05 00:08:12]

The CQ bit was checked by xhwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-05 00:08:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sandersd (OOO until July 31), 2017-05-05 00:36:47]

lgtm.

https://codereview.chromium.org/2837613004/diff/180001/media/filters/decoder_...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/2837613004/diff/180001/media/filters/decoder_...
media/filters/decoder_stream.cc:444: has_fallen_back_once_on_decode_error_ =
true;
It seems this should be reset after a successful decode?

[commit-bot: I haz the power, 2017-05-05 02:04:56]

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

[commit-bot: I haz the power, 2017-05-05 02:04:57]

Dry run: This issue passed the CQ dry run.

[xhwang, 2017-05-05 02:42:59]

https://codereview.chromium.org/2837613004/diff/180001/media/filters/decoder_...
File media/filters/decoder_stream.cc (right):

https://codereview.chromium.org/2837613004/diff/180001/media/filters/decoder_...
media/filters/decoder_stream.cc:444: has_fallen_back_once_on_decode_error_ =
true;
On 2017/05/05 00:36:46, sandersd wrote:
> It seems this should be reset after a successful decode?

We could do that, but then this name is not accurate, as pointed out by Dale and
Chris.

Also it will not help anything for now, since we never fallback again after
decoder_produced_a_frame_ is true, even after reinitialization. In the next CL,
I'll try to remove this restriction. Basically after reinitialization, we should
be able to fallback again on decode error on the first buffer.

[xhwang, 2017-05-05 02:43:10]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2017-05-05 02:43:11]

The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org
Link to the patchset: https://codereview.chromium.org/2837613004/#ps180001
(title: "Mock*Decoder name")

[commit-bot: I haz the power, 2017-05-05 02:43:43]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 03:01:24]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1493952190933020,
"parent_rev": "6815aefeae88a9e62065ca8084075e3e98dec267", "commit_rev":
"1492be9e0d583b0110f22d7e5f2ded139611f8bd"}

[commit-bot: I haz the power, 2017-05-05 03:04:34]

Description was changed from

==========
media: Support better decoder switching

Today on reinitialization error, or decode error of the first buffer,
DecoderStream will fall back to decoders left in the DecoderSelector.
Since we will have less and less decoders in the DecoderSelector, the
ability to switch decoders is limited.

This CL updates DecoderSelector so that it takes a callback to create
a list of decoders, instead of taking the list of decoders directly.
This allows the DecoderSelector to select a decoder that has been
tried or selected before, such that upon decoder reinitialization error
(e.g. switching from clear to encrypted config), or upon decode error of
the first buffer, we always have the full list of decoders to select
from.

Two mechanisms are added to avoid trying the same failing decoder again:

1. Blacklist

When SelectDecoder() is called due to reinitialization failure, or
decoding failure of the first buffer, the existing decoder is listed as
the "blacklisted decoder" such that DecoderSelector will not even try
it.

There is one exception though. When DecryptingDemuxerStream is selected,
since the input steam is changed (encrypted -> clear), the blacklisted
decoder is ignored. For example, FFmpegVideoDecoder is slected first for
a clear stream. Then on a config change, the stream becomes encrypted so
FFmpegVideoDecoder fails to reinitialize and we need to select a new
decoder. After DecryptingDemuxerStream is selected, we should still be
able to use FFmpegVideoDecoder to decode the decrypted stream.

2. Fall back at most once

If fallback has already happened and decode of the first buffer failed
again, we don't try to fallback again. This is to avoid the infinite
loop of "select decoder 1 -> decode error -> select decoder 2 -> decode
error -> select decoder 1".

BUG=695595
TEST=Updated/Added unittests.
==========

to

==========
media: Support better decoder switching

Today on reinitialization error, or decode error of the first buffer,
DecoderStream will fall back to decoders left in the DecoderSelector.
Since we will have less and less decoders in the DecoderSelector, the
ability to switch decoders is limited.

This CL updates DecoderSelector so that it takes a callback to create
a list of decoders, instead of taking the list of decoders directly.
This allows the DecoderSelector to select a decoder that has been
tried or selected before, such that upon decoder reinitialization error
(e.g. switching from clear to encrypted config), or upon decode error of
the first buffer, we always have the full list of decoders to select
from.

Two mechanisms are added to avoid trying the same failing decoder again:

1. Blacklist

When SelectDecoder() is called due to reinitialization failure, or
decoding failure of the first buffer, the existing decoder is listed as
the "blacklisted decoder" such that DecoderSelector will not even try
it.

There is one exception though. When DecryptingDemuxerStream is selected,
since the input steam is changed (encrypted -> clear), the blacklisted
decoder is ignored. For example, FFmpegVideoDecoder is slected first for
a clear stream. Then on a config change, the stream becomes encrypted so
FFmpegVideoDecoder fails to reinitialize and we need to select a new
decoder. After DecryptingDemuxerStream is selected, we should still be
able to use FFmpegVideoDecoder to decode the decrypted stream.

2. Fall back at most once

If fallback has already happened and decode of the first buffer failed
again, we don't try to fallback again. This is to avoid the infinite
loop of "select decoder 1 -> decode error -> select decoder 2 -> decode
error -> select decoder 1".

BUG=695595
TEST=Updated/Added unittests.

Review-Url: https://codereview.chromium.org/2837613004
Cr-Commit-Position: refs/heads/master@{#469579}
Committed:
https://chromium.googlesource.com/chromium/src/+/1492be9e0d583b0110f22d7e5f2d...
==========

[commit-bot: I haz the power, 2017-05-05 03:04:35]

Committed patchset #9 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/1492be9e0d583b0110f22d7e5f2d...