Replace VideoRendererBase Get/PutCurrentFrame() with a VideoFrame-containing callback.

Replace VideoRendererBase Get/PutCurrentFrame() with a VideoFrame-containing callback.

BUG=108435
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180578

[scherkus (not reviewing), 2013-01-31 17:54:05]

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
File media/filters/video_renderer_base.cc (right):

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
media/filters/video_renderer_base.cc:424: // XXX do we still need this!?
PWNRF() fires the paint_cb_ ... I suppose this might be here to handle the case
where we preroll to nothing but EOS frames?

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
File media/filters/video_renderer_base.h (right):

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
media/filters/video_renderer_base.h:252: // this through Pipeline.
actually ... this might not be true

I wonder if there's an edge case where VRB might not fire paint_cb_, meaning
WMPI misses the new frame size. In that case though I believe WMPI would simply
update the size on subsequent frames.

Hmmm...

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
File media/filters/video_renderer_base_unittest.cc (left):

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
media/filters/video_renderer_base_unittest.cc:526: TEST_F(VideoRendererBaseTest,
Shutdown_DuringPaint) {
this test doesn't makes sense anymore because we pass frames outside of VRB and
don't expect them to return

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
File media/filters/video_renderer_base_unittest.cc (right):

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
media/filters/video_renderer_base_unittest.cc:492: scoped_refptr<VideoFrame>
expected = GetCurrentFrame();
I went down the path of using very fancy gmock EXPECT_CALL(*this,
OnPaint(HasTimestamp(blah blah blah)); with matchers and all this stuff ... but
it actually made the test harder to understand when things started failing.

https://codereview.chromium.org/12096081/diff/4001/webkit/media/webmediaplaye...
File webkit/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/12096081/diff/4001/webkit/media/webmediaplaye...
webkit/media/webmediaplayer_impl.cc:645: delete web_video_frame;
this whole pCF() stuff seems ripe for review / deletion at some point

[acolwell GONE FROM CHROMIUM, 2013-02-01 00:24:33]

Here is my initial round of comments. I like the direction this is going.

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
File media/filters/video_renderer_base.cc (left):

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
media/filters/video_renderer_base.cc:672: (current_frame_ && (current_frame_ ==
last_available_frame_) ? -1 : 0);
o.O  I'm not going to miss this.

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
File media/filters/video_renderer_base.cc (right):

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
media/filters/video_renderer_base.cc:85: if (state_ == kUninitialized || state_
== kStopped) {
I'm pretty sure these checks should be done under lock.

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
media/filters/video_renderer_base.cc:86: callback.Run();
message_loop_->PostTask(FROM_HERE, callback) ?

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
media/filters/video_renderer_base.cc:105: thread_ = base::kNullThreadHandle;
nit: std::swap(thread_to_join, thread_)

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
media/filters/video_renderer_base.cc:316: void
VideoRendererBase::PaintWithNextReadyFrame() {
nit: Add _Locked() & corresponding assert since this is expected to be called
under lock.

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
media/filters/video_renderer_base.cc:327: paint_cb_.Run(next_frame);
This calls the callback under lock now. Are you sure that is ok?

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
media/filters/video_renderer_base.cc:411: if (state_ == kPrerolling) {
nit: reverse check and unindent?

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
media/filters/video_renderer_base.cc:424: // XXX do we still need this!?
On 2013/01/31 17:54:05, scherkus wrote:
> PWNRF() fires the paint_cb_ ... I suppose this might be here to handle the
case
> where we preroll to nothing but EOS frames?
Not sure. What did we use to paint in the case where there wasn't a current
frame?

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
File media/filters/video_renderer_base.h (right):

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
media/filters/video_renderer_base.h:252: // this through Pipeline.
On 2013/01/31 17:54:05, scherkus wrote:
> actually ... this might not be true
> 
> I wonder if there's an edge case where VRB might not fire paint_cb_, meaning
> WMPI misses the new frame size. In that case though I believe WMPI would
simply
> update the size on subsequent frames.
> 
> Hmmm...
I think it depends on whether we want to bubble frame size change information up
to the application even if frames aren't being displayed. (aka falling behind on
decode.)

I'd be interested in seeing a CL that moves this to WMPI. The one tricky part I
can think of is I believe the natural size has to be set before we transition to
HAVE_METADATA. I'm not sure how many hoops you'll have to jump through to make
sure that happens.

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
media/filters/video_renderer_base.h:255: // The last timestamp of the frame
|paint_cb_| was called with. Set to
nit:s/The last timestamp of the frame/The timestamp on the last frame/

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
File media/filters/video_renderer_base_unittest.cc (right):

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
media/filters/video_renderer_base_unittest.cc:492: scoped_refptr<VideoFrame>
expected = GetCurrentFrame();
On 2013/01/31 17:54:05, scherkus wrote:
> I went down the path of using very fancy gmock EXPECT_CALL(*this,
> OnPaint(HasTimestamp(blah blah blah)); with matchers and all this stuff ...
but
> it actually made the test harder to understand when things started failing.
Doesn't this slightly change the test. Don't you want to show that paint doesn't
get called w/ a new frame? What about clearing current_frame_, flushing, and
then checking to make sure that current_frame_ isn't set.

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
media/filters/video_renderer_base_unittest.cc:509: scoped_refptr<VideoFrame>
expected = GetCurrentFrame();
ditto here and below. This doesn't discriminate the paint callback being called
again w/ the same frame.

https://codereview.chromium.org/12096081/diff/4001/media/tools/player_x11/pla...
File media/tools/player_x11/player_x11.cc (left):

https://codereview.chromium.org/12096081/diff/4001/media/tools/player_x11/pla...
media/tools/player_x11/player_x11.cc:94:
g_video_renderer->GetCurrentFrame(&video_frame);
I think g_video_renderer can go away now. 

Perhaps even this whole file. ;)

https://codereview.chromium.org/12096081/diff/4001/webkit/media/webmediaplaye...
File webkit/media/webmediaplayer_proxy.cc (right):

https://codereview.chromium.org/12096081/diff/4001/webkit/media/webmediaplaye...
webkit/media/webmediaplayer_proxy.cc:40: current_frame_ = frame;
I think you need more code in here to track dropped frames. It seems to me that
there should be some coordination between this function and GetCurrentFrame().
You need to be able to detect how many times current_frame_ gets updated before
another paint actually happens. This would be the number of frames that were
dropped.

[scherkus (not reviewing), 2013-02-01 22:45:25]

PTAL

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
File media/filters/video_renderer_base.cc (right):

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
media/filters/video_renderer_base.cc:85: if (state_ == kUninitialized || state_
== kStopped) {
On 2013/02/01 00:24:34, acolwell wrote:
> I'm pretty sure these checks should be done under lock.

Done.

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
media/filters/video_renderer_base.cc:86: callback.Run();
On 2013/02/01 00:24:34, acolwell wrote:
> message_loop_->PostTask(FROM_HERE, callback) ?

I'm going to hold off on cleaning up VRB threading semantics is for a different
CL! (I want to combo it w/ adding DCHECKs to SerialRunner)

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
media/filters/video_renderer_base.cc:105: thread_ = base::kNullThreadHandle;
On 2013/02/01 00:24:34, acolwell wrote:
> nit: std::swap(thread_to_join, thread_)

Done.

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
media/filters/video_renderer_base.cc:316: void
VideoRendererBase::PaintWithNextReadyFrame() {
On 2013/02/01 00:24:34, acolwell wrote:
> nit: Add _Locked() & corresponding assert since this is expected to be called
> under lock.

Done.

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
media/filters/video_renderer_base.cc:327: paint_cb_.Run(next_frame);
On 2013/02/01 00:24:34, acolwell wrote:
> This calls the callback under lock now. Are you sure that is ok?

Yes. Previously it was terrifying because the most common use case for callees
was to turn around and call GetCurrentFrame().

Callees (i.e., WMPProxy, the player_x11 thingies) don't even maintain a
reference to a VRB anymore!

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
media/filters/video_renderer_base.cc:411: if (state_ == kPrerolling) {
On 2013/02/01 00:24:34, acolwell wrote:
> nit: reverse check and unindent?

Done.

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
media/filters/video_renderer_base.cc:424: // XXX do we still need this!?
On 2013/02/01 00:24:34, acolwell wrote:
> On 2013/01/31 17:54:05, scherkus wrote:
> > PWNRF() fires the paint_cb_ ... I suppose this might be here to handle the
> case
> > where we preroll to nothing but EOS frames?
> Not sure. What did we use to paint in the case where there wasn't a current
> frame?

current_frame_ would have been null, so SkCanvasVideoRenderer would have painted
black

If at all possible I'd like for us to guarantee that a PaintCB won't get called
with a null frame. I definitely don't want VRB to generate black frames as I
feel that the decision on what the application should render if it doesn't have
a frame should be left up to the application, not VRB!

Anyway ... I dug into the history of this code and the paint_cb_.Run() below is
actually handled by PaintNextReadyFrame_Locked() so we can safely delete. The
previous code would update current_frame_ and rely on the paint_cb_.Run() below
to be called (refer to old school comment about "manually update the current
frame and notify the subclass below").

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
File media/filters/video_renderer_base.h (right):

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
media/filters/video_renderer_base.h:252: // this through Pipeline.
On 2013/02/01 00:24:34, acolwell wrote:
> On 2013/01/31 17:54:05, scherkus wrote:
> > actually ... this might not be true
> > 
> > I wonder if there's an edge case where VRB might not fire paint_cb_, meaning
> > WMPI misses the new frame size. In that case though I believe WMPI would
> simply
> > update the size on subsequent frames.
> > 
> > Hmmm...
> I think it depends on whether we want to bubble frame size change information
up
> to the application even if frames aren't being displayed. (aka falling behind
on
> decode.)
> 
> I'd be interested in seeing a CL that moves this to WMPI. The one tricky part
I
> can think of is I believe the natural size has to be set before we transition
to
> HAVE_METADATA. I'm not sure how many hoops you'll have to jump through to make
> sure that happens.

Yeah I had the same thought -- updated TODO w/ potential pitfall

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
media/filters/video_renderer_base.h:255: // The last timestamp of the frame
|paint_cb_| was called with. Set to
On 2013/02/01 00:24:34, acolwell wrote:
> nit:s/The last timestamp of the frame/The timestamp on the last frame/

Done.

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
File media/filters/video_renderer_base_unittest.cc (right):

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
media/filters/video_renderer_base_unittest.cc:492: scoped_refptr<VideoFrame>
expected = GetCurrentFrame();
On 2013/02/01 00:24:34, acolwell wrote:
> On 2013/01/31 17:54:05, scherkus wrote:
> > I went down the path of using very fancy gmock EXPECT_CALL(*this,
> > OnPaint(HasTimestamp(blah blah blah)); with matchers and all this stuff ...
> but
> > it actually made the test harder to understand when things started failing.
> Doesn't this slightly change the test. Don't you want to show that paint
doesn't
> get called w/ a new frame? What about clearing current_frame_, flushing, and
> then checking to make sure that current_frame_ isn't set.

Good catch *and* I get to keep my gmock-free code ;)

Done!

https://codereview.chromium.org/12096081/diff/4001/media/filters/video_render...
media/filters/video_renderer_base_unittest.cc:509: scoped_refptr<VideoFrame>
expected = GetCurrentFrame();
On 2013/02/01 00:24:34, acolwell wrote:
> ditto here and below. This doesn't discriminate the paint callback being
called
> again w/ the same frame.

Done.

https://codereview.chromium.org/12096081/diff/4001/media/tools/player_x11/pla...
File media/tools/player_x11/player_x11.cc (left):

https://codereview.chromium.org/12096081/diff/4001/media/tools/player_x11/pla...
media/tools/player_x11/player_x11.cc:94:
g_video_renderer->GetCurrentFrame(&video_frame);
On 2013/02/01 00:24:34, acolwell wrote:
> I think g_video_renderer can go away now. 
> 
> Perhaps even this whole file. ;)

Done.

https://codereview.chromium.org/12096081/diff/4001/webkit/media/webmediaplaye...
File webkit/media/webmediaplayer_proxy.cc (right):

https://codereview.chromium.org/12096081/diff/4001/webkit/media/webmediaplaye...
webkit/media/webmediaplayer_proxy.cc:40: current_frame_ = frame;
On 2013/02/01 00:24:34, acolwell wrote:
> I think you need more code in here to track dropped frames. It seems to me
that
> there should be some coordination between this function and GetCurrentFrame().
> You need to be able to detect how many times current_frame_ gets updated
before
> another paint actually happens. This would be the number of frames that were
> dropped.

As discussed offline, I added back the frame dropping code that was accidentally
removed. The metric is purely based on missed deadlines due to stalls in the
pipeline.

https://codereview.chromium.org/12096081/diff/12001/media/filters/video_rende...
File media/filters/video_renderer_base.cc (right):

https://codereview.chromium.org/12096081/diff/12001/media/filters/video_rende...
media/filters/video_renderer_base.cc:292: // Deadline is defined as the midpoint
between this frame and the next
The old code's deadline was defined as:
remaining_time < 0 microseconds && pending_paint_

... meaning we'd only drop frames if painting was so slow (i.e., delta between
GCF() and PCF() calls is large enough) that we'd check the deadline condition.

While this isn't a perfect at least we'll avoid running paint_cb_ with frames
that we know are running late.

[acolwell GONE FROM CHROMIUM, 2013-02-04 17:35:15]

LGTM

https://codereview.chromium.org/12096081/diff/12001/media/filters/video_rende...
File media/filters/video_renderer_base.cc (left):

https://codereview.chromium.org/12096081/diff/12001/media/filters/video_rende...
media/filters/video_renderer_base.cc:290: ready_frames_.clear();
Shouldn't we keep this line?

[scherkus (not reviewing), 2013-02-05 00:29:39]

https://codereview.chromium.org/12096081/diff/12001/media/filters/video_rende...
File media/filters/video_renderer_base.cc (left):

https://codereview.chromium.org/12096081/diff/12001/media/filters/video_rende...
media/filters/video_renderer_base.cc:290: ready_frames_.clear();
On 2013/02/04 17:35:15, acolwell wrote:
> Shouldn't we keep this line?

Done.

https://codereview.chromium.org/12096081/diff/12001/media/filters/video_rende...
File media/filters/video_renderer_base.cc (right):

https://codereview.chromium.org/12096081/diff/12001/media/filters/video_rende...
media/filters/video_renderer_base.cc:292: // Deadline is defined as the midpoint
between this frame and the next
On 2013/02/01 22:45:25, scherkus wrote:
> The old code's deadline was defined as:
> remaining_time < 0 microseconds && pending_paint_
> 
> ... meaning we'd only drop frames if painting was so slow (i.e., delta between
> GCF() and PCF() calls is large enough) that we'd check the deadline condition.
> 
> While this isn't a perfect at least we'll avoid running paint_cb_ with frames
> that we know are running late.

Expanded this comment w/ more ideas.