Fix black video frames when seeking (which also fixes flashing poster issue).

Fix black video frames when seeking (which also fixes flashing poster issue).

Keep track of the last available video frame in VideoRendererBase.

Don't drop our readystate when starting to seek as we do actually have
something to display (this fixes the poster issue).

BUG=57173, 50581
TEST=media_unittests, test_shell_tests, media layout tests

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

[scherkus (not reviewing), 2010-12-17 18:50:39]

I'm wondering if it makes more sense to do the caching in VideoRendererImpl so
that WebMediaPlayerImpl doesn't have to change and worry about caching

We can see about adding something to VideoRendererBase to make Impls have a last
chance to grab the frame before it gets discarded

Overall patch looking good I'd just also test the HW render path because it
looks like we might pass a NULL/black frame to WebKit

http://codereview.chromium.org/5878007/diff/1/webkit/glue/media/video_rendere...
File webkit/glue/media/video_renderer_impl.cc (right):

http://codereview.chromium.org/5878007/diff/1/webkit/glue/media/video_rendere...
webkit/glue/media/video_renderer_impl.cc:58: } else {
nit: de-else/indent this block due to early return

http://codereview.chromium.org/5878007/diff/1/webkit/glue/media/video_rendere...
webkit/glue/media/video_renderer_impl.cc:183: // 1. Convert YUV frame to RGB.
nit: remove the 1. (or the comment itself, your call)

http://codereview.chromium.org/5878007/diff/1/webkit/glue/media/video_rendere...
webkit/glue/media/video_renderer_impl.cc:215: // 2. Paint the bitmap to canvas.
ditto

http://codereview.chromium.org/5878007/diff/1/webkit/glue/media/video_rendere...
webkit/glue/media/video_renderer_impl.cc:231: void
VideoRendererImpl::FastPaint(media::VideoFrame* video_frame,
nit: maybe rename this to FastPaintToCanvas ?

http://codereview.chromium.org/5878007/diff/1/webkit/glue/webmediaplayer_impl.cc
File webkit/glue/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/5878007/diff/1/webkit/glue/webmediaplayer_impl...
webkit/glue/webmediaplayer_impl.cc:665: WebKit::WebVideoFrame*
WebMediaPlayerImpl::getCurrentFrame() {
watch out for these two methods.. they're using the accelerated rendering case

[sjl, 2010-12-18 03:00:02]

On 2010/12/17 18:50:39, scherkus wrote:
> I'm wondering if it makes more sense to do the caching in VideoRendererImpl so
> that WebMediaPlayerImpl doesn't have to change and worry about caching
> 
> We can see about adding something to VideoRendererBase to make Impls have a
last
> chance to grab the frame before it gets discarded

Initially, when I looked at this I thought about that approach, and after a
bunch more investigation today (and more grokking of the h/w and s/w paths) it
seems like the best approach would be a variant on your suggestion and for
VideoRendererBase to cache the last usable frame and just return that from
GetCurrentFrame in the case that current_frame_ isn't available.

This reduces the change to a pretty minor change without affecting the software
or hardware paths at all (they both pick up the benefit).

I couldn't think of a reason why you'd ever really want base to return a null
frame, so this seems like it would a reasonable fix, unless there's something
I'm missing (probably :-).

Anyhow, I've tried this out and it works well in both h/w and s/w rendering
cases (modulo a threading issue I've tracked down and am fixing).

Thoughts? If this new approach sounds good, I'll abandon this CL as the approach
is pretty different.

> 
> Overall patch looking good I'd just also test the HW render path because it
> looks like we might pass a NULL/black frame to WebKit
> 
>
http://codereview.chromium.org/5878007/diff/1/webkit/glue/media/video_rendere...
> File webkit/glue/media/video_renderer_impl.cc (right):
> 
>
http://codereview.chromium.org/5878007/diff/1/webkit/glue/media/video_rendere...
> webkit/glue/media/video_renderer_impl.cc:58: } else {
> nit: de-else/indent this block due to early return
> 
>
http://codereview.chromium.org/5878007/diff/1/webkit/glue/media/video_rendere...
> webkit/glue/media/video_renderer_impl.cc:183: // 1. Convert YUV frame to RGB.
> nit: remove the 1. (or the comment itself, your call)
> 
>
http://codereview.chromium.org/5878007/diff/1/webkit/glue/media/video_rendere...
> webkit/glue/media/video_renderer_impl.cc:215: // 2. Paint the bitmap to
canvas.
> ditto
> 
>
http://codereview.chromium.org/5878007/diff/1/webkit/glue/media/video_rendere...
> webkit/glue/media/video_renderer_impl.cc:231: void
> VideoRendererImpl::FastPaint(media::VideoFrame* video_frame,
> nit: maybe rename this to FastPaintToCanvas ?
> 
>
http://codereview.chromium.org/5878007/diff/1/webkit/glue/webmediaplayer_impl.cc
> File webkit/glue/webmediaplayer_impl.cc (right):
> 
>
http://codereview.chromium.org/5878007/diff/1/webkit/glue/webmediaplayer_impl...
> webkit/glue/webmediaplayer_impl.cc:665: WebKit::WebVideoFrame*
> WebMediaPlayerImpl::getCurrentFrame() {
> watch out for these two methods.. they're using the accelerated rendering case

[sjl, 2010-12-18 23:19:00]

On 2010/12/18 03:00:02, sjl wrote:
> On 2010/12/17 18:50:39, scherkus wrote:
> > I'm wondering if it makes more sense to do the caching in VideoRendererImpl
so
> > that WebMediaPlayerImpl doesn't have to change and worry about caching
> > 
> > We can see about adding something to VideoRendererBase to make Impls have a
> last
> > chance to grab the frame before it gets discarded
> 
> Initially, when I looked at this I thought about that approach, and after a
> bunch more investigation today (and more grokking of the h/w and s/w paths) it
> seems like the best approach would be a variant on your suggestion and for
> VideoRendererBase to cache the last usable frame and just return that from
> GetCurrentFrame in the case that current_frame_ isn't available.
> 
> This reduces the change to a pretty minor change without affecting the
software
> or hardware paths at all (they both pick up the benefit).
> 
> I couldn't think of a reason why you'd ever really want base to return a null
> frame, so this seems like it would a reasonable fix, unless there's something
> I'm missing (probably :-).
> 
> Anyhow, I've tried this out and it works well in both h/w and s/w rendering
> cases (modulo a threading issue I've tracked down and am fixing).
> 
> Thoughts? If this new approach sounds good, I'll abandon this CL as the
approach
> is pretty different.

I moved ahead with this approach - now ready for review.

> 
> > 
> > Overall patch looking good I'd just also test the HW render path because it
> > looks like we might pass a NULL/black frame to WebKit
> > 
> >
>
http://codereview.chromium.org/5878007/diff/1/webkit/glue/media/video_rendere...
> > File webkit/glue/media/video_renderer_impl.cc (right):
> > 
> >
>
http://codereview.chromium.org/5878007/diff/1/webkit/glue/media/video_rendere...
> > webkit/glue/media/video_renderer_impl.cc:58: } else {
> > nit: de-else/indent this block due to early return
> > 
> >
>
http://codereview.chromium.org/5878007/diff/1/webkit/glue/media/video_rendere...
> > webkit/glue/media/video_renderer_impl.cc:183: // 1. Convert YUV frame to
RGB.
> > nit: remove the 1. (or the comment itself, your call)
> > 
> >
>
http://codereview.chromium.org/5878007/diff/1/webkit/glue/media/video_rendere...
> > webkit/glue/media/video_renderer_impl.cc:215: // 2. Paint the bitmap to
> canvas.
> > ditto
> > 
> >
>
http://codereview.chromium.org/5878007/diff/1/webkit/glue/media/video_rendere...
> > webkit/glue/media/video_renderer_impl.cc:231: void
> > VideoRendererImpl::FastPaint(media::VideoFrame* video_frame,
> > nit: maybe rename this to FastPaintToCanvas ?
> > 
> >
>
http://codereview.chromium.org/5878007/diff/1/webkit/glue/webmediaplayer_impl.cc
> > File webkit/glue/webmediaplayer_impl.cc (right):
> > 
> >
>
http://codereview.chromium.org/5878007/diff/1/webkit/glue/webmediaplayer_impl...
> > webkit/glue/webmediaplayer_impl.cc:665: WebKit::WebVideoFrame*
> > WebMediaPlayerImpl::getCurrentFrame() {
> > watch out for these two methods.. they're using the accelerated rendering
case

[scherkus (not reviewing), 2010-12-20 22:49:54]

few questions but overall looking good

it's a tiny bit hacky but it'll work -- we need to revamp VideoRendererBase at
some point

Ideally I think we'd split the current_frame_ stuff into a separate class that
properly handles cycling buffers + ownership.  For example I think we should
basically keep current_frame_ un-touched and instead queue frames.  For example
with the Flush() method we wouldn't drop current_frame_ -- we could flush/drop
all other frames and only update current_frame_ when a new frame is actually
available

</rambling> but hclam@ and I have a bunch of ideas if you're interested to keep
picking away at this

http://codereview.chromium.org/5878007/diff/8001/media/filters/video_renderer...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/5878007/diff/8001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:101: if (pending_paint_ == false)
ditto?

http://codereview.chromium.org/5878007/diff/8001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:478: DCHECK(!pending_paint_);
add a DCHECK for pending_paint_with_last_available_ here?

[sjl, 2010-12-21 22:13:19]

On 2010/12/20 22:49:54, scherkus wrote:
> few questions but overall looking good
> 
> it's a tiny bit hacky but it'll work -- we need to revamp VideoRendererBase at
> some point

Yup - a tiny bit hacky, but a small change :-)

> 
> Ideally I think we'd split the current_frame_ stuff into a separate class that
> properly handles cycling buffers + ownership.  For example I think we should
> basically keep current_frame_ un-touched and instead queue frames.  For
example
> with the Flush() method we wouldn't drop current_frame_ -- we could flush/drop
> all other frames and only update current_frame_ when a new frame is actually
> available
> 
> </rambling> but hclam@ and I have a bunch of ideas if you're interested to
keep
> picking away at this

Yup - I am. If this patch is good (modulo my qs), I'll open another bug to track
a refactoring.
> 
>
http://codereview.chromium.org/5878007/diff/8001/media/filters/video_renderer...
> File media/filters/video_renderer_base.cc (right):
> 
>
http://codereview.chromium.org/5878007/diff/8001/media/filters/video_renderer...
> media/filters/video_renderer_base.cc:101: if (pending_paint_ == false)
> ditto?
> 
>
http://codereview.chromium.org/5878007/diff/8001/media/filters/video_renderer...
> media/filters/video_renderer_base.cc:478: DCHECK(!pending_paint_);
> add a DCHECK for pending_paint_with_last_available_ here?

[sjl, 2010-12-21 22:13:39]

http://codereview.chromium.org/5878007/diff/8001/media/filters/video_renderer...
File media/filters/video_renderer_base.cc (right):

http://codereview.chromium.org/5878007/diff/8001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:101: if (pending_paint_ == false)
On 2010/12/20 22:49:55, scherkus wrote:
> ditto?

Do you mean '&& pending_paint_with_last_available_ == false' ?

If so, I don't believe this is actually needed as FlushBuffers() only affects
current_frame_. It can still run even if the last_available_frame_ is being
painted.

http://codereview.chromium.org/5878007/diff/8001/media/filters/video_renderer...
media/filters/video_renderer_base.cc:478: DCHECK(!pending_paint_);
On 2010/12/20 22:49:55, scherkus wrote:
> add a DCHECK for pending_paint_with_last_available_ here?

As above, it's ok if we have a pending paint on last_available_frame_.

[scherkus (not reviewing), 2010-12-21 22:25:07]

cool!

LGTM!

as for a follow up bug, check out the following two:
http://code.google.com/p/chromium/issues/detail?id=28208
http://code.google.com/p/chromium/issues/detail?id=53715

[scherkus (not reviewing), 2010-12-22 17:34:47]

Committed as r69956.