Use GpuMemoryBufferVideoFramePool for WebMediaPlayerMS and MediaStreamVideoRendererSink

Use GpuMemoryBufferVideoFramePool for WebMediaPlayerMS and MediaStreamVideoRendererSink

MediaStreamVideoRendererSink is the common sink for video playback where we receive both the local camera capture frame and all the remote capture frames from peer connections. They are all hooked to the MediaStream elements.

This CL follows dcastagna's CL that applies asynchronous copies to WebMediaPlayerimpl [0].
[0] https://codereview.chromium.org/1273943002/

BUG=440843, 503835
TEST=
On Linux:
- With and Without the kUseGpuMemoryBuffersForCapture flag, AppRTC loopback runs as expected.
- With and Without the kEnableGpuMemoryBufferVideoFrames flag, AppRTC loopback runs as expected

Committed: https://crrev.com/d49c3945570e7eff2ce728927f8c9fecd04f3487
Cr-Commit-Position: refs/heads/master@{#353839}

[emircan, 2015-09-21 22:56:11]

Patchset #1 (id:1) has been deleted

[emircan, 2015-09-22 20:28:40]

Patchset #1 (id:20001) has been deleted

[emircan, 2015-09-22 20:40:13]

emircan@chromium.org changed reviewers:
+ dcastagna@chromium.org, mcasas@chromium.org, reveman@chromium.org

[emircan, 2015-09-22 20:40:55]

Patchset #1 (id:40001) has been deleted

[emircan, 2015-09-22 20:43:22]

PTAL. As you review the overall function signature changes RenderFrameImpl to
MediaStreamVideoRendererSink, I will change the unittests accordingly.

[emircan, 2015-09-22 22:22:49]

Patchset #1 (id:60001) has been deleted

[Daniele Castagna, 2015-09-25 00:26:33]

On 2015/09/22 at 20:43:22, emircan wrote:
> PTAL. As you review the overall function signature changes RenderFrameImpl to
MediaStreamVideoRendererSink, I will change the unittests accordingly.

In GMBVideoFramePool we assume that visible_size origin is (0,0). Is that the
case for all the VideoFrames passed to
MediaStreamVideoRendererSink::OnVideoFrame too?

[emircan, 2015-09-25 17:54:31]

On 2015/09/25 00:26:33, Daniele Castagna wrote:
> On 2015/09/22 at 20:43:22, emircan wrote:
> > PTAL. As you review the overall function signature changes RenderFrameImpl
to
> MediaStreamVideoRendererSink, I will change the unittests accordingly.
> 
> In GMBVideoFramePool we assume that visible_size origin is (0,0). Is that the
> case for all the VideoFrames passed to
> MediaStreamVideoRendererSink::OnVideoFrame too?

Both for remote frames[0] and local frames[1], we set visible_size just based on
the original dimensions, which takes (0, 0) as the origin I assume. 

[0]
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
[1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

[Daniele Castagna, 2015-09-25 19:00:58]

On 2015/09/25 at 17:54:31, emircan wrote:
> On 2015/09/25 00:26:33, Daniele Castagna wrote:
> > On 2015/09/22 at 20:43:22, emircan wrote:
> > > PTAL. As you review the overall function signature changes RenderFrameImpl
to
> > MediaStreamVideoRendererSink, I will change the unittests accordingly.
> > 
> > In GMBVideoFramePool we assume that visible_size origin is (0,0). Is that
the
> > case for all the VideoFrames passed to
> > MediaStreamVideoRendererSink::OnVideoFrame too?
> 
> Both for remote frames[0] and local frames[1], we set visible_size just based
on the original dimensions, which takes (0, 0) as the origin I assume. 
> 
> [0]
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...
> [1]
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

visible_size could be changed by
VideoTrackAdapter::VideoFrameResolutionAdapter::DeliverFrame, right?

https://codereview.chromium.org/1371683002 changes the pool so that it crops the
videoframe using visible_size.

[mcasas, 2015-10-02 19:30:46]

We should find a way to measure impact in a 
favourable scenario. f.i. CPU/memory pressure
in a getUserMedia(video)-><video> scenario

https://chromiumcodereview.appspot.com/1358883003/diff/80001/content/public/r...
File content/public/renderer/media_stream_renderer_factory.h (right):

https://chromiumcodereview.appspot.com/1358883003/diff/80001/content/public/r...
content/public/renderer/media_stream_renderer_factory.h:13: #include
"media/renderers/gpu_video_accelerator_factories.h"
Forward declare the fellas needed SingleThreadTaskRunner
and GpuVideoAcceleratorFactories plz.

https://chromiumcodereview.appspot.com/1358883003/diff/80001/content/renderer...
File content/renderer/media/media_stream_video_renderer_sink.cc (right):

https://chromiumcodereview.appspot.com/1358883003/diff/80001/content/renderer...
content/renderer/media/media_stream_video_renderer_sink.cc:33: if (gpu_factories
&&
For debugging is fine, but DCHECK(bla) + if(bla) is
discouraged [1]

[1]
https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC...

https://chromiumcodereview.appspot.com/1358883003/diff/80001/content/renderer...
content/renderer/media/media_stream_video_renderer_sink.cc:99: DCHECK(frame);
nit: Preconditions in the beginning of the method.

https://chromiumcodereview.appspot.com/1358883003/diff/80001/content/renderer...
content/renderer/media/media_stream_video_renderer_sink.cc:100: if
(gpu_memory_buffer_pool_) {
  if (!gpu_memory_buffer_pool_) {
    FrameReady(frame);
    return;
  }
  // then bla...

https://chromiumcodereview.appspot.com/1358883003/diff/80001/content/renderer...
content/renderer/media/media_stream_video_renderer_sink.cc:104: this, frame));
I think you can remove the two trampoline methods using some
PostTaskAndReply and BindToCurrentLoop bricolage :) :)

  media_task_runner_->PostTaskAndReply(
      FROM_HERE,
      base::Bind(&GpuMemoryBufferVideoFramePool::MaybeCreateHardwareFrame,
                 base::Unretained(gpu_memory_buffer_pool_), frame),
      media::BindToCurrentLoop(&MediaStreamVideoRendererSink::FrameReady, this,
                               frame));

WDYT?

https://chromiumcodereview.appspot.com/1358883003/diff/80001/content/renderer...
File content/renderer/media/media_stream_video_renderer_sink.h (right):

https://chromiumcodereview.appspot.com/1358883003/diff/80001/content/renderer...
content/renderer/media/media_stream_video_renderer_sink.h:16: #include
"media/video/gpu_memory_buffer_video_frame_pool.h"
You can forward declare SingleThreadTaskRunner (actually is done
in l.21), GpuVideoAcceleratorFactories and 
GpuMemoryBufferVideoFramePool.

https://chromiumcodereview.appspot.com/1358883003/diff/80001/content/renderer...
File content/renderer/render_frame_impl.cc (right):

https://chromiumcodereview.appspot.com/1358883003/diff/80001/content/renderer...
content/renderer/render_frame_impl.cc:4734: bool found_neon =
const :) :)

https://chromiumcodereview.appspot.com/1358883003/diff/80001/content/renderer...
content/renderer/render_frame_impl.cc:4738: RenderThreadImpl* render_thread =
RenderThreadImpl::current();
Gosh more static methods! Is this OK with unit tests...? Because there
are unit tests for it, right?
At least DCHECK(render_thread), TODO() etc :)

[emircan, 2015-10-03 00:11:26]

https://codereview.chromium.org/1358883003/diff/80001/content/public/renderer...
File content/public/renderer/media_stream_renderer_factory.h (right):

https://codereview.chromium.org/1358883003/diff/80001/content/public/renderer...
content/public/renderer/media_stream_renderer_factory.h:13: #include
"media/renderers/gpu_video_accelerator_factories.h"
On 2015/10/02 19:30:46, mcasas wrote:
> Forward declare the fellas needed SingleThreadTaskRunner
> and GpuVideoAcceleratorFactories plz.

Done.

https://codereview.chromium.org/1358883003/diff/80001/content/renderer/media/...
File content/renderer/media/media_stream_video_renderer_sink.cc (right):

https://codereview.chromium.org/1358883003/diff/80001/content/renderer/media/...
content/renderer/media/media_stream_video_renderer_sink.cc:33: if (gpu_factories
&&
On 2015/10/02 19:30:46, mcasas wrote:
> For debugging is fine, but DCHECK(bla) + if(bla) is
> discouraged [1]
> 
> [1]
>
https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC...

Ok. Removing DCHECK.

https://codereview.chromium.org/1358883003/diff/80001/content/renderer/media/...
content/renderer/media/media_stream_video_renderer_sink.cc:99: DCHECK(frame);
On 2015/10/02 19:30:46, mcasas wrote:
> nit: Preconditions in the beginning of the method.

Done.

https://codereview.chromium.org/1358883003/diff/80001/content/renderer/media/...
content/renderer/media/media_stream_video_renderer_sink.cc:100: if
(gpu_memory_buffer_pool_) {
On 2015/10/02 19:30:46, mcasas wrote:
>   if (!gpu_memory_buffer_pool_) {
>     FrameReady(frame);
>     return;
>   }
>   // then bla...

Done.

https://codereview.chromium.org/1358883003/diff/80001/content/renderer/media/...
content/renderer/media/media_stream_video_renderer_sink.cc:104: this, frame));
On 2015/10/02 19:30:46, mcasas wrote:
> I think you can remove the two trampoline methods using some
> PostTaskAndReply and BindToCurrentLoop bricolage :) :)
> 
>   media_task_runner_->PostTaskAndReply(
>       FROM_HERE,
>       base::Bind(&GpuMemoryBufferVideoFramePool::MaybeCreateHardwareFrame,
>                  base::Unretained(gpu_memory_buffer_pool_), frame),
>       media::BindToCurrentLoop(&MediaStreamVideoRendererSink::FrameReady,
this,
>                                frame));
> 
> WDYT?

Done.

https://codereview.chromium.org/1358883003/diff/80001/content/renderer/media/...
File content/renderer/media/media_stream_video_renderer_sink.h (right):

https://codereview.chromium.org/1358883003/diff/80001/content/renderer/media/...
content/renderer/media/media_stream_video_renderer_sink.h:16: #include
"media/video/gpu_memory_buffer_video_frame_pool.h"
On 2015/10/02 19:30:46, mcasas wrote:
> You can forward declare SingleThreadTaskRunner (actually is done
> in l.21), GpuVideoAcceleratorFactories and 
> GpuMemoryBufferVideoFramePool.

Done.

https://codereview.chromium.org/1358883003/diff/80001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1358883003/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:4734: bool found_neon =
On 2015/10/02 19:30:46, mcasas wrote:
> const :) :)

Done.

https://codereview.chromium.org/1358883003/diff/80001/content/renderer/render...
content/renderer/render_frame_impl.cc:4738: RenderThreadImpl* render_thread =
RenderThreadImpl::current();
On 2015/10/02 19:30:46, mcasas wrote:
> Gosh more static methods! Is this OK with unit tests...? Because there
> are unit tests for it, right?
> At least DCHECK(render_thread), TODO() etc :)

Well the earlier usage on l.4739 worked, so I am assuming it works Ok now. I
will work on additional unit tests once dcastgan@ gives approval on the overall
design and signatures.

[mcasas, 2015-10-05 17:49:45]

lgtm

https://chromiumcodereview.appspot.com/1358883003/diff/80001/content/renderer...
File content/renderer/media/media_stream_video_renderer_sink.cc (right):

https://chromiumcodereview.appspot.com/1358883003/diff/80001/content/renderer...
content/renderer/media/media_stream_video_renderer_sink.cc:104: this, frame));
On 2015/10/03 00:11:26, emircan wrote:
> On 2015/10/02 19:30:46, mcasas wrote:
> > I think you can remove the two trampoline methods using some
> > PostTaskAndReply and BindToCurrentLoop bricolage :) :)
> > 
> >   media_task_runner_->PostTaskAndReply(
> >       FROM_HERE,
> >       base::Bind(&GpuMemoryBufferVideoFramePool::MaybeCreateHardwareFrame,
> >                  base::Unretained(gpu_memory_buffer_pool_), frame),
> >       media::BindToCurrentLoop(&MediaStreamVideoRendererSink::FrameReady,
> this,
> >                                frame));
> > 
> > WDYT?
> 
> Done.

How so? I still see the two methods.

https://chromiumcodereview.appspot.com/1358883003/diff/100001/content/public/...
File content/public/renderer/media_stream_renderer_factory.h (right):

https://chromiumcodereview.appspot.com/1358883003/diff/100001/content/public/...
content/public/renderer/media_stream_renderer_factory.h:17: }
nit: add // namespace X
when closing namespace

[Daniele Castagna, 2015-10-05 22:00:32]

https://codereview.chromium.org/1358883003/diff/100001/content/renderer/media...
File content/renderer/media/media_stream_video_renderer_sink.cc (right):

https://codereview.chromium.org/1358883003/diff/100001/content/renderer/media...
content/renderer/media/media_stream_video_renderer_sink.cc:23: const
scoped_refptr<base::SingleThreadTaskRunner>& media_task_runner,
You can get this from gpu_factories if you want.

https://codereview.chromium.org/1358883003/diff/100001/content/renderer/media...
content/renderer/media/media_stream_video_renderer_sink.cc:35:
gpu_factories->ShouldUseGpuMemoryBuffersForVideoFrames()) {
This is controlled by --enable-gpu-memory-buffer-video-frames. Are you OK tying
this to that flag?

https://codereview.chromium.org/1358883003/diff/100001/content/renderer/media...
content/renderer/media/media_stream_video_renderer_sink.cc:109:
&MediaStreamVideoRendererSink::MaybeCreateHardwareFrameOnMediaThread,
Do you need this method?
Can't you do:

PostTask( ..., base::Bind(&GpuMemoryBufferVFP::MaybeCreateHardwareFrame,
gpu_memory_buffer_pool_, frame, cb) ?

[emircan, 2015-10-07 19:43:52]

On 2015/10/05 17:49:45, mcasas wrote:
> How so? I still see the two methods.

Sorry, I forgot to update with a reply. I tried PostTaskAndReply, but it is not
the intended behavior as we expect a callback within the function itself, rather
than a reply. I updated the wrappers as well.

[emircan, 2015-10-07 19:45:24]

@dcastagna, I added the new signature to
media_stream_video_renderer_sink_unittest.cc. What other unittests you think I
should look at? I realized gpu_memory_buffer_video_frame_pool_unittest is
testing most of the functionality within.

https://codereview.chromium.org/1358883003/diff/100001/content/renderer/media...
File content/renderer/media/media_stream_video_renderer_sink.cc (right):

https://codereview.chromium.org/1358883003/diff/100001/content/renderer/media...
content/renderer/media/media_stream_video_renderer_sink.cc:23: const
scoped_refptr<base::SingleThreadTaskRunner>& media_task_runner,
On 2015/10/05 22:00:32, Daniele Castagna wrote:
> You can get this from gpu_factories if you want.

Keeping it as a parameter makes it cleaner, because we don't need to check if
gpu_factories is given, mostly for unittests. WDYT?

https://codereview.chromium.org/1358883003/diff/100001/content/renderer/media...
content/renderer/media/media_stream_video_renderer_sink.cc:35:
gpu_factories->ShouldUseGpuMemoryBuffersForVideoFrames()) {
On 2015/10/05 22:00:32, Daniele Castagna wrote:
> This is controlled by --enable-gpu-memory-buffer-video-frames. Are you OK
tying
> this to that flag?

Yes, I tested with the flag.

https://codereview.chromium.org/1358883003/diff/100001/content/renderer/media...
content/renderer/media/media_stream_video_renderer_sink.cc:109:
&MediaStreamVideoRendererSink::MaybeCreateHardwareFrameOnMediaThread,
On 2015/10/05 22:00:32, Daniele Castagna wrote:
> Do you need this method?
> Can't you do:
> 
> PostTask( ..., base::Bind(&GpuMemoryBufferVFP::MaybeCreateHardwareFrame,
> gpu_memory_buffer_pool_, frame, cb) ?

Done.

[Daniele Castagna, 2015-10-07 19:57:11]

On 2015/10/07 at 19:45:24, emircan wrote:
> @dcastagna, I added the new signature to
media_stream_video_renderer_sink_unittest.cc. What other unittests you think I
should look at? I realized gpu_memory_buffer_video_frame_pool_unittest is
testing most of the functionality within.

You can take a look at the end of video_renderer_impl_unittest.cc for a place
where we test something that uses GMBVFP.

https://codereview.chromium.org/1358883003/diff/100001/content/renderer/media...
File content/renderer/media/media_stream_video_renderer_sink.cc (right):

https://codereview.chromium.org/1358883003/diff/100001/content/renderer/media...
content/renderer/media/media_stream_video_renderer_sink.cc:23: const
scoped_refptr<base::SingleThreadTaskRunner>& media_task_runner,
On 2015/10/07 at 19:45:23, emircan wrote:
> On 2015/10/05 22:00:32, Daniele Castagna wrote:
> > You can get this from gpu_factories if you want.
> 
> Keeping it as a parameter makes it cleaner, because we don't need to check if
gpu_factories is given, mostly for unittests. WDYT?

SGTM.

https://codereview.chromium.org/1358883003/diff/120001/content/public/rendere...
File content/public/renderer/media_stream_renderer_factory.h (right):

https://codereview.chromium.org/1358883003/diff/120001/content/public/rendere...
content/public/renderer/media_stream_renderer_factory.h:15: class
SingleThreadTaskRunner;
nit: no spaces at the beginning of the line.

https://codereview.chromium.org/1358883003/diff/120001/content/public/rendere...
content/public/renderer/media_stream_renderer_factory.h:17: }
nit: // namespace base

https://codereview.chromium.org/1358883003/diff/120001/content/renderer/media...
File content/renderer/media/media_stream_video_renderer_sink.h (right):

https://codereview.chromium.org/1358883003/diff/120001/content/renderer/media...
content/renderer/media/media_stream_video_renderer_sink.h:19: class
SingleThreadTaskRunner;
nit: no spaces.

https://codereview.chromium.org/1358883003/diff/120001/content/renderer/media...
content/renderer/media/media_stream_video_renderer_sink.h:72: void
MaybeCreateHardwareFrameOnMediaThread(
Are these two method decls still needed?

[mcasas, 2015-10-07 23:44:55]

I didn't mean LGTM before but I do now :)
(% dcastgna's parts)

https://codereview.chromium.org/1358883003/diff/120001/content/renderer/media...
File content/renderer/media/webmediaplayer_ms.h (right):

https://codereview.chromium.org/1358883003/diff/120001/content/renderer/media...
content/renderer/media/webmediaplayer_ms.h:17: #include
"media/renderers/gpu_video_accelerator_factories.h"
nit: I think you can forward declare media::GpuVideoAcceleratorFactories

[reveman, 2015-10-07 23:56:56]

No need to wait for my review here. dcastagna@'s review is enough.

[emircan, 2015-10-08 01:44:24]

On 2015/10/07 19:57:11, Daniele Castagna wrote:
> On 2015/10/07 at 19:45:24, emircan wrote:
> > @dcastagna, I added the new signature to
> media_stream_video_renderer_sink_unittest.cc. What other unittests you think I
> should look at? I realized gpu_memory_buffer_video_frame_pool_unittest is
> testing most of the functionality within.
Thanks. I followed that example and added a unittest. Also, refactored
MockGpuMemoryBufferVideoFramePool to a sperate class.

https://codereview.chromium.org/1358883003/diff/120001/content/public/rendere...
File content/public/renderer/media_stream_renderer_factory.h (right):

https://codereview.chromium.org/1358883003/diff/120001/content/public/rendere...
content/public/renderer/media_stream_renderer_factory.h:15: class
SingleThreadTaskRunner;
On 2015/10/07 19:57:11, Daniele Castagna wrote:
> nit: no spaces at the beginning of the line.

Done.

https://codereview.chromium.org/1358883003/diff/120001/content/public/rendere...
content/public/renderer/media_stream_renderer_factory.h:17: }
On 2015/10/07 19:57:11, Daniele Castagna wrote:
> nit: // namespace base

Done.

https://codereview.chromium.org/1358883003/diff/120001/content/renderer/media...
File content/renderer/media/media_stream_video_renderer_sink.h (right):

https://codereview.chromium.org/1358883003/diff/120001/content/renderer/media...
content/renderer/media/media_stream_video_renderer_sink.h:19: class
SingleThreadTaskRunner;
On 2015/10/07 19:57:11, Daniele Castagna wrote:
> nit: no spaces.

Done.

https://codereview.chromium.org/1358883003/diff/120001/content/renderer/media...
content/renderer/media/media_stream_video_renderer_sink.h:72: void
MaybeCreateHardwareFrameOnMediaThread(
On 2015/10/07 19:57:11, Daniele Castagna wrote:
> Are these two method decls still needed?

My bad. Removing them.

https://codereview.chromium.org/1358883003/diff/120001/content/renderer/media...
File content/renderer/media/webmediaplayer_ms.h (right):

https://codereview.chromium.org/1358883003/diff/120001/content/renderer/media...
content/renderer/media/webmediaplayer_ms.h:17: #include
"media/renderers/gpu_video_accelerator_factories.h"
On 2015/10/07 23:44:55, mcasas wrote:
> nit: I think you can forward declare media::GpuVideoAcceleratorFactories

It causes a compile error regarding ref counting.

[emircan, 2015-10-08 01:54:27]

Also, I rebased so that it no longer follows the other patch and can land by
itself.

[emircan, 2015-10-08 02:04:14]

Patchset #5 (id:130010) has been deleted

[emircan, 2015-10-08 02:24:18]

Patchset #5 (id:170001) has been deleted

[Daniele Castagna, 2015-10-08 15:38:36]

LGTM.

It seems to be failing consistently on mac though. :/

[emircan, 2015-10-08 19:56:17]

emircan@chromium.org changed reviewers:
+ dalecurtis@chromium.org, sievers@chromium.org

[emircan, 2015-10-08 19:56:18]

sievers@chromium.org: Please review changes in content/public/*,
content/shell/*, and  content/renderer/render_frame_impl.cc

dalecurtis@chromium.org: Please review changes in media/*

[no sievers, 2015-10-08 21:52:52]

On 2015/10/08 19:56:18, emircan wrote:
> mailto:sievers@chromium.org: Please review changes in content/public/*,
> content/shell/*, and  content/renderer/render_frame_impl.cc
> 
> mailto:dalecurtis@chromium.org: Please review changes in media/*

lgtm

[emircan, 2015-10-13 01:54:54]

Patchset #7 (id:230001) has been deleted

[emircan, 2015-10-13 05:01:22]

emircan@chromium.org changed reviewers:
- dalecurtis@chromium.org

[emircan, 2015-10-13 05:14:11]

emircan@chromium.org changed reviewers:
+ phoglund@chromium.org

[emircan, 2015-10-13 05:22:30]

emircan@chromium.org changed reviewers:
+ xhwang@chromium.org

[emircan, 2015-10-13 05:22:31]

xhwang@chromium.org: Please review changes in media/* . I just realized
dalecurtis is OOO.

phoglund@chromium.org: Please review changes in content/test/data/media/* for
fixing the failed unittests.

[phoglund_chromium, 2015-10-13 08:07:01]

lgtm content/test/data/media with one suggestion

https://codereview.chromium.org/1358883003/diff/250001/content/test/data/medi...
File content/test/data/media/getusermedia.html (right):

https://codereview.chromium.org/1358883003/diff/250001/content/test/data/medi...
content/test/data/media/getusermedia.html:487: COLOR_BACKGROUND_GREEN_THRESHOLD)
I think you might as well inline the threshold constant for simplicity, and
extract a helper method called isAlmostLightGreen for this rather complex
condition.

[emircan, 2015-10-13 18:12:27]

https://codereview.chromium.org/1358883003/diff/250001/content/test/data/medi...
File content/test/data/media/getusermedia.html (right):

https://codereview.chromium.org/1358883003/diff/250001/content/test/data/medi...
content/test/data/media/getusermedia.html:487: COLOR_BACKGROUND_GREEN_THRESHOLD)
On 2015/10/13 08:07:01, phoglund_chrome wrote:
> I think you might as well inline the threshold constant for simplicity, and
> extract a helper method called isAlmostLightGreen for this rather complex
> condition.

Done.

[emircan, 2015-10-13 18:15:05]

Patchset #8 (id:270001) has been deleted

[emircan, 2015-10-13 19:07:37]

emircan@chromium.org changed reviewers:
- xhwang@chromium.org

[emircan, 2015-10-13 19:08:53]

emircan@chromium.org changed reviewers:
+ xhwang@chromium.org

[xhwang, 2015-10-13 19:13:31]

media/ lgtm % tiny nits

https://codereview.chromium.org/1358883003/diff/290001/media/renderers/mock_g...
File media/renderers/mock_gpu_memory_buffer_video_frame_pool.h (right):

https://codereview.chromium.org/1358883003/diff/290001/media/renderers/mock_g...
media/renderers/mock_gpu_memory_buffer_video_frame_pool.h:18: const
FrameReadyCB& frame_ready_cb) override;
nit: add empty line

https://codereview.chromium.org/1358883003/diff/290001/media/renderers/mock_g...
media/renderers/mock_gpu_memory_buffer_video_frame_pool.h:21: };
nit: add empty line

[emircan, 2015-10-13 19:17:36]

Thanks.

https://codereview.chromium.org/1358883003/diff/290001/media/renderers/mock_g...
File media/renderers/mock_gpu_memory_buffer_video_frame_pool.h (right):

https://codereview.chromium.org/1358883003/diff/290001/media/renderers/mock_g...
media/renderers/mock_gpu_memory_buffer_video_frame_pool.h:18: const
FrameReadyCB& frame_ready_cb) override;
On 2015/10/13 19:13:31, xhwang wrote:
> nit: add empty line

Done.

https://codereview.chromium.org/1358883003/diff/290001/media/renderers/mock_g...
media/renderers/mock_gpu_memory_buffer_video_frame_pool.h:21: };
On 2015/10/13 19:13:31, xhwang wrote:
> nit: add empty line

Done.

[emircan, 2015-10-13 19:17:42]

The CQ bit was checked by emircan@chromium.org

[emircan, 2015-10-13 19:17:42]

The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org,
dcastagna@chromium.org, sievers@chromium.org, phoglund@chromium.org,
xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1358883003/#ps310001
(title: " ")

[commit-bot: I haz the power, 2015-10-13 19:19:40]

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

[commit-bot: I haz the power, 2015-10-13 20:30:16]

Committed patchset #9 (id:310001)

[commit-bot: I haz the power, 2015-10-13 20:30:51]

Patchset 9 (id:??) landed as
https://crrev.com/d49c3945570e7eff2ce728927f8c9fecd04f3487
Cr-Commit-Position: refs/heads/master@{#353839}