Implemented end-of-stream callback for media::PipelineImpl.

Implemented end-of-stream callback for media::PipelineImpl.

A new method HasEnded() was added to renderer interfaces.  Renderers return true when they have both received and rendered an end-of-stream buffer.  For audio this translates to sending the very last buffer to the hardware.  For video this translates to displaying a black frame after the very last frame has been displayed.

Renderers can notify the pipeline that the value of HasEnded() has changed to true via FilterHost::NotifyEnded().  Instead of tracking which renderers have called NotifyEnded(), the pipeline uses the notification to poll every renderer.  The ended callback will only be executed once every renderer returns true for HasEnded().  This has a nice benefit of being able to ignore extra NotifyEnded() calls if we already determine the pipeline has ended.

With the changes to WebMediaPlayerImpl, we should now properly support both the ended event and looping.

BUG=16768, 17970, 18433, 18846
TEST=media_unittests, media layout tests, ended event, timeupdate should stop firing, looping should work, seeking after video ends

[scherkus (not reviewing), 2009-08-12 19:08:50]

Finally... took a few iterations and throw away changes to get here but I'm
happy with what I have now.

[Alpha Left Google, 2009-08-12 20:54:35]

the code looks good
althoguh I have a question about the use of NotifyEnded vs using a callback, do
we want to keep everything using callback or except this one?

http://codereview.chromium.org/164403/diff/48/1011
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/164403/diff/48/1011#newcode200
Line 200: remaining_time = current_frame_->GetTimestamp() - host()->GetTime();
what is this remaining time do?

http://codereview.chromium.org/164403/diff/48/1011#newcode248
Line 248: CreateBlackFrame(&current_frame_);
If the end-of-stream frame is black by itself, we don't need to do this
explicitly right?

http://codereview.chromium.org/164403/diff/48/1011#newcode314
Line 314: CreateBlackFrame(&current_frame_);
If a end-of-stream frame is a black frame by itself we don't need to generate a
black frame explicitly. What if we generate a black frame and mark it as
end-of-stream in the decoder?

http://codereview.chromium.org/164403/diff/48/1011#newcode374
Line 374: void VideoRendererBase::CreateBlackFrame(scoped_refptr<VideoFrame>*
frame_out) {
what about moving this method to VideoFrame so can be shared by VideoDecoder and
this class?

http://codereview.chromium.org/164403/diff/48/1014
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/164403/diff/48/1014#newcode267
Line 267: // at the start impacts startup latency.
this is interesting.. :-/

[scherkus (not reviewing), 2009-08-12 22:22:54]

callback vs. NotifyEnded()... it's tough to say because FilterHost itself is
like an interface of callbacks

i'm tempted to leave as is for now.  maybe there's some other way of doing it

http://codereview.chromium.org/164403/diff/48/1011
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/164403/diff/48/1011#newcode200
Line 200: remaining_time = current_frame_->GetTimestamp() - host()->GetTime();
On 2009/08/12 20:54:35, Alpha wrote:
> what is this remaining time do?

time remaining until we should advance the frame

will add a comment

http://codereview.chromium.org/164403/diff/48/1011#newcode248
Line 248: CreateBlackFrame(&current_frame_);
On 2009/08/12 20:54:35, Alpha wrote:
> If the end-of-stream frame is black by itself, we don't need to do this
> explicitly right?

right I don't think so... we can leave it as whatever displayed last

seeking past the end of the video still results in a black frame, but I think
that's ok

http://codereview.chromium.org/164403/diff/48/1011#newcode314
Line 314: CreateBlackFrame(&current_frame_);
On 2009/08/12 20:54:35, Alpha wrote:
> If a end-of-stream frame is a black frame by itself we don't need to generate
a
> black frame explicitly. What if we generate a black frame and mark it as
> end-of-stream in the decoder?

generating end-of-stream frames in the decoder is super fast right now, which is
good because the decoder might have to generate a couple of them

if it had to allocate a surface + fill it to black, that would be bad

we also made a decision that valid data cannot be combined with end of stream,
so it's actually impossible to have frame data + mark it end of stream (see
VideoFrameImpl).  it's the same with audio (audio end of stream is only true
when there is no data in the audio buffer)

http://codereview.chromium.org/164403/diff/48/1011#newcode374
Line 374: void VideoRendererBase::CreateBlackFrame(scoped_refptr<VideoFrame>*
frame_out) {
On 2009/08/12 20:54:35, Alpha wrote:
> what about moving this method to VideoFrame so can be shared by VideoDecoder
and
> this class?

Done.

http://codereview.chromium.org/164403/diff/48/1014
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/164403/diff/48/1014#newcode267
Line 267: // at the start impacts startup latency.
On 2009/08/12 20:54:35, Alpha wrote:
> this is interesting.. :-/

indeed... will try and run some javascript times

[Alpha Left Google, 2009-08-12 22:27:05]

LGTM

[awong, 2009-08-12 22:36:15]

couple of comments.

http://codereview.chromium.org/164403/diff/1045/1050
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/164403/diff/1045/1050#newcode678
Line 678: return;
If NotifyEnded is called when it isn't supposed to, it does nothing.  This
doesn't seem like it'd be obvious to the client of this API.  Is there any case
in the current situation where we are depending on delivery of the notification,
but we dont' get it?

http://codereview.chromium.org/164403/diff/1045/1053
File media/base/video_frame_impl.cc (right):

http://codereview.chromium.org/164403/diff/1045/1053#newcode56
Line 56: void VideoFrameImpl::CreateBlackFrame(scoped_refptr<VideoFrame>*
frame_out,
output parameters are supposed to be last.

http://codereview.chromium.org/164403/diff/1045/1053#newcode60
Line 60: *frame_out = NULL;
Who do we need this here?

http://codereview.chromium.org/164403/diff/1045/1053#newcode72
Line 72: DCHECK_EQ(VideoSurface::YV12, surface.format) << "Expected YV12
surface";
There isn't any code that would buffer overflow if this is violated is there?

http://codereview.chromium.org/164403/diff/1045/1053#newcode78
Line 78: += surface.strides[VideoSurface::kYPlane];
Google code line-breaks after the operator, not before.  Same below.

http://codereview.chromium.org/164403/diff/1045/1053#newcode83
Line 83: memset(surface.data[VideoSurface::kUPlane], 0x80, surface.width / 2);
Why 0x80?  constant?

[scherkus (not reviewing), 2009-08-12 23:19:45]

http://codereview.chromium.org/164403/diff/1045/1050
File media/base/pipeline_impl.cc (right):

http://codereview.chromium.org/164403/diff/1045/1050#newcode678
Line 678: return;
On 2009/08/12 22:36:15, awong wrote:
> If NotifyEnded is called when it isn't supposed to, it does nothing.  This
> doesn't seem like it'd be obvious to the client of this API.  Is there any
case
> in the current situation where we are depending on delivery of the
notification,
> but we dont' get it?

If I'm interpreting this right.. clients = renderers, right?

They don't expect anything when they call NotifyEnded() and shouldn't.  It's
them telling whoever implemented FilterHost that "hey, I'm done -- do as you
may".  In MVC speak, the renderers are models and they're notifying that one of
their visible properties (HasEnded()) has changed in value.

http://codereview.chromium.org/164403/diff/1045/1053
File media/base/video_frame_impl.cc (right):

http://codereview.chromium.org/164403/diff/1045/1053#newcode56
Line 56: void VideoFrameImpl::CreateBlackFrame(scoped_refptr<VideoFrame>*
frame_out,
On 2009/08/12 22:36:15, awong wrote:
> output parameters are supposed to be last.

Done.

http://codereview.chromium.org/164403/diff/1045/1053#newcode60
Line 60: *frame_out = NULL;
On 2009/08/12 22:36:15, awong wrote:
> Who do we need this here?
guess we dont!

the intent was that this function sets it to NULL in case of error but this
function only has one exit point

http://codereview.chromium.org/164403/diff/1045/1053#newcode72
Line 72: DCHECK_EQ(VideoSurface::YV12, surface.format) << "Expected YV12
surface";
On 2009/08/12 22:36:15, awong wrote:
> There isn't any code that would buffer overflow if this is violated is there?

well we create the frame a few lines up so maybe I'm being a little paranoid
here...

http://codereview.chromium.org/164403/diff/1045/1053#newcode78
Line 78: += surface.strides[VideoSurface::kYPlane];
On 2009/08/12 22:36:15, awong wrote:
> Google code line-breaks after the operator, not before.  Same below.

Done.

http://codereview.chromium.org/164403/diff/1045/1053#newcode83
Line 83: memset(surface.data[VideoSurface::kUPlane], 0x80, surface.width / 2);
On 2009/08/12 22:36:15, awong wrote:
> Why 0x80?  constant?

Done.

http://codereview.chromium.org/164403/diff/1045/1053#newcode83
Line 83: memset(surface.data[VideoSurface::kUPlane], 0x80, surface.width / 2);
On 2009/08/12 22:36:15, awong wrote:
> Why 0x80?  constant?

Done.

[awong, 2009-08-12 23:33:05]

LGTM