Create abstract interface for media::Pipeline.

Create abstract interface for media::Pipeline.

This changes media::Pipeline to be an abstract base, and moves the implementation into media::PipelineImpl. This will make it possible to mock Pipeline in tests.

Committed: https://crrev.com/c82a278bd475fd1a68d0b9452e028537f4263043
Cr-Commit-Position: refs/heads/master@{#377035}

[sandersd (OOO until July 31), 2016-02-02 19:57:41]

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

[sandersd (OOO until July 31), 2016-02-02 19:57:57]

Description was changed from

==========
Make Pipeline methods virtual.

This will let us mock Pipeline in tests.
==========

to

==========
Make media::Pipeline methods virtual.

This will let us mock media::Pipeline in tests.
==========

[sandersd (OOO until July 31), 2016-02-04 22:00:52]

Description was changed from

==========
Make media::Pipeline methods virtual.

This will let us mock media::Pipeline in tests.
==========

to

==========
Create abstract interface for media::Pipeline.

This changes media::Pipeline to be an abstract base, and moves the
implementation into media::PipelineImpl. This will make it possible to mock
Pipeline in tests.
==========

[sandersd (OOO until July 31), 2016-02-17 00:14:48]

After discussion with xhwang@, we have agreed to this style of refactor for
Pipeline, although with final naming TBD.

PTAL.

[DaleCurtis, 2016-02-17 00:19:11]

sgtm!

https://codereview.chromium.org/1658303002/diff/20001/media/base/pipeline.h
File media/base/pipeline.h (right):

https://codereview.chromium.org/1658303002/diff/20001/media/base/pipeline.h#n...
media/base/pipeline.h:79: scoped_ptr<Renderer>* renderer,
Remind me, what was the reason for this change? I think this defeats the point
of a scoped_ptr for readability if multiple classes have a pointer to it :)

[sandersd (OOO until July 31), 2016-02-18 19:14:47]

https://codereview.chromium.org/1658303002/diff/20001/media/base/pipeline.h
File media/base/pipeline.h (right):

https://codereview.chromium.org/1658303002/diff/20001/media/base/pipeline.h#n...
media/base/pipeline.h:79: scoped_ptr<Renderer>* renderer,
On 2016/02/17 00:19:11, DaleCurtis wrote:
> Remind me, what was the reason for this change? I think this defeats the point
> of a scoped_ptr for readability if multiple classes have a pointer to it :)

The ownership of |renderer| is already pretty messed up :-(. I'm starting to
think it should have been a refcounted object (despite the fact that single
ownership has been working).

This specific change is required to be able to mock Pipeline::Start().

[sandersd (OOO until July 31), 2016-02-19 01:03:55]

scoped_ptr pointers removed, PTAL.

[DaleCurtis, 2016-02-19 01:06:50]

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

[DaleCurtis, 2016-02-19 01:06:51]

dalecurtis@chromium.org changed required reviewers:
+ xhwang@chromium.org

[DaleCurtis, 2016-02-19 01:06:52]

+xhwang since I won't have a chance to look over this today.

[xhwang, 2016-02-19 17:06:08]

Overall looks good!

I think we should take this opportunity take a more closer look at our pipeline
interface and make it clear when a method can or cannot be called. If a method
can be called at any time we should also make it clear. The rest of media/ often
assumes that you can only call something after the previous call finished (the
completion callback fired), except for stop/destroy/destruction.

https://chromiumcodereview.appspot.com/1658303002/diff/60001/media/base/pipel...
File media/base/pipeline.h (right):

https://chromiumcodereview.appspot.com/1658303002/diff/60001/media/base/pipel...
media/base/pipeline.h:60: // a filter chain, executing |seek_cb| when the
initial seek has completed.
Can we change the |seek_cb| to something like a |start_cb|? So the |start_cb|
will be fired after the pipeline is fully started (including initial seek
completed).

Hmm, I saw Resume() is using the |seek_cb| as well. So maybe this is okay.

https://chromiumcodereview.appspot.com/1658303002/diff/60001/media/base/pipel...
media/base/pipeline.h:82: const PipelineStatusCB& seek_cb,
This callback is a special one time callback. Maybe move it up to be the first
callback (after |renderer|)?

https://chromiumcodereview.appspot.com/1658303002/diff/60001/media/base/pipel...
media/base/pipeline.h:105: // is suspended.
What if Suspend() has been called but |suspend_cb| isn't fired yet. Can we call
Seek() at that time?

Can we call Seek() while we are seeking? We should make this clear as well.

https://chromiumcodereview.appspot.com/1658303002/diff/60001/media/base/pipel...
media/base/pipeline.h:108: // Suspend the pipeline, discarding the current
renderer.
Suspend_s_, same everywhere else.

https://chromiumcodereview.appspot.com/1658303002/diff/60001/media/base/pipel...
media/base/pipeline.h:117: // Resume the pipeline with a new renderer, and
initialize it with a seek.
When can you call resume?

https://chromiumcodereview.appspot.com/1658303002/diff/60001/media/base/pipel...
media/base/pipeline.h:125: virtual bool IsRunning() const = 0;
The caller should know whether the pipeline is running since it is the caller of
Start()?

https://chromiumcodereview.appspot.com/1658303002/diff/60001/media/base/pipel...
media/base/pipeline.h:131: virtual double GetPlaybackRate() const = 0;
The caller called SetPlaybackRate(), shouldn't it already know the rate? Or is
this just for convenience?

https://chromiumcodereview.appspot.com/1658303002/diff/60001/media/base/pipel...
media/base/pipeline.h:212: class MEDIA_EXPORT PipelineImpl : public Pipeline,
public DemuxerHost {
Should we put this in a different file?

https://chromiumcodereview.appspot.com/1658303002/diff/60001/media/base/pipel...
media/base/pipeline.h:222: // PipelineInterface implementation.
s/PipelineInterface/Pipeline/

https://chromiumcodereview.appspot.com/1658303002/diff/60001/media/base/pipel...
File media/base/pipeline_unittest.cc (right):

https://chromiumcodereview.appspot.com/1658303002/diff/60001/media/base/pipel...
media/base/pipeline_unittest.cc:99: PipelineTest()
Should this be PipelineImplTest?

[DaleCurtis, 2016-02-19 23:19:45]

I think we should keep these as a purely mechanical change before tweaking any
APIs. It'll be hard to review otherwise.

lgtm % moving PipelineImpl into pipeline_impl.{cc,h}

https://codereview.chromium.org/1658303002/diff/60001/media/base/pipeline.h
File media/base/pipeline.h (right):

https://codereview.chromium.org/1658303002/diff/60001/media/base/pipeline.h#n...
media/base/pipeline.h:212: class MEDIA_EXPORT PipelineImpl : public Pipeline,
public DemuxerHost {
On 2016/02/19 at 17:06:07, xhwang wrote:
> Should we put this in a different file?

+1

[sandersd (OOO until July 31), 2016-02-22 22:32:57]

https://codereview.chromium.org/1658303002/diff/60001/media/base/pipeline.h
File media/base/pipeline.h (right):

https://codereview.chromium.org/1658303002/diff/60001/media/base/pipeline.h#n...
media/base/pipeline.h:105: // is suspended.
On 2016/02/19 17:06:07, xhwang wrote:
> What if Suspend() has been called but |suspend_cb| isn't fired yet. Can we
call
> Seek() at that time?
> 
> Can we call Seek() while we are seeking? We should make this clear as well.

Done.

https://codereview.chromium.org/1658303002/diff/60001/media/base/pipeline.h#n...
media/base/pipeline.h:108: // Suspend the pipeline, discarding the current
renderer.
On 2016/02/19 17:06:07, xhwang wrote:
> Suspend_s_, same everywhere else.

Done.

https://codereview.chromium.org/1658303002/diff/60001/media/base/pipeline.h#n...
media/base/pipeline.h:117: // Resume the pipeline with a new renderer, and
initialize it with a seek.
On 2016/02/19 17:06:07, xhwang wrote:
> When can you call resume?

Done.

https://codereview.chromium.org/1658303002/diff/60001/media/base/pipeline.h#n...
media/base/pipeline.h:125: virtual bool IsRunning() const = 0;
On 2016/02/19 17:06:07, xhwang wrote:
> The caller should know whether the pipeline is running since it is the caller
of
> Start()?

That's true, but this method is currently used in WMPI in the suspend logic.
That will be going away in the WMPI refactor CL, so if we also remove calls to
this method from tests we can get rid of it after that.

https://codereview.chromium.org/1658303002/diff/60001/media/base/pipeline.h#n...
media/base/pipeline.h:212: class MEDIA_EXPORT PipelineImpl : public Pipeline,
public DemuxerHost {
On 2016/02/19 17:06:07, xhwang wrote:
> Should we put this in a different file?

Done.

https://codereview.chromium.org/1658303002/diff/60001/media/base/pipeline.h#n...
media/base/pipeline.h:222: // PipelineInterface implementation.
On 2016/02/19 17:06:07, xhwang wrote:
> s/PipelineInterface/Pipeline/

Done.

https://codereview.chromium.org/1658303002/diff/60001/media/base/pipeline_uni...
File media/base/pipeline_unittest.cc (right):

https://codereview.chromium.org/1658303002/diff/60001/media/base/pipeline_uni...
media/base/pipeline_unittest.cc:99: PipelineTest()
On 2016/02/19 17:06:07, xhwang wrote:
> Should this be PipelineImplTest?

Done.

[xhwang, 2016-02-22 22:47:13]

lgtm % one discussion

I like the Pipeline/PipelineImpl separation. But it's kinda sad to see the
integration test now named PipelineImplIntegrationTest. The name is kinda of
unnecessarily long given that we only have one real Pipeline implementation.

WDYT?

If we stay with this new name, we should fix the mojo one as well:

https://code.google.com/p/chromium/codesearch#search/&q=mojo%20pipeline_integ...

[sandersd (OOO until July 31), 2016-02-22 22:58:50]

On 2016/02/22 22:47:13, xhwang wrote:
> lgtm % one discussion
> 
> I like the Pipeline/PipelineImpl separation. But it's kinda sad to see the
> integration test now named PipelineImplIntegrationTest. The name is kinda of
> unnecessarily long given that we only have one real Pipeline implementation.
> 
> WDYT?

I think that after PipelineImpl is refactored, it may make more sense to write
these against PipelineController. I'm fine naming them back to
PipelineIntegration if that's simpler.

[sandersd (OOO until July 31), 2016-02-23 00:49:38]

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

[commit-bot: I haz the power, 2016-02-23 00:52:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1658303002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1658303002/180001

[commit-bot: I haz the power, 2016-02-23 01:20:03]

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

[commit-bot: I haz the power, 2016-02-23 01:20:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[sandersd (OOO until July 31), 2016-02-23 01:51:23]

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

[commit-bot: I haz the power, 2016-02-23 01:51:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1658303002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1658303002/200001

[commit-bot: I haz the power, 2016-02-23 03:29:37]

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

[commit-bot: I haz the power, 2016-02-23 03:29:38]

Dry run: This issue passed the CQ dry run.

[sandersd (OOO until July 31), 2016-02-23 18:44:33]

The CQ bit was checked by sandersd@chromium.org

[sandersd (OOO until July 31), 2016-02-23 18:44:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org, xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1658303002/#ps200001
(title: "Fix FakeMediaSource includes.")

[commit-bot: I haz the power, 2016-02-23 18:46:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1658303002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1658303002/200001

[commit-bot: I haz the power, 2016-02-23 19:15:38]

Description was changed from

==========
Create abstract interface for media::Pipeline.

This changes media::Pipeline to be an abstract base, and moves the
implementation into media::PipelineImpl. This will make it possible to mock
Pipeline in tests.
==========

to

==========
Create abstract interface for media::Pipeline.

This changes media::Pipeline to be an abstract base, and moves the
implementation into media::PipelineImpl. This will make it possible to mock
Pipeline in tests.
==========

[commit-bot: I haz the power, 2016-02-23 19:15:39]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-02-23 19:16:35]

Description was changed from

==========
Create abstract interface for media::Pipeline.

This changes media::Pipeline to be an abstract base, and moves the
implementation into media::PipelineImpl. This will make it possible to mock
Pipeline in tests.
==========

to

==========
Create abstract interface for media::Pipeline.

This changes media::Pipeline to be an abstract base, and moves the
implementation into media::PipelineImpl. This will make it possible to mock
Pipeline in tests.

Committed: https://crrev.com/c82a278bd475fd1a68d0b9452e028537f4263043
Cr-Commit-Position: refs/heads/master@{#377035}
==========

[commit-bot: I haz the power, 2016-02-23 19:16:36]

Patchset 11 (id:??) landed as
https://crrev.com/c82a278bd475fd1a68d0b9452e028537f4263043
Cr-Commit-Position: refs/heads/master@{#377035}