Big media::Pipeline cleanup.

Big media::Pipeline cleanup.

Before I can even start refactoring, I need to remove a lot of accumulated cruft and fix some style stuff.  PipelineStatus has been merged into Pipeline and I got rid of GetInterpolatedTime() as well as the ability to schedule callbacks when time has updated.  We haven't found a need for these features and they introduced a good amount of code complexity.

TEST=media_unittests should still pass
BUG=16008

[Alpha Left Google, 2009-07-06 23:41:14]

http://codereview.chromium.org/149215/diff/1002/16
File media/base/filter_host.h (right):

http://codereview.chromium.org/149215/diff/1002/16#newcode36
Line 36: // Gets the current pipeline time in microseconds.
nit: current playback time or maybe just current time

http://codereview.chromium.org/149215/diff/1002/16#newcode39
Line 39: // Sets the current time.  Other filters should poll to examine the
updated
nit: Should say this updates the current time the pipeline.

http://codereview.chromium.org/149215/diff/1002/21
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/149215/diff/1002/21#newcode323
Line 323: void PipelineThread::InitializationComplete(FilterHostImpl* host) {
This method is also called from client's thread.

http://codereview.chromium.org/149215/diff/1002/21#newcode355
Line 355: STLDeleteElements(&filter_hosts_);
Can you explain why you simple cleanup the container but not calling to Stop()?

[scherkus (not reviewing), 2009-07-08 02:02:54]

http://codereview.chromium.org/149215/diff/1002/16
File media/base/filter_host.h (right):

http://codereview.chromium.org/149215/diff/1002/16#newcode36
Line 36: // Gets the current pipeline time in microseconds.
On 2009/07/06 23:41:14, Alpha wrote:
> nit: current playback time or maybe just current time

Done.

http://codereview.chromium.org/149215/diff/1002/16#newcode39
Line 39: // Sets the current time.  Other filters should poll to examine the
updated
On 2009/07/06 23:41:14, Alpha wrote:
> nit: Should say this updates the current time the pipeline.

Done.

http://codereview.chromium.org/149215/diff/1002/21
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/149215/diff/1002/21#newcode323
Line 323: void PipelineThread::InitializationComplete(FilterHostImpl* host) {
On 2009/07/06 23:41:14, Alpha wrote:
> This method is also called from client's thread.

actually it's called from any thread I suppose

comment added

http://codereview.chromium.org/149215/diff/1002/21#newcode355
Line 355: STLDeleteElements(&filter_hosts_);
On 2009/07/06 23:41:14, Alpha wrote:
> Can you explain why you simple cleanup the container but not calling to
Stop()?
> 

I have no idea why (I only moved this code to reflect the header file), but this
code no longer exists in my next changelist (inject a message loop) so I'm
leaving it here :)

[awong, 2009-07-08 02:22:18]

couple of comments.

http://codereview.chromium.org/149215/diff/27/1010
File media/base/pipeline.h (right):

http://codereview.chromium.org/149215/diff/27/1010#newcode85
Line 85: // is acceptable to call Start() again since Stop() leaves the pipeline
Oh interesting...so unlike the filters, teh pipeline is restartable?

http://codereview.chromium.org/149215/diff/27/1010#newcode131
Line 131: // Gets the current pipeline time in microseconds. For a pipeline
"time"
You're getting a timedelta, not microseconds.  You can say getting the current
pipeline time with microsecond granularity.

http://codereview.chromium.org/149215/diff/27/1011
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/149215/diff/27/1011#newcode108
Line 108: AutoLock auto_lock(const_cast<Lock&>(lock_));
gha. suck.  Mark the lock mutable?  That's actually one of the semantically
correct places to use mutable since the lock is really meta-state about the
object so constness expectations don't really extend over it.

http://codereview.chromium.org/149215/diff/27/1011#newcode171
Line 171: DCHECK(width_out);
CHECK if you're just going to crash anyways.

BTW, darin updated the check/dcheck guidelines.
http://dev.chromium.org/developers/coding-style

http://codereview.chromium.org/149215/diff/27/1011#newcode349
Line 349: message_loop()->PostTask(FROM_HERE, task);
So...as convieneint as this is, doesn't this mess with the backtracing via the
cookie because all calls with be from the "PipelineThread::PostTask" function?

http://codereview.chromium.org/149215/diff/27/1012
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/149215/diff/27/1012#newcode37
Line 37: const std::string& uri,
I had throught tha tin chromium we always said url, not uri...

[scherkus (not reviewing), 2009-07-08 19:07:16]

http://codereview.chromium.org/149215/diff/27/1010
File media/base/pipeline.h (right):

http://codereview.chromium.org/149215/diff/27/1010#newcode85
Line 85: // is acceptable to call Start() again since Stop() leaves the pipeline
On 2009/07/08 02:22:18, awong wrote:
> Oh interesting...so unlike the filters, teh pipeline is restartable?

ahhhhh crusty old comments

I don't think it should be, and no one currently re-uses a stopped pipeline, but
for some reason it is restartable :(

I'll add a TODO

http://codereview.chromium.org/149215/diff/27/1010#newcode131
Line 131: // Gets the current pipeline time in microseconds. For a pipeline
"time"
On 2009/07/08 02:22:18, awong wrote:
> You're getting a timedelta, not microseconds.  You can say getting the current
> pipeline time with microsecond granularity.

Fixed up the comment to also remove hints at implementation

http://codereview.chromium.org/149215/diff/27/1011
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/149215/diff/27/1011#newcode108
Line 108: AutoLock auto_lock(const_cast<Lock&>(lock_));
On 2009/07/08 02:22:18, awong wrote:
> gha. suck.  Mark the lock mutable?  That's actually one of the semantically
> correct places to use mutable since the lock is really meta-state about the
> object so constness expectations don't really extend over it.

I think I asked the same thing when I reviewed this code

I'll fix'er up

http://codereview.chromium.org/149215/diff/27/1011#newcode171
Line 171: DCHECK(width_out);
On 2009/07/08 02:22:18, awong wrote:
> CHECK if you're just going to crash anyways.
> 
> BTW, darin updated the check/dcheck guidelines.
> http://dev.chromium.org/developers/coding-style
> 

Done.

http://codereview.chromium.org/149215/diff/27/1011#newcode349
Line 349: message_loop()->PostTask(FROM_HERE, task);
On 2009/07/08 02:22:18, awong wrote:
> So...as convieneint as this is, doesn't this mess with the backtracing via the
> cookie because all calls with be from the "PipelineThread::PostTask" function?

hmmm guess so

I'll remove it.. it was originally for FilterHost but that's long gone

http://codereview.chromium.org/149215/diff/27/1012
File media/base/pipeline_impl.h (right):

http://codereview.chromium.org/149215/diff/27/1012#newcode37
Line 37: const std::string& uri,
On 2009/07/08 02:22:18, awong wrote:
> I had throught tha tin chromium we always said url, not uri...

hmm copied from the header file

turns out it uses URI as well (also fixed)

[scherkus (not reviewing), 2009-07-08 23:23:16]

any additional thoughts on this one?

my git branches are starting to get pretty nested :)

[awong, 2009-07-08 23:25:20]

LGTM

Sorry..forgot this was outstanding.

[scherkus (not reviewing), 2009-07-08 23:26:25]

no worries at all!
git saves the day, many times over

On Wed, Jul 8, 2009 at 4:25 PM, <ajwong@chromium.org> wrote:

> LGTM
>
> Sorry..forgot this was outstanding.
>
>
> http://codereview.chromium.org/149215
>

[Alpha Left Google, 2009-07-08 23:33:07]

LGTM too, happy to see mysterious code go away. :)