[Mojo Video Capture] Add test coverage for accelerated jpeg decoding

[Mojo Video Capture] Add test coverage for accelerated jpeg decoding

This CL adds test cases to VideoCaptureBrowserTest that exercise the code path 
for accelerated jpeg decoding using a fake decode accelerator.

* Add a FakeJpegDecodeAccelerator that can run in the GPU process and decodes 
  jpeg frames in software using libyuv. 
* Add a command-line flag kUseFakeJpegDecodeAccelerator to enable the fake 
  accelerator (and suppress any "real" ones if present).
* Fix threading-related DCHECKs in GpuJpegDecodeAccelerator::Client to match 
  what is actually happening. This is needed to make the new test cases pass.
  It also fixes issue 688184 [1].

This CL is part of the Mojo Video Capture work. For the bigger picture,
see [2] CL14.

BUG=584797, 688184
TEST=
  content_browsertests --gtest_filter="VideoCaptureBrowserTest.*"

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=688184
[2] https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxsxWslJ_X-zpOhijvI8/edit?usp=sharing
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2735083002
Cr-Commit-Position: refs/heads/master@{#458782}
Committed: https://chromium.googlesource.com/chromium/src/+/ac2f201c499c2d65a5d43b6ce49d9af5b4520f99

[chfremer, 2017-03-07 01:01:10]

Description was changed from

==========
Cleanup before review.


format


Merge branch '13' into 14


Merge branch '13' into 14


Merge branch '13' into 14


Merge branch '13' into 14


Merge branch '13' into 14


Fix after gclient update


Merge branch '13' into 14


Add test case that exercises accelerated jpeg decoding code paths.

BUG=
==========

to

==========
Cleanup before review.


format


Merge branch '13' into 14


Merge branch '13' into 14


Merge branch '13' into 14


Merge branch '13' into 14


Merge branch '13' into 14


Fix after gclient update


Merge branch '13' into 14


Add test case that exercises accelerated jpeg decoding code paths.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[chfremer, 2017-03-07 01:01:35]

Description was changed from

==========
Cleanup before review.


format


Merge branch '13' into 14


Merge branch '13' into 14


Merge branch '13' into 14


Merge branch '13' into 14


Merge branch '13' into 14


Fix after gclient update


Merge branch '13' into 14


Add test case that exercises accelerated jpeg decoding code paths.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
[Mojo Video Capture] Add test coverage for accelerated jpeg decoding

This CL adds test cases to VideoCaptureBrowserTest that exercise the code path 
for accelerated jpeg decoding using a fake decode accelerator.

* Add a FakeJpegDecodeAccelerator that can run in the GPU process and decodes 
  jpeg frames in software using libyuv. 
* Add a command-line flag kUseFakeJpegDecodeAccelerator to enable the fake 
  accelerator (and suppress any "real" ones if present).
* Fix threading-related DCHECKs in GpuJpegDecodeAccelerator::Client to match 
  what is actually happening. This is needed to make the new test cases pass.
  It also fixes issue 688184 [1].

This CL is part of the Mojo Video Capture work. For the bigger picture,
see [2] CL14.

BUG=584797
TEST=
  content_browsertests --gtest_filter="VideoCaptureBrowserTest.*"

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=688184
[2]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

[chfremer, 2017-03-07 01:01:58]

The CQ bit was checked by chfremer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 01:03:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 01:08:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 01:08:58]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chfremer, 2017-03-07 01:14:07]

Description was changed from

==========
[Mojo Video Capture] Add test coverage for accelerated jpeg decoding

This CL adds test cases to VideoCaptureBrowserTest that exercise the code path 
for accelerated jpeg decoding using a fake decode accelerator.

* Add a FakeJpegDecodeAccelerator that can run in the GPU process and decodes 
  jpeg frames in software using libyuv. 
* Add a command-line flag kUseFakeJpegDecodeAccelerator to enable the fake 
  accelerator (and suppress any "real" ones if present).
* Fix threading-related DCHECKs in GpuJpegDecodeAccelerator::Client to match 
  what is actually happening. This is needed to make the new test cases pass.
  It also fixes issue 688184 [1].

This CL is part of the Mojo Video Capture work. For the bigger picture,
see [2] CL14.

BUG=584797
TEST=
  content_browsertests --gtest_filter="VideoCaptureBrowserTest.*"

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=688184
[2]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
==========

to

==========
[Mojo Video Capture] Add test coverage for accelerated jpeg decoding

This CL adds test cases to VideoCaptureBrowserTest that exercise the code path 
for accelerated jpeg decoding using a fake decode accelerator.

* Add a FakeJpegDecodeAccelerator that can run in the GPU process and decodes 
  jpeg frames in software using libyuv. 
* Add a command-line flag kUseFakeJpegDecodeAccelerator to enable the fake 
  accelerator (and suppress any "real" ones if present).
* Fix threading-related DCHECKs in GpuJpegDecodeAccelerator::Client to match 
  what is actually happening. This is needed to make the new test cases pass.
  It also fixes issue 688184 [1].

This CL is part of the Mojo Video Capture work. For the bigger picture,
see [2] CL14.

BUG=584797
TEST=
  content_browsertests --gtest_filter="VideoCaptureBrowserTest.*"

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=688184
[2]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[chfremer, 2017-03-07 01:14:20]

The CQ bit was checked by chfremer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 01:15:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 03:15:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 03:15:30]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[chfremer1, 2017-03-07 16:35:49]

Patchset #1 (id:1) has been deleted

[chfremer, 2017-03-07 16:37:48]

mcasas@: PTAL
miu@: PTAL
emircan@: FYI

[mcasas, 2017-03-07 19:39:37]

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

[mcasas, 2017-03-07 19:39:38]

https://codereview.chromium.org/2735083002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_browsertest.cc (right):

https://codereview.chromium.org/2735083002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_browsertest.cc:221: }));
Ugh waiting like this.  Can't we somehow inject the FakeJpeg decoder
via some Gpu process backdoor?

https://codereview.chromium.org/2735083002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller.h (right):

https://codereview.chromium.org/2735083002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.h:106: void
OnStoppedUsingGpuDecode() override;
I don't see the need for OnStoppedUsingGpuDecode():
it's not ending up anywhere in this CL and, once
OnStartedUsingGpuDecode() has been received,
it's fair assumption to think it's running until
OnError or capture stops.

https://codereview.chromium.org/2735083002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.cc
(right):

https://codereview.chromium.org/2735083002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.cc:62:
switches::kDisableAcceleratedMjpegDecode)) {
It seems like |kUseFakeJpegDecodeAccelerator| and
|kDisableAcceleratedMjpegDecode| are related; what
happens if we force-use-face and force-disable? Is 
that a situation we should care about? Should we merge
both?

https://codereview.chromium.org/2735083002/diff/20001/media/capture/video/vid...
File media/capture/video/video_capture_device_client.h (right):

https://codereview.chromium.org/2735083002/diff/20001/media/capture/video/vid...
media/capture/video/video_capture_device_client.h:114: bool
using_external_jpeg_decoder_;
Isn't this variable having the same lifetime changes
as |external_jpeg_decoder_| ?  If so, you don't need
the extra methods.

https://codereview.chromium.org/2735083002/diff/20001/media/gpu/fake_jpeg_dec...
File media/gpu/fake_jpeg_decode_accelerator.cc (right):

https://codereview.chromium.org/2735083002/diff/20001/media/gpu/fake_jpeg_dec...
media/gpu/fake_jpeg_decode_accelerator.cc:51: gfx::Size
src_coded_size(parse_result.frame_header.coded_width,
nit: const.

https://codereview.chromium.org/2735083002/diff/20001/media/gpu/fake_jpeg_dec...
media/gpu/fake_jpeg_decode_accelerator.cc:69: LOG(ERROR) << "Software decode
failed.";
s/LOG/DLOG/ for consistency.  Note that
release bots compile with debug logs on.

https://codereview.chromium.org/2735083002/diff/20001/media/gpu/fake_jpeg_dec...
media/gpu/fake_jpeg_decode_accelerator.cc:85: int32_t bitstream_buffer_id,
s/NotifyErrorFromIoThread/NotifyError/ 

same as Decode(), we don't need a from-thread indication
beyond the DCHECK in l.87.

https://codereview.chromium.org/2735083002/diff/20001/media/gpu/fake_jpeg_dec...
media/gpu/fake_jpeg_decode_accelerator.cc:87: task_runner_->PostTask(
DCHECK(io_task_runner_->BelongsToCurrentThread()); ?

https://codereview.chromium.org/2735083002/diff/20001/media/gpu/fake_jpeg_dec...
media/gpu/fake_jpeg_decode_accelerator.cc:93: void
FakeJpegDecodeAccelerator::OnNotifyErrorOnClientThread(
s/OnNotifyErrorOnClientThread/NotifyErrorOnClientThread/

https://codereview.chromium.org/2735083002/diff/20001/media/gpu/fake_jpeg_dec...
media/gpu/fake_jpeg_decode_accelerator.cc:96:
client_->NotifyError(bitstream_buffer_id, error);
Add DCHECK(task_runner_->BelongsToCurrentThread());

https://codereview.chromium.org/2735083002/diff/20001/media/gpu/fake_jpeg_dec...
media/gpu/fake_jpeg_decode_accelerator.cc:101:
client_->VideoFrameReady(input_buffer_id);
Add DCHECK(task_runner_->BelongsToCurrentThread());

https://codereview.chromium.org/2735083002/diff/20001/media/gpu/fake_jpeg_dec...
File media/gpu/fake_jpeg_decode_accelerator.h (right):

https://codereview.chromium.org/2735083002/diff/20001/media/gpu/fake_jpeg_dec...
media/gpu/fake_jpeg_decode_accelerator.h:42: const
scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
nit: Forward declare SingleThreadTaskRunner

https://codereview.chromium.org/2735083002/diff/20001/media/gpu/ipc/service/g...
File media/gpu/ipc/service/gpu_jpeg_decode_accelerator.cc (right):

https://codereview.chromium.org/2735083002/diff/20001/media/gpu/ipc/service/g...
media/gpu/ipc/service/gpu_jpeg_decode_accelerator.cc:156: class
GpuJpegDecodeAccelerator::MessageFilter : public IPC::MessageFilter {
Totally not your problem but this needs to be migrated to
Mojo.

Could you please create a bug and add here a TODO() ?
:-)

[chfremer, 2017-03-07 19:39:50]

chfremer@chromium.org changed reviewers:
+ emircan@chromium.org, miu@chromium.org, posciak@chromium.org

[chfremer, 2017-03-07 19:39:50]

posciak@: PTAL media/gpu/*
emircan@: PTAL content/browser/renderer_host/media/*, media/capture/vide*
mcasas@, miu@: FYI

[chfremer, 2017-03-07 19:44:57]

On 2017/03/07 19:39:50, chfremer wrote:
> posciak@: PTAL media/gpu/*
> emircan@: PTAL content/browser/renderer_host/media/*, media/capture/vide*
> mcasas@, miu@: FYI

Sorry, I messed up the reviewer assignment the first time around and forgot to
actually include any reviewers e-mails.

mcasas@: Thanks that you already reviewed, I will address ASAP.
emircan@, miu@: Reduced to FYI

[miu, 2017-03-07 20:31:45]

Description was changed from

==========
[Mojo Video Capture] Add test coverage for accelerated jpeg decoding

This CL adds test cases to VideoCaptureBrowserTest that exercise the code path 
for accelerated jpeg decoding using a fake decode accelerator.

* Add a FakeJpegDecodeAccelerator that can run in the GPU process and decodes 
  jpeg frames in software using libyuv. 
* Add a command-line flag kUseFakeJpegDecodeAccelerator to enable the fake 
  accelerator (and suppress any "real" ones if present).
* Fix threading-related DCHECKs in GpuJpegDecodeAccelerator::Client to match 
  what is actually happening. This is needed to make the new test cases pass.
  It also fixes issue 688184 [1].

This CL is part of the Mojo Video Capture work. For the bigger picture,
see [2] CL14.

BUG=584797
TEST=
  content_browsertests --gtest_filter="VideoCaptureBrowserTest.*"

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=688184
[2]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
[Mojo Video Capture] Add test coverage for accelerated jpeg decoding

This CL adds test cases to VideoCaptureBrowserTest that exercise the code path 
for accelerated jpeg decoding using a fake decode accelerator.

* Add a FakeJpegDecodeAccelerator that can run in the GPU process and decodes 
  jpeg frames in software using libyuv. 
* Add a command-line flag kUseFakeJpegDecodeAccelerator to enable the fake 
  accelerator (and suppress any "real" ones if present).
* Fix threading-related DCHECKs in GpuJpegDecodeAccelerator::Client to match 
  what is actually happening. This is needed to make the new test cases pass.
  It also fixes issue 688184 [1].

This CL is part of the Mojo Video Capture work. For the bigger picture,
see [2] CL14.

BUG=584797,688184
TEST=
  content_browsertests --gtest_filter="VideoCaptureBrowserTest.*"

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=688184
[2]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[miu, 2017-03-07 20:32:15]

FYI: I added crbug 688184 to the BUG= line in the change description.

[miu, 2017-03-07 20:35:48]

Moving myself to cc, since I don't really know how the accelerated jpeg decode
code works.

[miu, 2017-03-07 20:36:02]

miu@chromium.org changed reviewers:
- miu@chromium.org

[chfremer, 2017-03-07 22:30:39]

mcasas@: PTAL

https://codereview.chromium.org/2735083002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_browsertest.cc (right):

https://codereview.chromium.org/2735083002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_browsertest.cc:221: }));
On 2017/03/07 19:39:37, mcasas wrote:
> Ugh waiting like this.  Can't we somehow inject the FakeJpeg decoder
> via some Gpu process backdoor?

I don't see how that would help.
The current behavior is that VideoCaptureDeviceClient only spins up the Gpu
decoder when encountering an Mjpeg frame, and it performs software decode while
waiting for it to initialize. Unless we want to change that behavior, I don't
see a way around waiting.

https://codereview.chromium.org/2735083002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller.h (right):

https://codereview.chromium.org/2735083002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.h:106: void
OnStoppedUsingGpuDecode() override;
On 2017/03/07 19:39:37, mcasas wrote:
> I don't see the need for OnStoppedUsingGpuDecode():
> it's not ending up anywhere in this CL and, once
> OnStartedUsingGpuDecode() has been received,
> it's fair assumption to think it's running until
> OnError or capture stops.

True that it is not strictly needed. I added it for symmetry reasons and for
indicating to the reader that it actually _can_ happen that Gpu decode stops
after it has started. Looking at [1], when the Gpu decoder errors out, the
VCDClient just switches back to software decoding. It does not call OnError() in
that case.

Let me know if you still want me to remove it, and I'll make the change.

[1]
https://cs.chromium.org/chromium/src/media/capture/video/video_capture_device...

https://codereview.chromium.org/2735083002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.cc
(right):

https://codereview.chromium.org/2735083002/diff/20001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_gpu_jpeg_decoder.cc:62:
switches::kDisableAcceleratedMjpegDecode)) {
On 2017/03/07 19:39:37, mcasas wrote:
> It seems like |kUseFakeJpegDecodeAccelerator| and
> |kDisableAcceleratedMjpegDecode| are related; what
> happens if we force-use-face and force-disable? Is 
> that a situation we should care about? Should we merge
> both?

Good point. Switches depending on each other is a general problem we are facing,
not just here. Typically it is not specified what happens in such a situation. I
have put some thought into what we could do about it and written up a small
design doc [1].

Luckily, for this specific case, it is still relatively easy to see what would
(probably) happen if we set both flags based on lines 51-65. The
|kDisableAcceleratedMjpegDecode| overrides the |kUseFakeJpegDecodeAccelerator|.
I don't think we should merge them, because they are useful individually.

[1]
https://docs.google.com/document/d/1kydw6u7UwPG0WNdhRqVzEWr3rvEvTtYshTvD5CY0Z...

https://codereview.chromium.org/2735083002/diff/20001/media/capture/video/vid...
File media/capture/video/video_capture_device_client.h (right):

https://codereview.chromium.org/2735083002/diff/20001/media/capture/video/vid...
media/capture/video/video_capture_device_client.h:114: bool
using_external_jpeg_decoder_;
On 2017/03/07 19:39:38, mcasas wrote:
> Isn't this variable having the same lifetime changes
> as |external_jpeg_decoder_| ?  If so, you don't need
> the extra methods.

The |external_jpeg_decoder_| is created and initially exists with a status
INIT_PENDING. At some point after that it changes its state to INIT_PASSED. Only
then do we want to send OnStartedUsingGpuDecode() and we only want to send it
once. I don't see how to do it without the flag.

https://codereview.chromium.org/2735083002/diff/20001/media/gpu/fake_jpeg_dec...
File media/gpu/fake_jpeg_decode_accelerator.cc (right):

https://codereview.chromium.org/2735083002/diff/20001/media/gpu/fake_jpeg_dec...
media/gpu/fake_jpeg_decode_accelerator.cc:51: gfx::Size
src_coded_size(parse_result.frame_header.coded_width,
On 2017/03/07 19:39:38, mcasas wrote:
> nit: const.

Done.

https://codereview.chromium.org/2735083002/diff/20001/media/gpu/fake_jpeg_dec...
media/gpu/fake_jpeg_decode_accelerator.cc:69: LOG(ERROR) << "Software decode
failed.";
On 2017/03/07 19:39:38, mcasas wrote:
> s/LOG/DLOG/ for consistency.  Note that
> release bots compile with debug logs on.

Done.

https://codereview.chromium.org/2735083002/diff/20001/media/gpu/fake_jpeg_dec...
media/gpu/fake_jpeg_decode_accelerator.cc:85: int32_t bitstream_buffer_id,
On 2017/03/07 19:39:38, mcasas wrote:
> s/NotifyErrorFromIoThread/NotifyError/ 
> 
> same as Decode(), we don't need a from-thread indication
> beyond the DCHECK in l.87.

Done.

https://codereview.chromium.org/2735083002/diff/20001/media/gpu/fake_jpeg_dec...
media/gpu/fake_jpeg_decode_accelerator.cc:87: task_runner_->PostTask(
On 2017/03/07 19:39:38, mcasas wrote:
> DCHECK(io_task_runner_->BelongsToCurrentThread()); ?

Done.

https://codereview.chromium.org/2735083002/diff/20001/media/gpu/fake_jpeg_dec...
media/gpu/fake_jpeg_decode_accelerator.cc:93: void
FakeJpegDecodeAccelerator::OnNotifyErrorOnClientThread(
On 2017/03/07 19:39:38, mcasas wrote:
> s/OnNotifyErrorOnClientThread/NotifyErrorOnClientThread/

Done.

https://codereview.chromium.org/2735083002/diff/20001/media/gpu/fake_jpeg_dec...
media/gpu/fake_jpeg_decode_accelerator.cc:96:
client_->NotifyError(bitstream_buffer_id, error);
On 2017/03/07 19:39:38, mcasas wrote:
> Add DCHECK(task_runner_->BelongsToCurrentThread());

Done.

https://codereview.chromium.org/2735083002/diff/20001/media/gpu/fake_jpeg_dec...
media/gpu/fake_jpeg_decode_accelerator.cc:101:
client_->VideoFrameReady(input_buffer_id);
On 2017/03/07 19:39:38, mcasas wrote:
> Add DCHECK(task_runner_->BelongsToCurrentThread());

Done.

https://codereview.chromium.org/2735083002/diff/20001/media/gpu/fake_jpeg_dec...
File media/gpu/fake_jpeg_decode_accelerator.h (right):

https://codereview.chromium.org/2735083002/diff/20001/media/gpu/fake_jpeg_dec...
media/gpu/fake_jpeg_decode_accelerator.h:42: const
scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
On 2017/03/07 19:39:38, mcasas wrote:
> nit: Forward declare SingleThreadTaskRunner

Done.

https://codereview.chromium.org/2735083002/diff/20001/media/gpu/ipc/service/g...
File media/gpu/ipc/service/gpu_jpeg_decode_accelerator.cc (right):

https://codereview.chromium.org/2735083002/diff/20001/media/gpu/ipc/service/g...
media/gpu/ipc/service/gpu_jpeg_decode_accelerator.cc:156: class
GpuJpegDecodeAccelerator::MessageFilter : public IPC::MessageFilter {
On 2017/03/07 19:39:38, mcasas wrote:
> Totally not your problem but this needs to be migrated to
> Mojo.
> 
> Could you please create a bug and add here a TODO() ?
> :-)

Done.

[chfremer, 2017-03-07 22:30:44]

The CQ bit was checked by chfremer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 22:31:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chfremer, 2017-03-07 23:19:23]

The CQ bit was checked by chfremer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 23:20:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mcasas, 2017-03-08 01:33:17]

Looking good, some comments/suggestions.

https://codereview.chromium.org/2735083002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/2735083002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:481: }
This code looks a lot like l.452-455 and l.467-470 (and
perhaps others), but I don't see an easy way to generalize
it since functions cannot have function templates.

template <typename F>
void NotifyEventHandlerIfSessionOpen(F handler) {
  DCHECK_CURRENTLY_ON(BrowserThread::IO);
  for (const auto& client : controller_clients_) {
    if (client->session_closed)
      continue;
    handler(client->controller_id);
  }
}

called e.g. like
NotifyEventHandlerIfSessionOpen(client->event_handler->OnStartedUsingGpuDecode);
probably won't compile as-is :S

Not sure if this is better or worse, after writing it :-)

https://codereview.chromium.org/2735083002/diff/60001/content/browser/rendere...
File
content/browser/renderer_host/media/video_capture_controller_event_handler.h
(right):

https://codereview.chromium.org/2735083002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller_event_handler.h:60:
virtual void OnStartedUsingGpuDecode(VideoCaptureControllerID id) {}
= 0? The only three subclasses are in 
video_capture_manager_unittest.cc
video_capture_controller_unittest.cc and
video_capture_host.h and it might be good to
force having MOCK_METHOD()s to catch unexpected
calls.

Also it's strange to have this as the only non-pure
virtual method (specially since OnStarted() is =0).

https://codereview.chromium.org/2735083002/diff/60001/media/base/media_switch...
File media/base/media_switches.cc (right):

https://codereview.chromium.org/2735083002/diff/60001/media/base/media_switch...
media/base/media_switches.cc:100: // Use fake device for accelerated decoding of
jpeg. This allows, for example,
s/jpeg/JPEG/

https://codereview.chromium.org/2735083002/diff/60001/media/capture/video/vid...
File media/capture/video/video_capture_device_client.h (right):

https://codereview.chromium.org/2735083002/diff/60001/media/capture/video/vid...
media/capture/video/video_capture_device_client.h:113: bool
using_external_jpeg_decoder_;
What about removing OnUsingExternalJpegDecoder()
and |using_external_jpeg_decoder_|, instead having
a base::CallbackOnce, you bind it using BindOnce(), 
in the ctor and with base::Unretained(receiver_) and 
VideoFrameReceiver::OnStartedUsingGpuDecode().
Test before Run()ing it as you'd do with a unique_ptr.

https://codereview.chromium.org/2735083002/diff/60001/media/capture/video/vid...
File media/capture/video/video_frame_receiver.h (right):

https://codereview.chromium.org/2735083002/diff/60001/media/capture/video/vid...
media/capture/video/video_frame_receiver.h:56: virtual void
OnStartedUsingGpuDecode(){};
s/(){}/() {}/
however, same comments as above, why not make it 
pure virtual? We should only have one meaningful 
implementation and it won't hurt to MOCK and potentially
catch spurious calls in tests.

https://codereview.chromium.org/2735083002/diff/60001/media/gpu/fake_jpeg_dec...
File media/gpu/fake_jpeg_decode_accelerator.cc (right):

https://codereview.chromium.org/2735083002/diff/60001/media/gpu/fake_jpeg_dec...
media/gpu/fake_jpeg_decode_accelerator.cc:26: client_ = client;
I guess here is 
DCHECK(io_task_runner_->BelongsToCurrentThread());

[commit-bot: I haz the power, 2017-03-08 01:56:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 01:56:24]

Dry run: This issue passed the CQ dry run.

[chfremer, 2017-03-08 18:55:51]

The CQ bit was checked by chfremer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 18:56:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chfremer, 2017-03-08 18:58:08]

mcasas@: PTAL

https://codereview.chromium.org/2735083002/diff/60001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller.cc (right):

https://codereview.chromium.org/2735083002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.cc:481: }
On 2017/03/08 01:33:17, mcasas wrote:
> This code looks a lot like l.452-455 and l.467-470 (and
> perhaps others), but I don't see an easy way to generalize
> it since functions cannot have function templates.
> 
> template <typename F>
> void NotifyEventHandlerIfSessionOpen(F handler) {
>   DCHECK_CURRENTLY_ON(BrowserThread::IO);
>   for (const auto& client : controller_clients_) {
>     if (client->session_closed)
>       continue;
>     handler(client->controller_id);
>   }
> }
> 
> called e.g. like
>
NotifyEventHandlerIfSessionOpen(client->event_handler->OnStartedUsingGpuDecode);
> probably won't compile as-is :S
> 
> Not sure if this is better or worse, after writing it :-)

Without saying anything about whether or not it is worth it, I see two C++ ways
of doing what you want (not counting anything that would involve pre-processor
macros). A template approach would probably look as follows:

template <typename Action>
void VideoCaptureController::PerformForClientsWithOpenSession() {
  DCHECK_CURRENTLY_ON(BrowserThread::IO);
  for (const auto& client : controller_clients_) {
    if (client->session_closed)
      continue;
    Action::Perform(client->event_handler, client->controller_id);
  }
}

struct CallOnStartedAction {
  static void Perform(VideoCaptureControllerEventHandler* client,
                      VideoCaptureControllerID id) {
    client->OnStarted(id);
  }
};

void VideoCaptureController::OnStarted() {
  PerformForClientsWithOpenSession<CallOnStartedAction>();
}

However, the same could be achieved by passing the "actions" as runtime
parameters:

using EventHandlerAction =
    base::Callback<void(VideoCaptureControllerEventHandler* client,
                        VideoCaptureControllerID id)>;

void VideoCaptureController::PerformForClientsWithOpenSession(
    EventHandlerAction action) {
  DCHECK_CURRENTLY_ON(BrowserThread::IO);
  for (const auto& client : controller_clients_) {
    if (client->session_closed)
      continue;
    action.Run(client->event_handler, client->controller_id);
  }
}

void CallOnStarted(VideoCaptureControllerEventHandler* client,
                 VideoCaptureControllerID id) {
  client->OnStarted(id);
}

void VideoCaptureController::OnStarted() {
  PerformForClientsWithOpenSession(base::Bind(&CallOnStarted));
}

As a variation, we could use lambdas instead of file-scope functions such as
CallOnStarted().
I went with the second version in the latest patch set.

https://codereview.chromium.org/2735083002/diff/60001/content/browser/rendere...
File
content/browser/renderer_host/media/video_capture_controller_event_handler.h
(right):

https://codereview.chromium.org/2735083002/diff/60001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller_event_handler.h:60:
virtual void OnStartedUsingGpuDecode(VideoCaptureControllerID id) {}
On 2017/03/08 01:33:17, mcasas wrote:
> = 0? The only three subclasses are in 
> video_capture_manager_unittest.cc
> video_capture_controller_unittest.cc and
> video_capture_host.h and it might be good to
> force having MOCK_METHOD()s to catch unexpected
> calls.
> 
> Also it's strange to have this as the only non-pure
> virtual method (specially since OnStarted() is =0).

Done.

https://codereview.chromium.org/2735083002/diff/60001/media/base/media_switch...
File media/base/media_switches.cc (right):

https://codereview.chromium.org/2735083002/diff/60001/media/base/media_switch...
media/base/media_switches.cc:100: // Use fake device for accelerated decoding of
jpeg. This allows, for example,
On 2017/03/08 01:33:17, mcasas wrote:
> s/jpeg/JPEG/

Done.

https://codereview.chromium.org/2735083002/diff/60001/media/capture/video/vid...
File media/capture/video/video_capture_device_client.h (right):

https://codereview.chromium.org/2735083002/diff/60001/media/capture/video/vid...
media/capture/video/video_capture_device_client.h:113: bool
using_external_jpeg_decoder_;
On 2017/03/08 01:33:17, mcasas wrote:
> What about removing OnUsingExternalJpegDecoder()
> and |using_external_jpeg_decoder_|, instead having
> a base::CallbackOnce, you bind it using BindOnce(), 
> in the ctor and with base::Unretained(receiver_) and 
> VideoFrameReceiver::OnStartedUsingGpuDecode().
> Test before Run()ing it as you'd do with a unique_ptr.

Done.

https://codereview.chromium.org/2735083002/diff/60001/media/capture/video/vid...
File media/capture/video/video_frame_receiver.h (right):

https://codereview.chromium.org/2735083002/diff/60001/media/capture/video/vid...
media/capture/video/video_frame_receiver.h:56: virtual void
OnStartedUsingGpuDecode(){};
On 2017/03/08 01:33:17, mcasas wrote:
> s/(){}/() {}/
> however, same comments as above, why not make it 
> pure virtual? We should only have one meaningful 
> implementation and it won't hurt to MOCK and potentially
> catch spurious calls in tests.

Done.

https://codereview.chromium.org/2735083002/diff/60001/media/gpu/fake_jpeg_dec...
File media/gpu/fake_jpeg_decode_accelerator.cc (right):

https://codereview.chromium.org/2735083002/diff/60001/media/gpu/fake_jpeg_dec...
media/gpu/fake_jpeg_decode_accelerator.cc:26: client_ = client;
On 2017/03/08 01:33:17, mcasas wrote:
> I guess here is 
> DCHECK(io_task_runner_->BelongsToCurrentThread());

In this case it is |task_runner_|, not |io_task_runner_|.
Done.

[commit-bot: I haz the power, 2017-03-08 21:01:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 21:01:36]

Dry run: This issue passed the CQ dry run.

[mcasas, 2017-03-08 22:04:51]

lgtm with the removal below.

https://codereview.chromium.org/2735083002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller.h (right):

https://codereview.chromium.org/2735083002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.h:182: void
PerformForClientsWithOpenSessionT();
If we use Bind() to call PerformForClientsWithOpenSession()
as we do now, then this template function is not needed,
remove it.

[chfremer, 2017-03-08 22:21:22]

posciak@: PTAL

https://codereview.chromium.org/2735083002/diff/80001/content/browser/rendere...
File content/browser/renderer_host/media/video_capture_controller.h (right):

https://codereview.chromium.org/2735083002/diff/80001/content/browser/rendere...
content/browser/renderer_host/media/video_capture_controller.h:182: void
PerformForClientsWithOpenSessionT();
On 2017/03/08 22:04:51, mcasas wrote:
> If we use Bind() to call PerformForClientsWithOpenSession()
> as we do now, then this template function is not needed,
> remove it.

Oops. Thanks for catching this.
Done.

[chfremer, 2017-03-08 22:21:31]

The CQ bit was checked by chfremer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 22:22:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-09 00:32:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-09 00:32:59]

Dry run: This issue passed the CQ dry run.

[emircan, 2017-03-09 23:44:25]

https://codereview.chromium.org/2735083002/diff/100001/content/browser/render...
File content/browser/renderer_host/media/video_capture_browsertest.cc (right):

https://codereview.chromium.org/2735083002/diff/100001/content/browser/render...
content/browser/renderer_host/media/video_capture_browsertest.cc:250:
EXPECT_FALSE(must_wait_for_gpu_decode_to_start);
Since we expect to quit via |must_wait_for_gpu_decode_to_start| clause, can we
also check if |received_frame_infos| is less than |kMaxFramesToReceive|?

https://codereview.chromium.org/2735083002/diff/100001/media/gpu/fake_jpeg_de...
File media/gpu/fake_jpeg_decode_accelerator.cc (right):

https://codereview.chromium.org/2735083002/diff/100001/media/gpu/fake_jpeg_de...
media/gpu/fake_jpeg_decode_accelerator.cc:34:
DCHECK(io_task_runner_->BelongsToCurrentThread());
I think this is too much work for IO thread which needs to be very responsive.
Considering that IO thread can also be used for other GPU process tasks, it
would make sense to create your own thread here like the other JpegVDA
implementations. 
That would avoid unwanted regressions that might happen when this is in use.
However, this is only in use for your own tests. I am leaving it upto you to
call if it is worth the effort.

https://codereview.chromium.org/2735083002/diff/100001/media/gpu/fake_jpeg_de...
media/gpu/fake_jpeg_decode_accelerator.cc:57: uint8_t* dst_y =
video_frame->data(VideoFrame::kYPlane);
Can you move these inside ConvertToI420()? They aren't used anywhere else.

https://codereview.chromium.org/2735083002/diff/100001/media/gpu/fake_jpeg_de...
File media/gpu/fake_jpeg_decode_accelerator.h (right):

https://codereview.chromium.org/2735083002/diff/100001/media/gpu/fake_jpeg_de...
media/gpu/fake_jpeg_decode_accelerator.h:45: const
scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
Is this supposed to be GPU main thread task runner? Can you please specify it
using a comment or rename?

https://codereview.chromium.org/2735083002/diff/100001/media/gpu/ipc/service/...
File media/gpu/ipc/service/gpu_jpeg_decode_accelerator.cc (right):

https://codereview.chromium.org/2735083002/diff/100001/media/gpu/ipc/service/...
media/gpu/ipc/service/gpu_jpeg_decode_accelerator.cc:68:
std::move(io_task_runner));
Do we need std::move() since this is already scoped_refptr?

[chfremer, 2017-03-10 18:29:55]

The CQ bit was checked by chfremer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 18:30:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chfremer, 2017-03-10 18:30:49]

https://codereview.chromium.org/2735083002/diff/100001/content/browser/render...
File content/browser/renderer_host/media/video_capture_browsertest.cc (right):

https://codereview.chromium.org/2735083002/diff/100001/content/browser/render...
content/browser/renderer_host/media/video_capture_browsertest.cc:250:
EXPECT_FALSE(must_wait_for_gpu_decode_to_start);
On 2017/03/09 23:44:25, emircan wrote:
> Since we expect to quit via |must_wait_for_gpu_decode_to_start| clause, can we
> also check if |received_frame_infos| is less than |kMaxFramesToReceive|?

Done.

https://codereview.chromium.org/2735083002/diff/100001/media/gpu/fake_jpeg_de...
File media/gpu/fake_jpeg_decode_accelerator.cc (right):

https://codereview.chromium.org/2735083002/diff/100001/media/gpu/fake_jpeg_de...
media/gpu/fake_jpeg_decode_accelerator.cc:34:
DCHECK(io_task_runner_->BelongsToCurrentThread());
On 2017/03/09 23:44:25, emircan wrote:
> I think this is too much work for IO thread which needs to be very responsive.
> Considering that IO thread can also be used for other GPU process tasks, it
> would make sense to create your own thread here like the other JpegVDA
> implementations. 
> That would avoid unwanted regressions that might happen when this is in use.
> However, this is only in use for your own tests. I am leaving it upto you to
> call if it is worth the effort. 

Done.
Note that I still kept the call to Map() on the IO thread, which matches what
the VAAPI implementation currently does. If we want to move that off the IO
thread, I recommend we do it in a separate CL.

https://codereview.chromium.org/2735083002/diff/100001/media/gpu/fake_jpeg_de...
media/gpu/fake_jpeg_decode_accelerator.cc:57: uint8_t* dst_y =
video_frame->data(VideoFrame::kYPlane);
On 2017/03/09 23:44:25, emircan wrote:
> Can you move these inside ConvertToI420()? They aren't used anywhere else.

Done.

https://codereview.chromium.org/2735083002/diff/100001/media/gpu/fake_jpeg_de...
File media/gpu/fake_jpeg_decode_accelerator.h (right):

https://codereview.chromium.org/2735083002/diff/100001/media/gpu/fake_jpeg_de...
media/gpu/fake_jpeg_decode_accelerator.h:45: const
scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
On 2017/03/09 23:44:25, emircan wrote:
> Is this supposed to be GPU main thread task runner? Can you please specify it
> using a comment or rename?

Done.

https://codereview.chromium.org/2735083002/diff/100001/media/gpu/ipc/service/...
File media/gpu/ipc/service/gpu_jpeg_decode_accelerator.cc (right):

https://codereview.chromium.org/2735083002/diff/100001/media/gpu/ipc/service/...
media/gpu/ipc/service/gpu_jpeg_decode_accelerator.cc:68:
std::move(io_task_runner));
On 2017/03/09 23:44:25, emircan wrote:
> Do we need std::move() since this is already scoped_refptr?

I think the coding guideline is to use std::move() here, because
|io_task_runner| is no longer needed after this call in this method. 
Without the std::move(), we would pass a copy of the scoped_refptr<>, which
would (thread-safely) increase and decrease the refcount, which would mean
unnecessary work.

[commit-bot: I haz the power, 2017-03-10 20:00:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 20:00:15]

Dry run: This issue passed the CQ dry run.

[emircan, 2017-03-13 17:47:38]

https://codereview.chromium.org/2735083002/diff/100001/media/gpu/ipc/service/...
File media/gpu/ipc/service/gpu_jpeg_decode_accelerator.cc (right):

https://codereview.chromium.org/2735083002/diff/100001/media/gpu/ipc/service/...
media/gpu/ipc/service/gpu_jpeg_decode_accelerator.cc:68:
std::move(io_task_runner));
On 2017/03/10 18:30:49, chfremer wrote:
> On 2017/03/09 23:44:25, emircan wrote:
> > Do we need std::move() since this is already scoped_refptr?
> 
> I think the coding guideline is to use std::move() here, because
> |io_task_runner| is no longer needed after this call in this method. 
> Without the std::move(), we would pass a copy of the scoped_refptr<>, which
> would (thread-safely) increase and decrease the refcount, which would mean
> unnecessary work.

Sounds good.

[emircan, 2017-03-13 17:47:58]

lgtm

[chfremer, 2017-03-13 22:28:38]

chfremer@chromium.org changed reviewers:
+ sandersd@chromium.org

[chfremer, 2017-03-13 22:28:38]

sandersd@: PTAL media/*

[sandersd (OOO until July 31), 2017-03-14 23:02:35]

https://codereview.chromium.org/2735083002/diff/120001/media/capture/video/vi...
File media/capture/video/video_capture_device_client.cc (right):

https://codereview.chromium.org/2735083002/diff/120001/media/capture/video/vi...
media/capture/video/video_capture_device_client.cc:289:
std::move(on_started_using_gpu_cb_).Run();
Does std::move() actually do anything here? Is this intended to do the same
thing as base::ResetAndReturn() (if so, prefer that).

https://codereview.chromium.org/2735083002/diff/120001/media/gpu/BUILD.gn
File media/gpu/BUILD.gn (right):

https://codereview.chromium.org/2735083002/diff/120001/media/gpu/BUILD.gn#new...
media/gpu/BUILD.gn:132: "fake_jpeg_decode_accelerator.h",
Are we sure we want to always build this? It seems dangerous to have this code
accessible in the GPU process if it's not necessary.

[chfremer, 2017-03-15 17:31:05]

sandersd@: PTAL

https://codereview.chromium.org/2735083002/diff/120001/media/capture/video/vi...
File media/capture/video/video_capture_device_client.cc (right):

https://codereview.chromium.org/2735083002/diff/120001/media/capture/video/vi...
media/capture/video/video_capture_device_client.cc:289:
std::move(on_started_using_gpu_cb_).Run();
On 2017/03/14 23:02:34, sandersd wrote:
> Does std::move() actually do anything here? Is this intended to do the same
> thing as base::ResetAndReturn() (if so, prefer that).

|on_started_using_gpu_cb_| is a OnceClosure. On OnceClosure and OnceCallback,
using std::move is the standard way of calling Run(). base::ResetAndReturn does
not work on OnceCallbacks.

https://codereview.chromium.org/2735083002/diff/120001/media/gpu/BUILD.gn
File media/gpu/BUILD.gn (right):

https://codereview.chromium.org/2735083002/diff/120001/media/gpu/BUILD.gn#new...
media/gpu/BUILD.gn:132: "fake_jpeg_decode_accelerator.h",
On 2017/03/14 23:02:34, sandersd wrote:
> Are we sure we want to always build this? It seems dangerous to have this code
> accessible in the GPU process if it's not necessary.

I assumed that this would be okay, considering that we also always build
fake_video_decode_accelerator.cc. What is the concrete danger you are seeing?

[sandersd (OOO until July 31), 2017-03-15 17:38:13]

https://codereview.chromium.org/2735083002/diff/120001/media/gpu/BUILD.gn
File media/gpu/BUILD.gn (right):

https://codereview.chromium.org/2735083002/diff/120001/media/gpu/BUILD.gn#new...
media/gpu/BUILD.gn:132: "fake_jpeg_decode_accelerator.h",
On 2017/03/15 17:31:05, chfremer wrote:
> On 2017/03/14 23:02:34, sandersd wrote:
> > Are we sure we want to always build this? It seems dangerous to have this
code
> > accessible in the GPU process if it's not necessary.
> 
> I assumed that this would be okay, considering that we also always build
> fake_video_decode_accelerator.cc. What is the concrete danger you are seeing?

While I don't think either should be in a release buid, FakeVideoDecoder does
nothing but FakeJpegDecoder actually decodes JPEGs.

I do not know the JPEG stack well enough to know if this is an attack surface
increase vs. the regular accelerated path.

[chfremer, 2017-03-15 18:49:52]

The CQ bit was checked by chfremer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 18:50:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chfremer, 2017-03-15 18:51:53]

sandersd@: PTAL

As per offline discussion, we now do not actually decode the Jpeg frames. The
test only cares about the size and format of the output frame, and this is
correctly set without having to decode the Jpeg data.

[commit-bot: I haz the power, 2017-03-15 18:53:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 18:53:56]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[sandersd (OOO until July 31), 2017-03-15 18:55:52]

lgtm

https://codereview.chromium.org/2735083002/diff/140001/media/gpu/fake_jpeg_de...
File media/gpu/fake_jpeg_decode_accelerator.cc (right):

https://codereview.chromium.org/2735083002/diff/140001/media/gpu/fake_jpeg_de...
media/gpu/fake_jpeg_decode_accelerator.cc:10: #include
"media/filters/jpeg_parser.h"
Remove?

[chfremer, 2017-03-15 19:19:24]

The CQ bit was checked by chfremer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 19:19:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chfremer, 2017-03-15 19:22:22]

chfremer@chromium.org changed reviewers:
+ zmo@chromium.org

[chfremer, 2017-03-15 19:22:23]

zmo@: PTAL content/browser/gpu/*

https://codereview.chromium.org/2735083002/diff/140001/media/gpu/fake_jpeg_de...
File media/gpu/fake_jpeg_decode_accelerator.cc (right):

https://codereview.chromium.org/2735083002/diff/140001/media/gpu/fake_jpeg_de...
media/gpu/fake_jpeg_decode_accelerator.cc:10: #include
"media/filters/jpeg_parser.h"
On 2017/03/15 18:55:52, sandersd wrote:
> Remove?

Done.

[commit-bot: I haz the power, 2017-03-15 20:58:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 20:58:32]

Dry run: This issue passed the CQ dry run.

[chfremer, 2017-03-21 19:48:44]

chfremer@chromium.org changed reviewers:
+ piman@chromium.org
- sandersd@chromium.org, zmo@chromium.org

[chfremer, 2017-03-21 19:48:45]

piman@: PTAL content/browser/gpu/gpu_process_host.cc

[piman, 2017-03-21 21:26:42]

lgtm

[chfremer, 2017-03-21 21:28:11]

The CQ bit was checked by chfremer@chromium.org

[chfremer, 2017-03-21 21:28:12]

The patchset sent to the CQ was uploaded after l-g-t-m from mcasas@chromium.org,
emircan@chromium.org, sandersd@chromium.org
Link to the patchset: https://codereview.chromium.org/2735083002/#ps160001
(title: "Rebase to March 15th, Remove #include jpeg_parser")

[commit-bot: I haz the power, 2017-03-21 21:28:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 23:25:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-21 23:25:14]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[chfremer, 2017-03-22 16:46:33]

The CQ bit was checked by chfremer@chromium.org

[commit-bot: I haz the power, 2017-03-22 16:46:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 16:53:10]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1490201193934100,
"parent_rev": "c13a0740ec6fc445376f3c18c987e60a08aec306", "commit_rev":
"ac2f201c499c2d65a5d43b6ce49d9af5b4520f99"}

[commit-bot: I haz the power, 2017-03-22 16:53:59]

Description was changed from

==========
[Mojo Video Capture] Add test coverage for accelerated jpeg decoding

This CL adds test cases to VideoCaptureBrowserTest that exercise the code path 
for accelerated jpeg decoding using a fake decode accelerator.

* Add a FakeJpegDecodeAccelerator that can run in the GPU process and decodes 
  jpeg frames in software using libyuv. 
* Add a command-line flag kUseFakeJpegDecodeAccelerator to enable the fake 
  accelerator (and suppress any "real" ones if present).
* Fix threading-related DCHECKs in GpuJpegDecodeAccelerator::Client to match 
  what is actually happening. This is needed to make the new test cases pass.
  It also fixes issue 688184 [1].

This CL is part of the Mojo Video Capture work. For the bigger picture,
see [2] CL14.

BUG=584797,688184
TEST=
  content_browsertests --gtest_filter="VideoCaptureBrowserTest.*"

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=688184
[2]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
[Mojo Video Capture] Add test coverage for accelerated jpeg decoding

This CL adds test cases to VideoCaptureBrowserTest that exercise the code path 
for accelerated jpeg decoding using a fake decode accelerator.

* Add a FakeJpegDecodeAccelerator that can run in the GPU process and decodes 
  jpeg frames in software using libyuv. 
* Add a command-line flag kUseFakeJpegDecodeAccelerator to enable the fake 
  accelerator (and suppress any "real" ones if present).
* Fix threading-related DCHECKs in GpuJpegDecodeAccelerator::Client to match 
  what is actually happening. This is needed to make the new test cases pass.
  It also fixes issue 688184 [1].

This CL is part of the Mojo Video Capture work. For the bigger picture,
see [2] CL14.

BUG=584797,688184
TEST=
  content_browsertests --gtest_filter="VideoCaptureBrowserTest.*"

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=688184
[2]
https://docs.google.com/a/chromium.org/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxs...
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2735083002
Cr-Commit-Position: refs/heads/master@{#458782}
Committed:
https://chromium.googlesource.com/chromium/src/+/ac2f201c499c2d65a5d43b6ce49d...
==========

[commit-bot: I haz the power, 2017-03-22 16:54:01]

Committed patchset #8 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/ac2f201c499c2d65a5d43b6ce49d...

[chfremer, 2017-03-22 18:37:01]

A revert of this CL (patchset #8 id:160001) has been created in
https://codereview.chromium.org/2766343002/ by chfremer@chromium.org.

The reason for reverting is: Causes a ChromiumOS test failure, see
https://uberchromegw.corp.google.com/i/chromium.chromiumos/builders/Linux%20C....