The guts of the video renderer logic that relate to timing and queuing of fra...

The guts of the video renderer logic that relate to timing and queuing of frames have been moved into a base class.  This class now
simply interracts with the delegate to paint the current frame when a repaint is needed.

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

[ralphl, 2009-02-13 01:33:54]

The 3rd of three related CL's.  This one actually uses the base class in the
change list 20343 to implement the web delegate renderer.  You will notice that
the code removed pretty much just the code that is now in the base class.

However, there are some bugs that were fixed in the base class because I was
able to run a test renderer against it, so this has those bugs fixed too.

[scherkus (not reviewing), 2009-02-19 08:48:21]

nice refactor, just some quick comments

I'll take a quick look at this again after your 2nd change is cleaned up and
committed

http://codereview.chromium.org/20345/diff/1/3
File chrome/renderer/media/video_renderer_impl.h (right):

http://codereview.chromium.org/20345/diff/1/3#newcode29
Line 29: // TODO(ralphl): What the *$*%##@ is this?  What does it mean?
just as an update, I talked to WebKit people about this particular method and it
turns out its badly named.

A more accurate name would be SetSize, where they pass in the output size of the
video based on the content size (i.e., <video width=400>).  Based on this
information, we should be able to set DCHECKs for left/top coordinates to always
be 0.

http://codereview.chromium.org/20345/diff/1/3#newcode33
Line 33: virtual void Paint(skia::PlatformCanvas* canvas, const gfx::Rect&
rect);
This rect is still a mystery at the moment :)

http://codereview.chromium.org/20345/diff/1/3#newcode73
Line 73: // |current_frame_bitmap_| are valid.
I'm a bit confused here... why should assume the pointer is invalid?  if we
maintain a reference, it shouldn't be released

http://codereview.chromium.org/20345/diff/1/3#newcode74
Line 74: media::VideoFrame* last_converted_frame_;
maybe this should be a scoped_refptr?

[ralphl, 2009-02-20 01:44:32]

Updated rect to size and added comments about how the rect's relate to each
other.  Also added Carlos since he was on the original renderer review.

http://codereview.chromium.org/20345/diff/1/3
File chrome/renderer/media/video_renderer_impl.h (right):

http://codereview.chromium.org/20345/diff/1/3#newcode29
Line 29: // TODO(ralphl): What the *$*%##@ is this?  What does it mean?
On 2009/02/19 08:48:21, scherkus wrote:
> just as an update, I talked to WebKit people about this particular method and
it
> turns out its badly named.
> 
> A more accurate name would be SetSize, where they pass in the output size of
the
> video based on the content size (i.e., <video width=400>).  Based on this
> information, we should be able to set DCHECKs for left/top coordinates to
always
> be 0.

Added a comment and store the rect as as Size type.

http://codereview.chromium.org/20345/diff/1/3#newcode33
Line 33: virtual void Paint(skia::PlatformCanvas* canvas, const gfx::Rect&
rect);
On 2009/02/19 08:48:21, scherkus wrote:
> This rect is still a mystery at the moment :)

This one makes more sense.  This is the region of the display surface to bitblt
the bitmap to.  This is quite normal for a paint() method.

http://codereview.chromium.org/20345/diff/1/3#newcode73
Line 73: // |current_frame_bitmap_| are valid.
On 2009/02/19 08:48:21, scherkus wrote:
> I'm a bit confused here... why should assume the pointer is invalid?  if we
> maintain a reference, it shouldn't be released

We explicitly DO NOT maintain a reference to the frame, just the address.  We
don't want to keep the frame around by holding a reference to it.  If the frame
falls off the front of the queue, it should be discarded, and this pointer will
no longer equal the front frame.

http://codereview.chromium.org/20345/diff/1/3#newcode74
Line 74: media::VideoFrame* last_converted_frame_;
On 2009/02/19 08:48:21, scherkus wrote:
> maybe this should be a scoped_refptr?

See above description.

[ralphl, 2009-02-24 18:26:31]

This is now passing the try servers.  I think this should be reasonably easy to
approve since it's really just moving code out of this class.  I'm not blocked
on it, but I would like to get it checked in.

[scherkus (not reviewing), 2009-02-24 21:00:19]

LGTM

Submit after fixing tiny nit.

http://codereview.chromium.org/20345/diff/4006/4007
File chrome/renderer/media/video_renderer_impl.cc (right):

http://codereview.chromium.org/20345/diff/4006/4007#newcode28
Line 28: void VideoRendererImpl::Paint(skia::PlatformCanvas *canvas,
pointer goes with the type

http://codereview.chromium.org/20345/diff/4006/4007#newcode34
Line 34: video_frame = NULL;
This line isn't really needed due to scoped_refptr unless you explicitly want to
release your reference here