Removed PushSource::Packet. SeekableBuffer.current_time() fixed to return

Removed PushSource::Packet. SeekableBuffer.current_time() fixed to return
kInvalidTimestamp when the time is unknown.
TEST=media_unittests
BUG=28654
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=48329

[Sergey Ulanov, 2010-05-22 01:04:40]

Couple more fixes for the buffers bug.

[Sergey Ulanov, 2010-05-24 22:27:49]

ping

[Sergey Ulanov, 2010-05-26 20:01:17]

ping

[awong, 2010-05-26 20:24:18]

http://codereview.chromium.org/2140001/diff/1/2
File media/audio/simple_sources.cc (right):

http://codereview.chromium.org/2140001/diff/1/2#newcode75
media/audio/simple_sources.cc:75: buffer_.Seek(size);
Is there a reason to use memcpy + Seek instead of read?

[Sergey Ulanov, 2010-05-26 20:48:48]

http://codereview.chromium.org/2140001/diff/1/2
File media/audio/simple_sources.cc (right):

http://codereview.chromium.org/2140001/diff/1/2#newcode75
media/audio/simple_sources.cc:75: buffer_.Seek(size);
On 2010/05/26 20:24:18, awong wrote:
> Is there a reason to use memcpy + Seek instead of read?
Nope. Replaced it with Read()

[awong, 2010-05-26 20:54:41]

LGTM

Seems like a good incremental change.

I remember the need for putting in push source, but having a class that
essentially adds a facade over another makes me wonder about whether our
interface is fundamentally screwed up here.  Anyways...that's a topic for a
longer discussion.

http://codereview.chromium.org/2140001/diff/7001/8001
File media/audio/simple_sources.cc (right):

http://codereview.chromium.org/2140001/diff/7001/8001#newcode63
media/audio/simple_sources.cc:63: AutoLock auto_lock(buffer_lock_);
PushSource really just becomes a small interface adapter on a seekable buffer
doesn't it?  This makes me feel like there's room for further simplification. 
Thoughts?

[Sergey Ulanov, 2010-05-26 21:10:43]

I think we can replace PushSource with SeekableBuffer in AudioRendererHost.
Beside that PushSource is used only for unittests, so we probably can get
rid of it completely.

On Wed, May 26, 2010 at 1:54 PM, <ajwong@chromium.org> wrote:

> LGTM
>
> Seems like a good incremental change.
>
> I remember the need for putting in push source, but having a class that
> essentially adds a facade over another makes me wonder about whether our
> interface is fundamentally screwed up here.  Anyways...that's a topic for a
> longer discussion.
>
>
> http://codereview.chromium.org/2140001/diff/7001/8001
>
> File media/audio/simple_sources.cc (right):
>
> http://codereview.chromium.org/2140001/diff/7001/8001#newcode63
> media/audio/simple_sources.cc:63: AutoLock auto_lock(buffer_lock_);
> PushSource really just becomes a small interface adapter on a seekable
> buffer doesn't it?  This makes me feel like there's room for further
> simplification.  Thoughts?
>
>
> http://codereview.chromium.org/2140001/show
>

[scherkus (not reviewing), 2010-05-27 02:36:05]

Adding my $0.02 despite comment being closed.

One of my biggest coding sins was make kInvalidTimestamp part of StreamSample. 
So much extra typing and we're already inside a namespace :(

http://codereview.chromium.org/2140001/diff/1/3
File media/audio/simple_sources.h (right):

http://codereview.chromium.org/2140001/diff/1/3#newcode84
media/audio/simple_sources.h:84: media::SeekableBuffer buffer_;
nit: blank line after this line, add comment

http://codereview.chromium.org/2140001/diff/7001/8001
File media/audio/simple_sources.cc (right):

http://codereview.chromium.org/2140001/diff/7001/8001#newcode63
media/audio/simple_sources.cc:63: AutoLock auto_lock(buffer_lock_);
On 2010/05/26 20:54:41, awong wrote:
> PushSource really just becomes a small interface adapter on a seekable buffer
> doesn't it?  This makes me feel like there's room for further simplification. 
> Thoughts?

Agreed.  SeekableBuffer is now a sort of swiss army knife of buffer
implementations :)

[scherkus (not reviewing), 2010-05-27 02:36:31]

Ignore that part where I said "nit" because that's really old comment