media: Make GpuMemoryBuffers VideoFrame copies asynchronous.

media: Make GpuMemoryBuffers VideoFrame copies asynchronous.

Plane copies from VideoFrames to GpuMemoryBuffers happening in the same thread
as the decoding thread might push the decoding time past the frame time and slow
down the entire video.

This patch adds a TaskRunner to GpuMemoryBufferVideoFramePool that is
used to do the copies asynchronously.

BUG=485859
Committed: https://crrev.com/617d086b0eb548e4841c03e85c55efd57fb5239f
Cr-Commit-Position: refs/heads/master@{#344401}

[Daniele Castagna, 2015-08-10 19:54:13]

Patchset #4 (id:60001) has been deleted

[Daniele Castagna, 2015-08-12 04:47:52]

dcastagna@chromium.org changed reviewers:
+ dalecurtis@chromium.org, reveman@chromium.org

[Daniele Castagna, 2015-08-12 04:47:53]

This is the second attempt to make videoframe to GPU memory buffer copies
asynchronous.
Since now we have a TaskRunner to post generic work, we also made the copy of a
single VideoFrame run in parallel in chunks of one megabyte.

You can see the output of 'media.tough_video_cases' here:
https://x20web.corp.google.com/~dcastagna/results.html

The first run is with GMB disabled, the second one with it enabled.
There is a lot of noise but the buffering time seems to be OK now.
The number of dropped frame improved while seek and time to play got worse as
expected.

[Daniele Castagna, 2015-08-12 16:38:40]

Patchset #10 (id:200001) has been deleted

[DaleCurtis, 2015-08-12 18:22:49]

Hmm, I'm not sure I'd call those results okay, there are more worse metrics than
there are good+equal metrics --  time to playback, seeking, and cpu utilization
seem to be uniformly worse.

The seeking you mentioned previously, the other two I was't aware of. Did you
run multiple runs of each test or just a single run? If there's an issue of
noise it'd be good to run each test a few times (telemetry has an option for
this).

[reveman, 2015-08-12 19:17:34]

On 2015/08/12 at 04:47:53, dcastagna wrote:
> This is the second attempt to make videoframe to GPU memory buffer copies
asynchronous.
> Since now we have a TaskRunner to post generic work, we also made the copy of
a single VideoFrame run in parallel in chunks of one megabyte.
> 
> You can see the output of 'media.tough_video_cases' here:
https://x20web.corp.google.com/~dcastagna/results.html

What platform is this from?

I'd expect no major improvement except moving some cost away from the critical
path, which of course is very important but not really reflected in the
benchmark results. So equal performance is success afaict. Am I understanding
this correctly?

Can you upload some results from MacOS too? I'm expecting some cpu usage
improvements in addition to moving the cost of the critical path on MacOs as we
can use IOSurfaces for zero-copy there.

[Daniele Castagna, 2015-08-15 03:35:13]

On 2015/08/12 at 19:17:34, reveman wrote:
> On 2015/08/12 at 04:47:53, dcastagna wrote:
> > This is the second attempt to make videoframe to GPU memory buffer copies
asynchronous.
> > Since now we have a TaskRunner to post generic work, we also made the copy
of a single VideoFrame run in parallel in chunks of one megabyte.
> > 
> > You can see the output of 'media.tough_video_cases' here:
https://x20web.corp.google.com/~dcastagna/results.html
> 
> What platform is this from?
> 
> I'd expect no major improvement except moving some cost away from the critical
path, which of course is very important but not really reflected in the
benchmark results. So equal performance is success afaict. Am I understanding
this correctly?
> 
> Can you upload some results from MacOS too? I'm expecting some cpu usage
improvements in addition to moving the cost of the critical path on MacOs as we
can use IOSurfaces for zero-copy there.

In the results I posted earlier, I ran each test only once. Here, there are more
results with --pageset-repeat=5:
https://x20web.corp.google.com/~dcastagna/results5.html
All the benchmarks ran on a Macbook Pro with native GpuMemoryBuffers enabled.

There is still noise, and the variance is pretty big in certain cases.
The increase in buffering_time that was the problem we noticed in
crrev.com/1133243007, and what we're mainly trying to address with this CL,
seems to be solved.

Time to playback is constantly worse, because it now includes the copies of
video frames that were not accounted before, but were happening later in the
compositor.
Seeking time got better with the last patch where we avoid copying the frame if
the timestamp is less than start_timestamp_ (please correct me if this check is
wrong).
Seeking time is still slightly worse than it used to be because, again, it takes
into account copying the videoframe (hopefully only the ones that are going to
be displayed), while before the copy happened at a later time in the compositor.

CPU time seems constantly worse for videos that use little CPU, but it seems
much better where we're using more.
I'm guessing there is a higher fixed cost per videoframe (more IPCs? Creating
and locking	IOSurfaces, etc.), but as soon as wego up with the resolution, we
can see the saving of avoiding one copy.
The worse performance regression is on tulip2.webm, from 5.52% to 9.81% (77.75%
regression).
The best performance gain is on crowd2160.webm,	from 27.38 to 15.65 (42.85%
better). smpte_3840x2160 goes from 42.04% to 28.02% on the GPU process.

However, my idea was to	land this CL without enabling the	trampoline in
video_renderer_impl.cc. I'll put up another CL for that, where we	can consider
enabling the GMB for videos only in specific circumstances (e.g: it's needed for
power savings on Mac).

David, does this answer all your questions?

[reveman, 2015-08-15 06:41:51]

On 2015/08/15 at 03:35:13, dcastagna wrote:
> On 2015/08/12 at 19:17:34, reveman wrote:
> > On 2015/08/12 at 04:47:53, dcastagna wrote:
> > > This is the second attempt to make videoframe to GPU memory buffer copies
asynchronous.
> > > Since now we have a TaskRunner to post generic work, we also made the copy
of a single VideoFrame run in parallel in chunks of one megabyte.
> > > 
> > > You can see the output of 'media.tough_video_cases' here:
https://x20web.corp.google.com/~dcastagna/results.html
> > 
> > What platform is this from?
> > 
> > I'd expect no major improvement except moving some cost away from the
critical path, which of course is very important but not really reflected in the
benchmark results. So equal performance is success afaict. Am I understanding
this correctly?
> > 
> > Can you upload some results from MacOS too? I'm expecting some cpu usage
improvements in addition to moving the cost of the critical path on MacOs as we
can use IOSurfaces for zero-copy there.
> 
> In the results I posted earlier, I ran each test only once. Here, there are
more results with --pageset-repeat=5:
https://x20web.corp.google.com/~dcastagna/results5.html
> All the benchmarks ran on a Macbook Pro with native GpuMemoryBuffers enabled.
> 
> There is still noise, and the variance is pretty big in certain cases.
> The increase in buffering_time that was the problem we noticed in
crrev.com/1133243007, and what we're mainly trying to address with this CL,
seems to be solved.
> 
> Time to playback is constantly worse, because it now includes the copies of
video frames that were not accounted before, but were happening later in the
compositor.
> Seeking time got better with the last patch where we avoid copying the frame
if the timestamp is less than start_timestamp_ (please correct me if this check
is wrong).
> Seeking time is still slightly worse than it used to be because, again, it
takes into account copying the videoframe (hopefully only the ones that are
going to be displayed), while before the copy happened at a later time in the
compositor.
> 
> CPU time seems constantly worse for videos that use little CPU, but it seems
much better where we're using more.
> I'm guessing there is a higher fixed cost per videoframe (more IPCs? Creating
and locking	IOSurfaces, etc.), but as soon as wego up with the resolution, we
can see the saving of avoiding one copy.
> The worse performance regression is on tulip2.webm, from 5.52% to 9.81%
(77.75% regression).

Is this only significantly worse when using IOSurfaces? Curious as that explains
a lot where the cost is.

> The best performance gain is on crowd2160.webm,	from 27.38 to 15.65 (42.85%
better). smpte_3840x2160 goes from 42.04% to 28.02% on the GPU process.
> 
> However, my idea was to	land this CL without enabling the	trampoline in
video_renderer_impl.cc. I'll put up another CL for that, where we	can consider
enabling the GMB for videos only in specific circumstances (e.g: it's needed for
power savings on Mac).
> 
> David, does this answer all your questions?

Yes, these results are great! Thanks for taking the time to measure all this.

Note: Moving copies from the compositor and gpu service to media/ is excellent
as takes the work off the critical path and should allow for better timing and
more responsive user interaction with pages as it's no longer preventing us from
producing frames at the right time. However, I don't think this benefit is
represented well in any of our benchmarks. If anything, it seems like it makes
some of the video benchmarks worse as they are based on the work done in media/
rather then taking the whole system into account.

[Daniele Castagna, 2015-08-17 16:19:28]

Can anyone take a look at the code?

The interface change and the passing around of the taskrunner is similar to what
you already reviewed in crrev.com/1196383002.
This time the worker_task_runner is just a TaskRunner, not a sequenced one.
Additionally PoolImpl::CopyVideoFrameToGpuMemoryBuffers splits the copy in many
small chunks and posts them to the worker_task_runner. All the copies are
synchronized using a barrier.

[reveman, 2015-08-17 16:38:30]

lgtm with nits

https://codereview.chromium.org/1273943002/diff/260001/media/base/renderer_fa...
File media/base/renderer_factory.h (right):

https://codereview.chromium.org/1273943002/diff/260001/media/base/renderer_fa...
media/base/renderer_factory.h:15: class SequencedTaskRunner;
needed?

https://codereview.chromium.org/1273943002/diff/260001/media/blink/webmediapl...
File media/blink/webmediaplayer_params.cc (right):

https://codereview.chromium.org/1273943002/diff/260001/media/blink/webmediapl...
media/blink/webmediaplayer_params.cc:7: #include "base/sequenced_task_runner.h"
needed?

https://codereview.chromium.org/1273943002/diff/260001/media/blink/webmediapl...
File media/blink/webmediaplayer_params.h (right):

https://codereview.chromium.org/1273943002/diff/260001/media/blink/webmediapl...
media/blink/webmediaplayer_params.h:14: class SequencedTaskRunner;
needed?

https://codereview.chromium.org/1273943002/diff/260001/media/video/gpu_memory...
File media/video/gpu_memory_buffer_video_frame_pool.cc (right):

https://codereview.chromium.org/1273943002/diff/260001/media/video/gpu_memory...
media/video/gpu_memory_buffer_video_frame_pool.cc:237: const size_t
kBytesPerCopyTarget = 1 << 20;
Can you add a comment that makes it clear exactly how much this is and what it's
used for. Maybe move it to anonymous namespace so it's less hidden. Also, I
think 1024 * 1024 is easier to read.

https://codereview.chromium.org/1273943002/diff/260001/media/video/gpu_memory...
media/video/gpu_memory_buffer_video_frame_pool.cc:244: int bytes =
VideoFrame::RowBytes(i, format, size.width());
nit: s/bytes/bytes_per_row/ or row_bytes

https://codereview.chromium.org/1273943002/diff/260001/media/video/gpu_memory...
media/video/gpu_memory_buffer_video_frame_pool.cc:257: int bytes =
VideoFrame::RowBytes(i, format, size.width());
nit: s/bytes/bytes_per_row/

https://codereview.chromium.org/1273943002/diff/260001/media/video/gpu_memory...
File media/video/gpu_memory_buffer_video_frame_pool.h (right):

https://codereview.chromium.org/1273943002/diff/260001/media/video/gpu_memory...
media/video/gpu_memory_buffer_video_frame_pool.h:14: class SequencedTaskRunner;
needed?

[DaleCurtis, 2015-08-17 18:01:30]

Something still seems a bit off, you're dropping more frames on average in the
1080p and 2160p cases. Do you know why that might be?

My suspicion is the post task overhead you've introduced, we went from a
blocking copy on the compositor thread to:
- post task to worker pool to start copy.
- post N tasks to the worker pool to achieve copy.
- post reply task to media thread to signal frame ready.

I'm not sure I follow why the N tasks are being posted. Isn't the worker pool
executing these tasks in sequence? Did you do this to avoid blocking the pool
while new frames are received from the decoder?

I think it might make sense to avoid these task posts for < 720p content and
just execute the copy inline.

https://codereview.chromium.org/1273943002/diff/260001/media/renderers/video_...
File media/renderers/video_renderer_impl.cc (right):

https://codereview.chromium.org/1273943002/diff/260001/media/renderers/video_...
media/renderers/video_renderer_impl.cc:482: if (status != VideoFrameStream::OK
|| start_timestamp_ > frame->timestamp()) {
Notably, the very first frame might not be in a GMB when playback starts. I.e.
if start time is 0.5 and we get frames 0, 1 you'll upload 1 but not zero, even
though the algorithm will keep 0 for display.

https://codereview.chromium.org/1273943002/diff/260001/media/video/gpu_memory...
File media/video/gpu_memory_buffer_video_frame_pool.cc (right):

https://codereview.chromium.org/1273943002/diff/260001/media/video/gpu_memory...
media/video/gpu_memory_buffer_video_frame_pool.cc:85: const FrameReadyCB& cb);
Please use something more descriptive than |cb|.

https://codereview.chromium.org/1273943002/diff/260001/media/video/gpu_memory...
media/video/gpu_memory_buffer_video_frame_pool.cc:153: base::Closure done) {
const&

https://codereview.chromium.org/1273943002/diff/260001/media/video/gpu_memory...
media/video/gpu_memory_buffer_video_frame_pool.cc:203: FrameResources*
frame_resources = GetOrCreateFrameResources(size, format);
How expensive is the create portion here? I.e. is it something that should be
considered for the worker thread?

https://codereview.chromium.org/1273943002/diff/260001/media/video/gpu_memory...
media/video/gpu_memory_buffer_video_frame_pool.cc:209:
worker_task_runner_->PostTask(
WDYT about doing an inline copy (i.e. w/o post task for resolution < 720p)? such
material represents something like 80% of playbacks.

https://codereview.chromium.org/1273943002/diff/260001/media/video/gpu_memory...
media/video/gpu_memory_buffer_video_frame_pool.cc:268:
worker_task_runner_->PostTask(
Hmm, did you change the worker task runner to run tasks in parallel? It doesn't
seem like splitting things up like this should be efficient otherwise.

[Daniele Castagna, 2015-08-19 21:31:25]

On 2015/08/17 at 18:01:30, dalecurtis wrote:
> Something still seems a bit off, you're dropping more frames on average in the
1080p and 2160p cases. Do you know why that might be?
> 

What I think it's happening is that after we post the copy asynchronously, we
then post FrameReady asynchronously on the media SequencedTaskRunner. FrameReady
won't be executed until the decode of the next frame is done.
This makes delivering a frame take longer, and it makes it more likely to drop
frames when the decode time is almost taking 1000/frame_rate milliseconds.

> My suspicion is the post task overhead you've introduced, we went from a
blocking copy on the compositor thread to:
> - post task to worker pool to start copy.
> - post N tasks to the worker pool to achieve copy.
> - post reply task to media thread to signal frame ready.
> 
> I'm not sure I follow why the N tasks are being posted. Isn't the worker pool
executing these tasks in sequence? Did you do this to avoid blocking the pool
while new frames are received from the decoder?
> 

The pool is executing tasks in parallel. I tried to remove the parallel copies
but it doesn't seem to reduce the CPU overhead.

> I think it might make sense to avoid these task posts for < 720p content and
just execute the copy inline.

I agree. In another comment I suggested to postpone this once we have the
conversion to 420 code so that we can better estimate if it makes sense or not.

https://codereview.chromium.org/1273943002/diff/260001/media/base/renderer_fa...
File media/base/renderer_factory.h (right):

https://codereview.chromium.org/1273943002/diff/260001/media/base/renderer_fa...
media/base/renderer_factory.h:15: class SequencedTaskRunner;
On 2015/08/17 at 16:38:29, reveman wrote:
> needed?

Nope.

https://codereview.chromium.org/1273943002/diff/260001/media/blink/webmediapl...
File media/blink/webmediaplayer_params.cc (right):

https://codereview.chromium.org/1273943002/diff/260001/media/blink/webmediapl...
media/blink/webmediaplayer_params.cc:7: #include "base/sequenced_task_runner.h"
On 2015/08/17 at 16:38:29, reveman wrote:
> needed?

Removed.

https://codereview.chromium.org/1273943002/diff/260001/media/blink/webmediapl...
File media/blink/webmediaplayer_params.h (right):

https://codereview.chromium.org/1273943002/diff/260001/media/blink/webmediapl...
media/blink/webmediaplayer_params.h:14: class SequencedTaskRunner;
On 2015/08/17 at 16:38:29, reveman wrote:
> needed?

No.

https://codereview.chromium.org/1273943002/diff/260001/media/renderers/video_...
File media/renderers/video_renderer_impl.cc (right):

https://codereview.chromium.org/1273943002/diff/260001/media/renderers/video_...
media/renderers/video_renderer_impl.cc:482: if (status != VideoFrameStream::OK
|| start_timestamp_ > frame->timestamp()) {
On 2015/08/17 at 18:01:30, DaleCurtis wrote:
> Notably, the very first frame might not be in a GMB when playback starts. I.e.
if start time is 0.5 and we get frames 0, 1 you'll upload 1 but not zero, even
though the algorithm will keep 0 for display.

Ack.

https://codereview.chromium.org/1273943002/diff/260001/media/video/gpu_memory...
File media/video/gpu_memory_buffer_video_frame_pool.cc (right):

https://codereview.chromium.org/1273943002/diff/260001/media/video/gpu_memory...
media/video/gpu_memory_buffer_video_frame_pool.cc:85: const FrameReadyCB& cb);
On 2015/08/17 at 18:01:30, DaleCurtis wrote:
> Please use something more descriptive than |cb|.

Done.

Change everywhere |cb| to |frame_ready_cb|

https://codereview.chromium.org/1273943002/diff/260001/media/video/gpu_memory...
media/video/gpu_memory_buffer_video_frame_pool.cc:153: base::Closure done) {
On 2015/08/17 at 18:01:30, DaleCurtis wrote:
> const&

Done.

https://codereview.chromium.org/1273943002/diff/260001/media/video/gpu_memory...
media/video/gpu_memory_buffer_video_frame_pool.cc:203: FrameResources*
frame_resources = GetOrCreateFrameResources(size, format);
On 2015/08/17 at 18:01:30, DaleCurtis wrote:
> How expensive is the create portion here? I.e. is it something that should be
considered for the worker thread?

It should not be particularly expensive but it definitely should be considered
for the worker thread.
Unfortunately it needs the gles2interface, that now can be used only the media
thread.
We should consider moving this away once we start working on crbug.com/506172

https://codereview.chromium.org/1273943002/diff/260001/media/video/gpu_memory...
media/video/gpu_memory_buffer_video_frame_pool.cc:209:
worker_task_runner_->PostTask(
On 2015/08/17 at 18:01:30, DaleCurtis wrote:
> WDYT about doing an inline copy (i.e. w/o post task for resolution < 720p)?
such material represents something like 80% of playbacks.

Sounds like a good idea.
Is it OK if we do this after we land the patch to do the YUV422 conversion so we
can get a better idea of how long this copy will take?

https://codereview.chromium.org/1273943002/diff/260001/media/video/gpu_memory...
media/video/gpu_memory_buffer_video_frame_pool.cc:237: const size_t
kBytesPerCopyTarget = 1 << 20;
On 2015/08/17 at 16:38:29, reveman wrote:
> Can you add a comment that makes it clear exactly how much this is and what
it's used for. Maybe move it to anonymous namespace so it's less hidden. Also, I
think 1024 * 1024 is easier to read.

Done.

https://codereview.chromium.org/1273943002/diff/260001/media/video/gpu_memory...
media/video/gpu_memory_buffer_video_frame_pool.cc:244: int bytes =
VideoFrame::RowBytes(i, format, size.width());
On 2015/08/17 at 16:38:29, reveman wrote:
> nit: s/bytes/bytes_per_row/ or row_bytes

Done.

https://codereview.chromium.org/1273943002/diff/260001/media/video/gpu_memory...
media/video/gpu_memory_buffer_video_frame_pool.cc:257: int bytes =
VideoFrame::RowBytes(i, format, size.width());
On 2015/08/17 at 16:38:29, reveman wrote:
> nit: s/bytes/bytes_per_row/

Done.

https://codereview.chromium.org/1273943002/diff/260001/media/video/gpu_memory...
media/video/gpu_memory_buffer_video_frame_pool.cc:268:
worker_task_runner_->PostTask(
On 2015/08/17 at 18:01:30, DaleCurtis wrote:
> Hmm, did you change the worker task runner to run tasks in parallel? It
doesn't seem like splitting things up like this should be efficient otherwise.

Correct, the worker task runner execute tasks in parallel now.

https://codereview.chromium.org/1273943002/diff/260001/media/video/gpu_memory...
File media/video/gpu_memory_buffer_video_frame_pool.h (right):

https://codereview.chromium.org/1273943002/diff/260001/media/video/gpu_memory...
media/video/gpu_memory_buffer_video_frame_pool.h:14: class SequencedTaskRunner;
On 2015/08/17 at 16:38:29, reveman wrote:
> needed?

Done.

[DaleCurtis, 2015-08-19 23:07:41]

media/ lgtm since this is disabled currently.

[Daniele Castagna, 2015-08-19 23:16:28]

dcastagna@chromium.org changed reviewers:
+ avi@chromium.org, halliwell@chromium.org, sky@google.com

[Daniele Castagna, 2015-08-19 23:16:29]

+halliwell for chromecast/
+avi for content/
+sky for components/html_viewer/

[Avi (use Gerrit), 2015-08-19 23:18:27]

On 2015/08/19 23:16:29, Daniele Castagna wrote:
> +halliwell for chromecast/
> +avi for content/
> +sky for components/html_viewer/

content lgtm

[halliwell, 2015-08-19 23:20:22]

On 2015/08/19 23:16:29, Daniele Castagna wrote:
> +halliwell for chromecast/
> +avi for content/
> +sky for components/html_viewer/

chromecast/ lgtm

[sky, 2015-08-19 23:56:44]

sky@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2015-08-19 23:56:44]

LGTM

[Daniele Castagna, 2015-08-19 23:57:12]

The CQ bit was checked by dcastagna@chromium.org

[Daniele Castagna, 2015-08-19 23:57:13]

The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/1273943002/#ps280001
(title: "Address reveman's and dalecurtis' comments. Disable GMB VideoFrames.")

[commit-bot: I haz the power, 2015-08-20 00:01:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1273943002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1273943002/280001

[commit-bot: I haz the power, 2015-08-20 01:39:39]

Committed patchset #13 (id:280001)

[commit-bot: I haz the power, 2015-08-20 01:40:27]

Patchset 13 (id:??) landed as
https://crrev.com/617d086b0eb548e4841c03e85c55efd57fb5239f
Cr-Commit-Position: refs/heads/master@{#344401}