This is the frozen interface definition for the media pipeline, filters, and ...

This is the frozen interface definition for the media pipeline, filters, and filter hosts.
It has been pre-code-reviewed by scherkus already.  There is no actual implementation in
this change -- only an empty skeleton for every public method is provided.  The actual code
is ready, but this change is intended to get buy-off for the public interface.

I added pipeline_impl.cc to the media_lib.scons since it was missing (it was present in the
.sln file already)

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=8396

[ralphl, 2009-01-20 23:29:48]

Carlos, 
These are the final changes to the video pipeline and filter interfaces.  The
code blocks are all NOTIMIPLEMENTED in this change, but the actual code exists,
and I'd like to start getting it checked in ASAP.

scherkus has already reviewed the changes, and our entire team has agreed on the
interfaces.  Once we get this checked in, I'll send a change with actual code.

For some reason, lint is continuing to complain about the lack of a newline at
the end of Pipeline.h, even though the file has a newline.  I'm not sure if it
has something to do with gcl or if it is a bug in lint.  Any help on resolving
that would be appreciated.

[cpu_(ooo_6.6-7.5), 2009-01-21 03:51:20]

LG

Some minor comments below.

http://codereview.chromium.org/18380/diff/224/231
File media/base/filter_host.h (right):

http://codereview.chromium.org/18380/diff/224/231#newcode38
Line 38: 
Who is the owner of this PipelineStatus pointer? is not clear from the headers
or maybe a missed it?

http://codereview.chromium.org/18380/diff/224/230
File media/base/filters.h (right):

http://codereview.chromium.org/18380/diff/224/230#newcode68
Line 68: 
end bracket on its own line.

http://codereview.chromium.org/18380/diff/224/228
File media/base/pipeline.h (right):

http://codereview.chromium.org/18380/diff/224/228#newcode12
Line 12: #include <string>
space after header groups.

http://codereview.chromium.org/18380/diff/224/228#newcode26
Line 26: };
Be careful with generic names on enums. They might clash with microsoft macro
expansions. I sugest some prefix like in the previous version.

http://codereview.chromium.org/18380/diff/224/228#newcode94
Line 94: //
One will hope they don't poll. We shall discourage that.

http://codereview.chromium.org/18380/diff/224/228#newcode139
Line 139: virtual bool SetVolume(float volume) = 0;
The volume does not consider multi-channel?

[ralphl, 2009-01-21 19:48:04]

I've made updates and fixed all the problems Carlos pointed out.  I have also
fixed the silly whitespace problem with this upload, but want to make sure there
are no further comments before uploading again.

Carlos, do you know how to get rid of the "no newline at end of file" problem? 
There is a newline there.  Is this just a known problem?

Thanks!

http://codereview.chromium.org/18380/diff/224/231
File media/base/filter_host.h (right):

http://codereview.chromium.org/18380/diff/224/231#newcode38
Line 38: 
On 2009/01/21 03:51:20, cpu wrote:
> Who is the owner of this PipelineStatus pointer? is not clear from the headers
> or maybe a missed it?

I have updated the comments.  It's just a pointer to the pipeline object without
the ability to access the client methods Start/Stop/Seek/SetVolume/SetRate.  I
hope this is more clear:

The PipelineStatus class allows read-only access to the pipeline state. This is
the same object that is used by the pipeline client to examine the state of the
running pipeline.  The lifetime of the PipelineStatus interface is the same as
the lifetime of the FilterHost interface, so it is acceptable for filters to use
the returned pointer until their Stop method has been called.

http://codereview.chromium.org/18380/diff/224/230
File media/base/filters.h (right):

http://codereview.chromium.org/18380/diff/224/230#newcode68
Line 68: 
On 2009/01/21 03:51:20, cpu wrote:
> end bracket on its own line.

Done.

http://codereview.chromium.org/18380/diff/224/228
File media/base/pipeline.h (right):

http://codereview.chromium.org/18380/diff/224/228#newcode12
Line 12: #include <string>
On 2009/01/21 03:51:20, cpu wrote:
> space after header groups.

Done.

http://codereview.chromium.org/18380/diff/224/228#newcode26
Line 26: };
On 2009/01/21 03:51:20, cpu wrote:
> Be careful with generic names on enums. They might clash with microsoft macro
> expansions. I sugest some prefix like in the previous version.

I put PIPELINE_ in front of every emun to avoid conflicts (ie, PIPELINE_OK,
PIPELINE_ERROR_ABORT, etc...)

http://codereview.chromium.org/18380/diff/224/228#newcode94
Line 94: //
On 2009/01/21 03:51:20, cpu wrote:
> One will hope they don't poll. We shall discourage that.

Added that we discourage polling to the comment block.

http://codereview.chromium.org/18380/diff/224/228#newcode139
Line 139: virtual bool SetVolume(float volume) = 0;
On 2009/01/21 03:51:20, cpu wrote:
> The volume does not consider multi-channel?

No the volume does not consider multi-channel.  I have never seen a media player
that has a mixer/fader control built in.  If the user wants to control this,
they would need to use a system control panel.  HTML 5.0 does not address
multi-channel either.  I'll add this to the comments:

This value affects all channels proportionately for multi-channel audio streams.

[scherkus (not reviewing), 2009-01-21 21:58:15]

Tiny style nits and a comment about the enum.

http://codereview.chromium.org/18380/diff/238/241
File media/base/filter_host_impl.h (right):

http://codereview.chromium.org/18380/diff/238/241#newcode19
Line 19: // FilterHost interface
Period at the end of comments.

http://codereview.chromium.org/18380/diff/238/241#newcode34
Line 34: DISALLOW_COPY_AND_ASSIGN(FilterHostImpl);
DISALLOW_COPY_AND_ASSIGN should be under private.

http://codereview.chromium.org/18380/diff/238/244
File media/base/filters.h (right):

http://codereview.chromium.org/18380/diff/238/244#newcode28
Line 28: #include <string>
Could you put a space between the headers here?  Looks like I forgot to do it :)

http://codereview.chromium.org/18380/diff/238/244#newcode30
Line 30: #include "base/logging.h"
Includes should be in alphabetical order.

http://codereview.chromium.org/18380/diff/238/244#newcode86
Line 86: private:
Add an extra line before private declaration.

http://codereview.chromium.org/18380/diff/238/244#newcode196
Line 196: #endif // MEDIA_BASE_FILTERS_H_
Could you make sure there are two spaces before the namespace and #endif
comments?

Looks like I didn't catch myself when checking this in :)

http://codereview.chromium.org/18380/diff/238/242
File media/base/pipeline.h (right):

http://codereview.chromium.org/18380/diff/238/242#newcode20
Line 20: enum PipelineErrors {
Looking through the rest of the Chrome source code, it looks like we use
singular enum definitions (PipelineError).

Also looking through the source code, it might make sense to call this
PipelineStatus.  Your call!

http://codereview.chromium.org/18380/diff/238/242#newcode146
Line 146: #endif  // MEDIA_BASE_PIPELINE_H_
Reitveld is showing there's a ton of trailing whitespace for this line.  I have
a feeling it's just a Reitveld bug.

http://codereview.chromium.org/18380/diff/238/247
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/18380/diff/238/247#newcode10
Line 10: #include <string>
Space between header groups.

[ralphl, 2009-01-21 22:15:41]

Fixed scherkus comments

http://codereview.chromium.org/18380/diff/238/241
File media/base/filter_host_impl.h (right):

http://codereview.chromium.org/18380/diff/238/241#newcode19
Line 19: // FilterHost interface
On 2009/01/21 21:58:16, scherkus wrote:
> Period at the end of comments.

Done.

http://codereview.chromium.org/18380/diff/238/241#newcode34
Line 34: DISALLOW_COPY_AND_ASSIGN(FilterHostImpl);
On 2009/01/21 21:58:16, scherkus wrote:
> DISALLOW_COPY_AND_ASSIGN should be under private.

Done.

http://codereview.chromium.org/18380/diff/238/244
File media/base/filters.h (right):

http://codereview.chromium.org/18380/diff/238/244#newcode28
Line 28: #include <string>
On 2009/01/21 21:58:16, scherkus wrote:
> Could you put a space between the headers here?  Looks like I forgot to do it
:)

Done.

http://codereview.chromium.org/18380/diff/238/244#newcode30
Line 30: #include "base/logging.h"
On 2009/01/21 21:58:16, scherkus wrote:
> Includes should be in alphabetical order.

Done.

http://codereview.chromium.org/18380/diff/238/244#newcode30
Line 30: #include "base/logging.h"
On 2009/01/21 21:58:16, scherkus wrote:
> Includes should be in alphabetical order.

Done.

http://codereview.chromium.org/18380/diff/238/244#newcode86
Line 86: private:
On 2009/01/21 21:58:16, scherkus wrote:
> Add an extra line before private declaration.

Done.

http://codereview.chromium.org/18380/diff/238/242
File media/base/pipeline.h (right):

http://codereview.chromium.org/18380/diff/238/242#newcode20
Line 20: enum PipelineErrors {
On 2009/01/21 21:58:16, scherkus wrote:
> Looking through the rest of the Chrome source code, it looks like we use
> singular enum definitions (PipelineError).
> 
> Also looking through the source code, it might make sense to call this
> PipelineStatus.  Your call!

Done.

http://codereview.chromium.org/18380/diff/238/242#newcode146
Line 146: #endif  // MEDIA_BASE_PIPELINE_H_
On 2009/01/21 21:58:16, scherkus wrote:
> Reitveld is showing there's a ton of trailing whitespace for this line.  I
have
> a feeling it's just a Reitveld bug.

Done.

http://codereview.chromium.org/18380/diff/238/247
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/18380/diff/238/247#newcode10
Line 10: #include <string>
On 2009/01/21 21:58:16, scherkus wrote:
> Space between header groups.

Done.

[scherkus (not reviewing), 2009-01-21 22:24:45]

LGTM

Just fix that one tiny pure virtual thing.

http://codereview.chromium.org/18380/diff/54/265
File media/base/filters.h (right):

http://codereview.chromium.org/18380/diff/54/265#newcode144
Line 144: virtual ~DemuxerStream() {}
This should be protected pure virtual.