media: Refactor VideoFrameStreamTest

media: Refactor VideoFrameStreamTest

This is in preparation for updating/adding tests for the change to support
run-time switch of decoders.

- Move decoders to be selected to a vector to avoid duplicate code.
- Add |decoder_| to track the currently selected decoder.
- Remove DECODER_INIT pending state since this is rarely used.
- Allow FakeVideoDecoder to use different display names. This is needed
  to check what decoder has been selected.

BUG=695595
TEST=Refactor only. No changes to existing tests.

Review-Url: https://codereview.chromium.org/2835203006
Cr-Commit-Position: refs/heads/master@{#467809}
Committed: https://chromium.googlesource.com/chromium/src/+/92607542c7cfde619e78d1b29490b1e1a13bb2da

[xhwang, 2017-04-26 07:28:00]

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

[commit-bot: I haz the power, 2017-04-26 07:28:26]

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-04-26 09:10:28]

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

[commit-bot: I haz the power, 2017-04-26 09:10:29]

Dry run: This issue passed the CQ dry run.

[xhwang, 2017-04-26 14:44:53]

Description was changed from

==========
media: Refactor VideoFrameStreamTest

This is in preparation for updating the tests for the change to support
run-time switch of decoders.

- Move decoders to be selected to a vector to avoid duplicate code.
- Add |decoder_| to track the currently selected decoder.
- Remove DECODER_INIT pending state since this is rarely used.
- Allow FakeVideoDecoder to use different display names. This is needed
  to check what decoder has been selected.

BUG=695595
TEST=Refactor only. No changes to existing tests.
==========

to

==========
media: Refactor VideoFrameStreamTest

This is in preparation for updating/adding tests for the change to support
run-time switch of decoders.

- Move decoders to be selected to a vector to avoid duplicate code.
- Add |decoder_| to track the currently selected decoder.
- Remove DECODER_INIT pending state since this is rarely used.
- Allow FakeVideoDecoder to use different display names. This is needed
  to check what decoder has been selected.

BUG=695595
TEST=Refactor only. No changes to existing tests.
==========

[xhwang, 2017-04-26 14:44:53]

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

[xhwang, 2017-04-26 14:45:39]

dalecurtis / sandersd: PTAL

[DaleCurtis, 2017-04-26 19:41:15]

lgtm

https://codereview.chromium.org/2835203006/diff/1/media/filters/decoder_stream.h
File media/filters/decoder_stream.h (right):

https://codereview.chromium.org/2835203006/diff/1/media/filters/decoder_strea...
media/filters/decoder_stream.h:116: typedef
base::RepeatingCallback<void(Decoder*)> DecoderChangeObserverCB;
using.

https://codereview.chromium.org/2835203006/diff/1/media/filters/decoder_strea...
media/filters/decoder_stream.h:118: const DecoderChangeObserverCB&
decoder_change_observer_cb) {
I believe std::move() is preferred now.

https://codereview.chromium.org/2835203006/diff/1/media/filters/video_frame_s...
File media/filters/video_frame_stream_unittest.cc (right):

https://codereview.chromium.org/2835203006/diff/1/media/filters/video_frame_s...
media/filters/video_frame_stream_unittest.cc:34: namespace {
no anonymous namespace in media generally. use static or constexpr.

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

https://codereview.chromium.org/2835203006/diff/1/media/filters/fake_video_de...
File media/filters/fake_video_decoder.h (right):

https://codereview.chromium.org/2835203006/diff/1/media/filters/fake_video_de...
media/filters/fake_video_decoder.h:37: FakeVideoDecoder(const std::string&
decoder_name,
I am still not comfortable with this concept. I'm also seeing that the
MediaCapabilities work is going to need some way to reference specific decoders
without necessarily having an instance of them.

At the same time, we have other "static" decoder data that is not currently
guaranteed to actually be static. I think we need a new concept that identifies
a decoder implementation and guarantees to answer static queries statically. If
that concept also provides a factory, it could replace our decoder factory
callbacks too, while simultaneously providing "names" for them in the sense of
operator==.

Of course what we really want is metaclasses, but I don't know a good way to
fake that directly in C++.

[sandersd (OOO until July 31), 2017-04-26 21:46:48]

https://codereview.chromium.org/2835203006/diff/1/media/filters/fake_video_de...
File media/filters/fake_video_decoder.h (right):

https://codereview.chromium.org/2835203006/diff/1/media/filters/fake_video_de...
media/filters/fake_video_decoder.h:37: FakeVideoDecoder(const std::string&
decoder_name,
On 2017/04/26 20:11:05, sandersd wrote:
> I am still not comfortable with this concept. I'm also seeing that the
> MediaCapabilities work is going to need some way to reference specific
decoders
> without necessarily having an instance of them.
> 
> At the same time, we have other "static" decoder data that is not currently
> guaranteed to actually be static. I think we need a new concept that
identifies
> a decoder implementation and guarantees to answer static queries statically.
If
> that concept also provides a factory, it could replace our decoder factory
> callbacks too, while simultaneously providing "names" for them in the sense of
> operator==.
> 
> Of course what we really want is metaclasses, but I don't know a good way to
> fake that directly in C++.

After taking this through in SEA, here is what we came up with:
  - There doesn't seem to be any reason we can't create decoders on the media
thread, and the fact that we are using a list of instances already implies that
Decoder constructors are not allowed to do much.
  - If you use a list of create callbacks rather than instances, there is no
need for a blacklist--you can remove callbacks from your local list, but not
remove callbacks for Initialize()d classes.
  - Callbacks already have operator== if that is useful to you.
  - DecoderSelector should be maintaining the blacklist, this doesn't seem to be
the caller's concern. (Possibly config changes should be plumbed explicitly?)

I should move this conversation back to the other CL, but basically the proposal
is 'use factory callbacks instead of instances'. If there are no fundamental
blockers to that, it seems to be a lot simpler.

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

On 2017/04/26 21:46:48, sandersd wrote:
>
https://codereview.chromium.org/2835203006/diff/1/media/filters/fake_video_de...
> File media/filters/fake_video_decoder.h (right):
> 
>
https://codereview.chromium.org/2835203006/diff/1/media/filters/fake_video_de...
> media/filters/fake_video_decoder.h:37: FakeVideoDecoder(const std::string&
> decoder_name,
> On 2017/04/26 20:11:05, sandersd wrote:
> > I am still not comfortable with this concept. I'm also seeing that the
> > MediaCapabilities work is going to need some way to reference specific
> decoders
> > without necessarily having an instance of them.
> > 
> > At the same time, we have other "static" decoder data that is not currently
> > guaranteed to actually be static. I think we need a new concept that
> identifies
> > a decoder implementation and guarantees to answer static queries statically.
> If
> > that concept also provides a factory, it could replace our decoder factory
> > callbacks too, while simultaneously providing "names" for them in the sense
of
> > operator==.
> > 
> > Of course what we really want is metaclasses, but I don't know a good way to
> > fake that directly in C++.
> 
> After taking this through in SEA, here is what we came up with:
>   - There doesn't seem to be any reason we can't create decoders on the media
> thread, and the fact that we are using a list of instances already implies
that
> Decoder constructors are not allowed to do much.
>   - If you use a list of create callbacks rather than instances, there is no
> need for a blacklist--you can remove callbacks from your local list, but not
> remove callbacks for Initialize()d classes.
>   - Callbacks already have operator== if that is useful to you.
>   - DecoderSelector should be maintaining the blacklist, this doesn't seem to
be
> the caller's concern. (Possibly config changes should be plumbed explicitly?)
> 
> I should move this conversation back to the other CL, but basically the
proposal
> is 'use factory callbacks instead of instances'. If there are no fundamental
> blockers to that, it seems to be a lot simpler.

lgtm (c.f. https://codereview.chromium.org/2837613004/)

[xhwang, 2017-04-27 20:53:01]

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

[commit-bot: I haz the power, 2017-04-27 20:53:45]

Dry run: CQ is trying da patch.

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

[xhwang, 2017-04-27 21:11:15]

https://codereview.chromium.org/2835203006/diff/1/media/filters/decoder_stream.h
File media/filters/decoder_stream.h (right):

https://codereview.chromium.org/2835203006/diff/1/media/filters/decoder_strea...
media/filters/decoder_stream.h:116: typedef
base::RepeatingCallback<void(Decoder*)> DecoderChangeObserverCB;
On 2017/04/26 19:41:14, DaleCurtis wrote:
> using.

Done.

https://codereview.chromium.org/2835203006/diff/1/media/filters/decoder_strea...
media/filters/decoder_stream.h:118: const DecoderChangeObserverCB&
decoder_change_observer_cb) {
On 2017/04/26 19:41:14, DaleCurtis wrote:
> I believe std::move() is preferred now.

Done.

https://codereview.chromium.org/2835203006/diff/1/media/filters/video_frame_s...
File media/filters/video_frame_stream_unittest.cc (right):

https://codereview.chromium.org/2835203006/diff/1/media/filters/video_frame_s...
media/filters/video_frame_stream_unittest.cc:34: namespace {
On 2017/04/26 19:41:14, DaleCurtis wrote:
> no anonymous namespace in media generally. use static or constexpr.

Done.

[xhwang, 2017-04-27 21:11:21]

The CQ bit was unchecked by xhwang@chromium.org

[xhwang, 2017-04-27 21:11:21]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2017-04-27 21:11:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org, sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/2835203006/#ps20001
(title: "comments")

[commit-bot: I haz the power, 2017-04-27 21:11:48]

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-04-28 01:44:37]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493327481939550,
"parent_rev": "b8289814a679b6c85d497fba56e5e74c14ddee5b", "commit_rev":
"92607542c7cfde619e78d1b29490b1e1a13bb2da"}

[commit-bot: I haz the power, 2017-04-28 01:44:47]

Description was changed from

==========
media: Refactor VideoFrameStreamTest

This is in preparation for updating/adding tests for the change to support
run-time switch of decoders.

- Move decoders to be selected to a vector to avoid duplicate code.
- Add |decoder_| to track the currently selected decoder.
- Remove DECODER_INIT pending state since this is rarely used.
- Allow FakeVideoDecoder to use different display names. This is needed
  to check what decoder has been selected.

BUG=695595
TEST=Refactor only. No changes to existing tests.
==========

to

==========
media: Refactor VideoFrameStreamTest

This is in preparation for updating/adding tests for the change to support
run-time switch of decoders.

- Move decoders to be selected to a vector to avoid duplicate code.
- Add |decoder_| to track the currently selected decoder.
- Remove DECODER_INIT pending state since this is rarely used.
- Allow FakeVideoDecoder to use different display names. This is needed
  to check what decoder has been selected.

BUG=695595
TEST=Refactor only. No changes to existing tests.

Review-Url: https://codereview.chromium.org/2835203006
Cr-Commit-Position: refs/heads/master@{#467809}
Committed:
https://chromium.googlesource.com/chromium/src/+/92607542c7cfde619e78d1b29490...
==========

[commit-bot: I haz the power, 2017-04-28 01:44:48]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/92607542c7cfde619e78d1b29490...