Potential bug on drawing video on SkCanvas

Potential bug on drawing video on SkCanvas

When drawing video frame on SkCanvas, chromium reuse the same cached SkBitmap
on both software SkCanvas and accelerated SkCanvas. After SkBitmap is used on
both SkCanvas, SkBitmap includes both GrTexture and system memory buffer. It's
weird and looks error-prone. Fortunately, it works well.

In detail, SkCanvasVideoRenderer::Paint(VideoFrame* frame, SkCanvas* canvas, ..)
draws |frame| on |canvas|. Paint() makes SkBitmap by |frame| via YUV
conversion or texture uploading and then caches SkBitmap as |last_frame_|.
Paint() can be used by both software SkCanvas and accelerated SkCanvas, and
|last_frame_| is used by both.
|last_frame_| is based on SkImageGenerator, so it's not defined to software
SkBitmap or accelerated SkBitmap at the creation time. When |last_frame_| is
drawn on software canvas, system memory cache is allocated and kept by
|last_frame_|. When |last_frame_| is drawn on accelerated canvas, GrTexture is
allocated and kept by |last_frame_|.

Although it works now, it's not-documented and unexpected behavior of SkBitmap
and SkImageGenerator. It looks like relying on skia implementation detail. This
issue makes SkCanvasVideoRenderer manage software SkBitmap and accelerated
SkBitmap explicitly.

This CL makes software SkBitmap without SkImageGenerator, which is same to the
code before crrev.com/531353002. It's because SkImageGenerator is introduced
to accelerate YUV to RGB conversion on accelerated SkCanvas, not software SkCanvas.

In addition, this CL speeds up performance when both software and accelerated SkCanvas
call Paint() interleavingly. Previously, |last_frame_| uploads YUV texture and converses
to RGB texture every time on accelerated SkCanvas, because software SkCanvas clears
the texture cache.

TEST=https://codereview.chromium.org/656683007/
BUG=424591, 91208
Committed: https://crrev.com/868f4332418328e507ed5b58c9c38ca498e926e2
Cr-Commit-Position: refs/heads/master@{#301947}

[dshwang, 2014-10-17 15:10:00]

dongseong.hwang@intel.com changed reviewers:
+ junov@chromium.org, rileya@chromium.org, scherkus@chromium.org

[dshwang, 2014-10-17 15:10:01]

scherkus@, could you review?
I extract this change from https://codereview.chromium.org/445013002/ to review
both easily.

Test CL is submitted in https://codereview.chromium.org/656683007/ separately,
because the CL is in Blink.

rileya@, could you review? This CL rollbacks the software part of your commit:
https://codereview.chromium.org/531353002

[dshwang, 2014-10-17 15:23:50]

https://codereview.chromium.org/662033002/diff/1/media/filters/skcanvas_video...
File media/filters/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/662033002/diff/1/media/filters/skcanvas_video...
media/filters/skcanvas_video_renderer.cc:38: const int
kTemporaryResourceDeletionDelay = 3;  // Seconds;
In crrev.com/445013002
> note this is in media time, which has little relation to actual wall clock
time
>
> can you provide some more details on why we need this? for example, what would
> happen if we didn't have this code?

This code exists only for memory optimization. It's brute-force optimization so
exact clock time is not needed.
Without this code, this class keeps both |last_frame_| and
|accelerated_generator_| until destruction of this class.

For example, imagine that there are one software canvas and one accelerated
canvas, and sw-canvas draws youtube video one time for thumbnail, and
accel-canvas draws the video every frame. |last_frame_| outlives until the
youtube video lives.

[dshwang, 2014-10-22 14:28:12]

Hi scherkus, rileya
Do you have a chance to review it? Thank you.

[scherkus (not reviewing), 2014-10-22 23:17:36]

mostly some nits and suggestions for clarifying the code

as mentioned in my other CL, given all the various combinations of source video
frame (SW vs HW) and destination canvas (SW vs HW), naming things correctly
becomes important

https://codereview.chromium.org/662033002/diff/1/media/filters/skcanvas_video...
File media/filters/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/662033002/diff/1/media/filters/skcanvas_video...
media/filters/skcanvas_video_renderer.cc:38: const int
kTemporaryResourceDeletionDelay = 3;  // Seconds;
On 2014/10/17 15:23:50, dshwang wrote:
> In crrev.com/445013002
> > note this is in media time, which has little relation to actual wall clock
> time
> >
> > can you provide some more details on why we need this? for example, what
would
> > happen if we didn't have this code?
> 
> This code exists only for memory optimization. It's brute-force optimization
so
> exact clock time is not needed.
> Without this code, this class keeps both |last_frame_| and
> |accelerated_generator_| until destruction of this class.
> 
> For example, imagine that there are one software canvas and one accelerated
> canvas, and sw-canvas draws youtube video one time for thumbnail, and
> accel-canvas draws the video every frame. |last_frame_| outlives until the
> youtube video lives.

Is this actually a problem in practice? I'm wary of optimizing ahead of time
without knowing whether this is something worth worrying about.

If we *do* think it's a problem worth solving, I'd still see something based on
message loops and wall clock time (e.g., using base::Timer) as it provides a
more deterministic behaviour regardless of what the incoming media timestamps
are and will also clear the cache if, say, the video is currently paused.

https://codereview.chromium.org/662033002/diff/1/media/filters/skcanvas_video...
media/filters/skcanvas_video_renderer.cc:219: // Generates an RGB image from a
VideoFrame. Convert YUV to RGB plain on GPU.
what's this new comment referring to?

it makes it sound as if it always converts on the GPU -- is that the case?

https://codereview.chromium.org/662033002/diff/1/media/filters/skcanvas_video...
media/filters/skcanvas_video_renderer.cc:329: if (!video_frame.get() ||
video_frame->natural_size().IsEmpty() ||
out of curiosity, is this change necessary?

https://codereview.chromium.org/662033002/diff/1/media/filters/skcanvas_video...
media/filters/skcanvas_video_renderer.cc:336: bool accelerated = false;
I would replace this with a SkBitmap* target_frame == NULL and change the
condition on line 360 to be an else and then insert the re-allocating+coverting
code inside a nested if

that way it's more clear that it's impossible to run both branches and that each
branch is responsible for setting |target_frame| prior to completing

https://codereview.chromium.org/662033002/diff/1/media/filters/skcanvas_video...
File media/filters/skcanvas_video_renderer.h (right):

https://codereview.chromium.org/662033002/diff/1/media/filters/skcanvas_video...
media/filters/skcanvas_video_renderer.h:47: // An RGB bitmap and corresponding
timestamp of the previously converted
I would be more explicit here to say it mention this is the result of software
color space conversion.

https://codereview.chromium.org/662033002/diff/1/media/filters/skcanvas_video...
media/filters/skcanvas_video_renderer.h:52: // It's used by accelerated SkCanvas
which is passed by a client.
ditto here to highlight that this is a hardware accelerated copy of the frame
generated by |accelerated_generator_|.

[dshwang, 2014-10-24 13:27:53]

On 2014/10/22 23:17:36, scherkus wrote:
> mostly some nits and suggestions for clarifying the code
> 
> as mentioned in my other CL, given all the various combinations of source
video
> frame (SW vs HW) and destination canvas (SW vs HW), naming things correctly
> becomes important

Thank you for detail review!

>
https://codereview.chromium.org/662033002/diff/1/media/filters/skcanvas_video...
> File media/filters/skcanvas_video_renderer.cc (right):
> 
>
https://codereview.chromium.org/662033002/diff/1/media/filters/skcanvas_video...
> media/filters/skcanvas_video_renderer.cc:38: const int
> kTemporaryResourceDeletionDelay = 3;  // Seconds;
> On 2014/10/17 15:23:50, dshwang wrote:
> > In crrev.com/445013002
> > > note this is in media time, which has little relation to actual wall clock
> > time
> > >
> > > can you provide some more details on why we need this? for example, what
> would
> > > happen if we didn't have this code?
> > 
> > This code exists only for memory optimization. It's brute-force optimization
> so
> > exact clock time is not needed.
> > Without this code, this class keeps both |last_frame_| and
> > |accelerated_generator_| until destruction of this class.
> > 
> > For example, imagine that there are one software canvas and one accelerated
> > canvas, and sw-canvas draws youtube video one time for thumbnail, and
> > accel-canvas draws the video every frame. |last_frame_| outlives until the
> > youtube video lives.
> 
> Is this actually a problem in practice? I'm wary of optimizing ahead of time
> without knowing whether this is something worth worrying about.

Yes, I think it's a problem in practice. If canvas or non-accelerated Blink
draws a video frame only once, the temporary bitmap live until the video element
is gone.

> If we *do* think it's a problem worth solving, I'd still see something based
on
> message loops and wall clock time (e.g., using base::Timer) as it provides a
> more deterministic behaviour regardless of what the incoming media timestamps
> are and will also clear the cache 

That's good idea. I use Timer in new patch set.

> if, say, the video is currently paused.

Timer can handle this case unlike previous brute-force code. Thank you.
Now the temporary bitmap is removed 3 sec after youtube video stops if
accelerated compositing is disabled.

>
https://codereview.chromium.org/662033002/diff/1/media/filters/skcanvas_video...
> media/filters/skcanvas_video_renderer.cc:219: // Generates an RGB image from a
> VideoFrame. Convert YUV to RGB plain on GPU.
> what's this new comment referring to?
> 
> it makes it sound as if it always converts on the GPU -- is that the case?

Yes, now VideoImageGenerator is used for only skia Ganesh.
If video include RGB plane, skia upload RGB bitmap to texture.
If video include YUV plane, skia upload Y, U, V plane to texture respectively
and draw them on target canvas by decoding logic.


>
https://codereview.chromium.org/662033002/diff/1/media/filters/skcanvas_video...
> media/filters/skcanvas_video_renderer.cc:329: if (!video_frame.get() ||
> video_frame->natural_size().IsEmpty() ||
> out of curiosity, is this change necessary?

In this CL, it's not necessary.
However, if Android use this method, the condition is needed, because of
following null size video frame.
scoped_refptr<VideoFrame> VideoFrame::CreateEOSFrame() {
  return new VideoFrame(VideoFrame::UNKNOWN,
                        gfx::Size(),
                        gfx::Rect(),
                        gfx::Size(),
                        scoped_ptr<gpu::MailboxHolder>(),
                        kNoTimestamp(),
                        true);
}

Moreover, If other platform video decoder also can use EOSFrame, current Paint()
can cause a bug. So I guard it for safety.
If you want, I can defer this condition to next optimization CL.

>
https://codereview.chromium.org/662033002/diff/1/media/filters/skcanvas_video...
> media/filters/skcanvas_video_renderer.cc:336: bool accelerated = false;
> I would replace this with a SkBitmap* target_frame == NULL and change the
> condition on line 360 to be an else and then insert the
re-allocating+coverting
> code inside a nested if
> 
> that way it's more clear that it's impossible to run both branches and that
each
> branch is responsible for setting |target_frame| prior to completing

That's good idea. Done.

> 
>
https://codereview.chromium.org/662033002/diff/1/media/filters/skcanvas_video...
> File media/filters/skcanvas_video_renderer.h (right):
> 
>
https://codereview.chromium.org/662033002/diff/1/media/filters/skcanvas_video...
> media/filters/skcanvas_video_renderer.h:47: // An RGB bitmap and corresponding
> timestamp of the previously converted
> I would be more explicit here to say it mention this is the result of software
> color space conversion.

Done.

> 
>
https://codereview.chromium.org/662033002/diff/1/media/filters/skcanvas_video...
> media/filters/skcanvas_video_renderer.h:52: // It's used by accelerated
SkCanvas
> which is passed by a client.
> ditto here to highlight that this is a hardware accelerated copy of the frame
> generated by |accelerated_generator_|.

Done.

[dshwang, 2014-10-24 14:17:40]

Patchset #2 (id:20001) has been deleted

[dshwang, 2014-10-28 13:11:02]

Hi scherkus, do you have a chance to review it again? Thank you!

[scherkus (not reviewing), 2014-10-28 23:24:19]

https://codereview.chromium.org/662033002/diff/40001/media/filters/skcanvas_v...
File media/filters/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/662033002/diff/40001/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer.cc:310: if
(frame_deleting_timer_.IsRunning())
Timer's destructor will take care of stopping timers -- no need for this code

https://codereview.chromium.org/662033002/diff/40001/media/filters/skcanvas_v...
File media/filters/skcanvas_video_renderer.h (right):

https://codereview.chromium.org/662033002/diff/40001/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer.h:53:
base::OneShotTimer<SkCanvasVideoRenderer> frame_deleting_timer_;
I believe DelayTimer() is a more appropriate timer (you keep calling Reset()
each time you produce a frame instead of inspecting whether it's runnign)

https://codereview.chromium.org/662033002/diff/40001/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer.h:62:
base::OneShotTimer<SkCanvasVideoRenderer> accelerated_frame_deleting_timer_;
ditto

https://codereview.chromium.org/662033002/diff/40001/media/filters/skcanvas_v...
File media/filters/skcanvas_video_renderer_unittest.cc (right):

https://codereview.chromium.org/662033002/diff/40001/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer_unittest.cc:89:
base::ThreadTaskRunnerHandle task_runner_handle_;
pretty sure you can replace all of this new code with a base::MessageLoop

as long as it's never ran, the timer will never fire

[dshwang, 2014-10-29 11:06:49]

On 2014/10/28 23:24:19, scherkus wrote:

Thank you for teaching me Timer a lot.

>
https://codereview.chromium.org/662033002/diff/40001/media/filters/skcanvas_v...
> File media/filters/skcanvas_video_renderer.cc (right):
> 
>
https://codereview.chromium.org/662033002/diff/40001/media/filters/skcanvas_v...
> media/filters/skcanvas_video_renderer.cc:310: if
> (frame_deleting_timer_.IsRunning())
> Timer's destructor will take care of stopping timers -- no need for this code

Yep, removed.

>
https://codereview.chromium.org/662033002/diff/40001/media/filters/skcanvas_v...
> File media/filters/skcanvas_video_renderer.h (right):
> 
>
https://codereview.chromium.org/662033002/diff/40001/media/filters/skcanvas_v...
> media/filters/skcanvas_video_renderer.h:53:
> base::OneShotTimer<SkCanvasVideoRenderer> frame_deleting_timer_;
> I believe DelayTimer() is a more appropriate timer (you keep calling Reset()
> each time you produce a frame instead of inspecting whether it's runnign)

Exactly. Done.

>
https://codereview.chromium.org/662033002/diff/40001/media/filters/skcanvas_v...
> media/filters/skcanvas_video_renderer.h:62:
> base::OneShotTimer<SkCanvasVideoRenderer> accelerated_frame_deleting_timer_;
> ditto

Done.

>
https://codereview.chromium.org/662033002/diff/40001/media/filters/skcanvas_v...
> File media/filters/skcanvas_video_renderer_unittest.cc (right):
> 
>
https://codereview.chromium.org/662033002/diff/40001/media/filters/skcanvas_v...
> media/filters/skcanvas_video_renderer_unittest.cc:89:
> base::ThreadTaskRunnerHandle task_runner_handle_;
> pretty sure you can replace all of this new code with a base::MessageLoop
> 
> as long as it's never ran, the timer will never fire

Yes, use MessageLoop.

[scherkus (not reviewing), 2014-10-29 17:24:10]

lgtm

https://codereview.chromium.org/662033002/diff/60001/media/filters/skcanvas_v...
File media/filters/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/662033002/diff/60001/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer.cc:453: last_frame_timestamp_ =
base::TimeDelta();
use media::kNoTimestamp()

https://codereview.chromium.org/662033002/diff/60001/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer.cc:459: accelerated_last_frame_timestamp_
= base::TimeDelta();
use media::kNoTimestamp()

https://codereview.chromium.org/662033002/diff/60001/media/filters/skcanvas_v...
File media/filters/skcanvas_video_renderer_unittest.cc (right):

https://codereview.chromium.org/662033002/diff/60001/media/filters/skcanvas_v...
media/filters/skcanvas_video_renderer_unittest.cc:87: // Needed because
SkCanvasVideoRenderer uses Timer.
I'd just remove this comment

[scherkus (not reviewing), 2014-10-29 17:24:26]

that should have been "lgtm after addressing these last few nits" :)

[dshwang, 2014-10-29 20:39:55]

Patchset #4 (id:80001) has been deleted

[dshwang, 2014-10-29 20:42:13]

On 2014/10/29 17:24:10, scherkus wrote:
> lgtm

Thank you for review! Patch becomes pretty better.

> 
>
https://codereview.chromium.org/662033002/diff/60001/media/filters/skcanvas_v...
> File media/filters/skcanvas_video_renderer.cc (right):
> 
>
https://codereview.chromium.org/662033002/diff/60001/media/filters/skcanvas_v...
> media/filters/skcanvas_video_renderer.cc:453: last_frame_timestamp_ =
> base::TimeDelta();
> use media::kNoTimestamp()

Done.

> 
>
https://codereview.chromium.org/662033002/diff/60001/media/filters/skcanvas_v...
> media/filters/skcanvas_video_renderer.cc:459:
accelerated_last_frame_timestamp_
> = base::TimeDelta();
> use media::kNoTimestamp()

Done.

> 
>
https://codereview.chromium.org/662033002/diff/60001/media/filters/skcanvas_v...
> File media/filters/skcanvas_video_renderer_unittest.cc (right):
> 
>
https://codereview.chromium.org/662033002/diff/60001/media/filters/skcanvas_v...
> media/filters/skcanvas_video_renderer_unittest.cc:87: // Needed because
> SkCanvasVideoRenderer uses Timer.
> I'd just remove this comment

Done.

[dshwang, 2014-10-29 20:42:19]

The CQ bit was checked by dongseong.hwang@intel.com

[commit-bot: I haz the power, 2014-10-29 20:43:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/662033002/100001

[commit-bot: I haz the power, 2014-10-29 22:01:22]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2014-10-29 22:01:55]

Patchset 4 (id:??) landed as
https://crrev.com/868f4332418328e507ed5b58c9c38ca498e926e2
Cr-Commit-Position: refs/heads/master@{#301947}