Make VEA client of command buffer; move sync. IPC to VDA/VEA::Initialize()

Make VEA client of command buffer; move sync. IPC to VDA/VEA::Initialize()

Much refactoring of VDA/VEA.  Issues:
* Encoders will need to be able to synchronize with the 3D command buffer to
  support encoding from textures.
* The IPC interface from GpuVideo{Decode,Encode}AcceleratorHost to
  GpuVideo{Decode,Encode}Accelerator collapses the creation and initialization
  IPC calls into one to save a round-trip.  Unfortunately this synchronous IPC
  happens in the constructor, making the interface unwieldy (and requiring that
  initialization parameters be passed both on construction and initialization).

Solutions:
* Make the VEA a client of the command buffer.  Harmonize
  GpuVideo{Decode,Encode}AcceleratorHost implementations to make codepaths
  parallel (and easier to reason about).
* Move the synchronous IPC calls in the constructor of GpuV{D,E}AH into the
  Initialize() call, and remove the requirement that the initialization
  parameters be passed in the constructor.
* Make VEA::Initialize() a 'bool' function call, as it is done in VDA, to allow
  Initialize() to be used synchronously.
* (minor) remove SupportsWeakPtr from VDA interface.

BUG=269312
TEST=local build, run, unittests on CrOS snow

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261365

[sheu, 2014-03-06 08:56:11]

Looking pretty close to where I want it.

posciak@: you get a first look at it :-)

[sheu, 2014-03-12 23:23:07]

Update: waiting on:

  https://chromiumcodereview.appspot.com/198073003/

to fix the Windows linker issues I keep on encountering here.

[Pawel Osciak, 2014-03-13 06:18:12]

Great stuff. No major issues from me.

https://codereview.chromium.org/185403020/diff/250001/content/common/gpu/clie...
File content/common/gpu/client/gpu_channel_host.cc (right):

https://codereview.chromium.org/185403020/diff/250001/content/common/gpu/clie...
content/common/gpu/client/gpu_channel_host.cc:194: DCHECK(it != proxies_.end());
DCHECK_NE ?

https://codereview.chromium.org/185403020/diff/250001/content/common/gpu/clie...
File content/common/gpu/client/gpu_video_decode_accelerator_host.h (right):

https://codereview.chromium.org/185403020/diff/250001/content/common/gpu/clie...
content/common/gpu/client/gpu_video_decode_accelerator_host.h:29: // for
|channel_| and |impl_|.)
Nit: "." after ")".

https://codereview.chromium.org/185403020/diff/250001/content/common/gpu/medi...
File content/common/gpu/media/exynos_video_encode_accelerator.cc (right):

https://codereview.chromium.org/185403020/diff/250001/content/common/gpu/medi...
content/common/gpu/media/exynos_video_encode_accelerator.cc:39:
IOCTL_OR_ERROR_RETURN_VALUE(fd, type, arg, ((void)0))
You could also just s/((void)0)// I think?

https://codereview.chromium.org/185403020/diff/250001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://codereview.chromium.org/185403020/diff/250001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:248:
Send(init_done_msg);
These seem to beg for a common define/method...

https://codereview.chromium.org/185403020/diff/250001/content/common/gpu/medi...
File content/common/gpu/media/gpu_video_encode_accelerator.cc (right):

https://codereview.chromium.org/185403020/diff/250001/content/common/gpu/medi...
content/common/gpu/media/gpu_video_encode_accelerator.cc:67: "input_format=" <<
input_format
Nit: add "<<" and below.

[sheu, 2014-03-13 22:39:52]

Updated.  PTAL

https://chromiumcodereview.appspot.com/185403020/diff/250001/content/common/g...
File content/common/gpu/client/gpu_channel_host.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/250001/content/common/g...
content/common/gpu/client/gpu_channel_host.cc:194: DCHECK(it != proxies_.end());
On 2014/03/13 06:18:13, Pawel Osciak wrote:
> DCHECK_NE ?

Usually, but in this case having the iterator pointer print out isn't very
helpful anyways.  I'd just leave it, unless you really want it :-)

https://chromiumcodereview.appspot.com/185403020/diff/250001/content/common/g...
File content/common/gpu/client/gpu_video_decode_accelerator_host.h (right):

https://chromiumcodereview.appspot.com/185403020/diff/250001/content/common/g...
content/common/gpu/client/gpu_video_decode_accelerator_host.h:29: // for
|channel_| and |impl_|.)
On 2014/03/13 06:18:13, Pawel Osciak wrote:
> Nit: "." after ")".

Grammar bits :-)

If this was a parenthetical inside a sentence, I would put '.' after ')'.  But
since the whole sentence is a parenthetical, I put the '.' inside the ')'.

https://chromiumcodereview.appspot.com/185403020/diff/250001/content/common/g...
File content/common/gpu/media/exynos_video_encode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/250001/content/common/g...
content/common/gpu/media/exynos_video_encode_accelerator.cc:39:
IOCTL_OR_ERROR_RETURN_VALUE(fd, type, arg, ((void)0))
On 2014/03/13 06:18:13, Pawel Osciak wrote:
> You could also just s/((void)0)// I think?

I think that's a gcc-specific extension.

https://chromiumcodereview.appspot.com/185403020/diff/250001/content/common/g...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/250001/content/common/g...
content/common/gpu/media/gpu_video_decode_accelerator.cc:248:
Send(init_done_msg);
On 2014/03/13 06:18:13, Pawel Osciak wrote:
> These seem to beg for a common define/method...

Guess I could macro-ize this.

https://chromiumcodereview.appspot.com/185403020/diff/250001/content/common/g...
File content/common/gpu/media/gpu_video_encode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/250001/content/common/g...
content/common/gpu/media/gpu_video_encode_accelerator.cc:67: "input_format=" <<
input_format
On 2014/03/13 06:18:13, Pawel Osciak wrote:
> Nit: add "<<" and below.

Concatenating strings?  More "<<" not used elsewhere in this file.

[sheu, 2014-03-13 22:53:37]

Alrighty, I'm gonna add in That Guy.

Ami: this is the solution to the ugliness of the IPC and the
construction/initialization not lining up for VDA and VEA.  Also adds 3D context
support to VEA, for future encoding from textures.

fischman@: PTAL.

[Ami GONE FROM CHROMIUM, 2014-03-17 03:17:53]

I <3 this CL.

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
File content/common/gpu/client/gpu_video_decode_accelerator_host.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/client/gpu_video_decode_accelerator_host.cc:82:
NOTIFY_ERROR(PLATFORM_FAILURE) << "OnChannelError()";
This will be a no-op since the posted error notification will never run b/c
InvalidateWeakPtrs() two lines up?

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/client/gpu_video_decode_accelerator_host.cc:198: // can still
be draining, so we're forgiving and simply ignore them.
Ditto comments from GVEAH.

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
File content/common/gpu/client/gpu_video_decode_accelerator_host.h (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/client/gpu_video_decode_accelerator_host.h:90:
base::WeakPtrFactory<GpuVideoDecodeAcceleratorHost> weak_this_factory_;
s/weak_ptr/WeakPtr/

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
File content/common/gpu/client/gpu_video_encode_accelerator_host.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/client/gpu_video_encode_accelerator_host.cc:75:
NOTIFY_ERROR(kPlatformFailureError) << "OnChannelError()";
ditto bug comment from GVDAH.cc

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/client/gpu_video_encode_accelerator_host.cc:87: DLOG(ERROR)
<< "impl_ destroyed";
"impl_" doesn't really connote CommandBufferProxyImpl to me...

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/client/gpu_video_encode_accelerator_host.cc:203: // can still
be draining, so we're forgiving and simply ignore them.
It's not very forgiving to try to send on channel_ when it's NULL because
OnChannelError!

(I see you added some if(channel_) guards around some calls to Send(), but it's
unclear to me why that's better than just early-returning here if !channel_ like
the code used to do)

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/gpu_command_buffer_stub.cc:758: decoder->Initialize(profile,
reply_message);
It is at best suspicious that the return value is ignored.

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/gpu_command_buffer_stub.cc:773:
encoder->Initialize(input_format,
ditto

(should the interfaces use WARN_UNUSED_RESULT?)

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
File content/common/gpu/media/exynos_video_encode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/media/exynos_video_encode_accelerator.cc:39:
IOCTL_OR_ERROR_RETURN_VALUE(fd, type, arg, ((void)0))
s/((void)0)// ?

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/media/gpu_video_decode_accelerator.cc:240:
Send(init_done_msg);                                                     \
s/init_done_msg/msg/
but why not just a helper method instead of a function?

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/media/gpu_video_decode_accelerator.cc:252:
WRITE_REPLY_AND_SEND(init_done_msg, -1);
I thought route ID of "0" was error.  But here I see -1.  Is that right?

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
File content/common/gpu/media/gpu_video_encode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/media/gpu_video_encode_accelerator.cc:28: if (!stub.get()) {
.get() shouldn't be necessary (search weak_ptr.h for "Testable")

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/media/gpu_video_encode_accelerator.cc:63:
Send(init_done_msg);                                                     \
again, this should just be |msg|, and should be in a helper function not macro.

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
File content/common/gpu/media/gpu_video_encode_accelerator.h (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/media/gpu_video_encode_accelerator.h:90: // |stub_| goes is
destroyed.
"goes is"

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
File content/common/gpu/media/v4l2_video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/media/v4l2_video_decode_accelerator.h:429: // The
WeakPtrFactory for |weak_this_|.
No point in having weak_this_ anymore, is there?
(just GetWeakPtr() when you need it)

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
File content/common/gpu/media/vaapi_video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/media/vaapi_video_decode_accelerator.h:271: // The
WeakPtrFactory for |weak_this_|.
ditto can drop

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
File content/common/gpu/media/video_encode_accelerator_unittest.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/media/video_encode_accelerator_unittest.cc:394: if
(!encoder_) {
unpossible

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/renderer...
File content/renderer/media/rtc_video_encoder.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/renderer...
content/renderer/media/rtc_video_encoder.cc:194:
NOTIFY_ERROR(media::VideoEncodeAccelerator::kInvalidArgumentError);
choice of error to return here seems vague; why not blame the platform for the
error (e.g. out of resources)?

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/renderer...
File content/renderer/pepper/pepper_video_capture_host.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/renderer...
content/renderer/pepper/pepper_video_capture_host.cc:14: #include
"media/base/video_frame.h"
??

https://chromiumcodereview.appspot.com/185403020/diff/270001/media/video/vide...
File media/video/video_decode_accelerator.h (left):

https://chromiumcodereview.appspot.com/185403020/diff/270001/media/video/vide...
media/video/video_decode_accelerator.h:23: : public
base::SupportsWeakPtr<VideoDecodeAccelerator> {
\o/

https://chromiumcodereview.appspot.com/185403020/diff/270001/media/video/vide...
File media/video/video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/media/video/vide...
media/video/video_decode_accelerator.h:72: // Callback to notify about decoding
errors.
Doco this can only be called after Initialize() returned true at some point.

https://chromiumcodereview.appspot.com/185403020/diff/270001/media/video/vide...
File media/video/video_encode_accelerator.h (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/media/video/vide...
media/video/video_encode_accelerator.h:84: virtual void NotifyError(Error error)
= 0;
Ditto

[Ami GONE FROM CHROMIUM, 2014-03-17 03:19:04]

On 2014/03/13 22:53:37, sheu wrote:
> Alrighty, I'm gonna add in That Guy.

nit: you forgot the (tm) following Guy.
Just sayin'.

[sheu, 2014-03-18 22:38:35]

Updated, PTAL.

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
File content/common/gpu/client/gpu_video_decode_accelerator_host.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/client/gpu_video_decode_accelerator_host.cc:82:
NOTIFY_ERROR(PLATFORM_FAILURE) << "OnChannelError()";
On 2014/03/17 03:17:54, Ami Fischman wrote:
> This will be a no-op since the posted error notification will never run b/c
> InvalidateWeakPtrs() two lines up?

I suppose.  Making this a DLOG(ERROR) instead.

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
File content/common/gpu/client/gpu_video_decode_accelerator_host.h (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/client/gpu_video_decode_accelerator_host.h:90:
base::WeakPtrFactory<GpuVideoDecodeAcceleratorHost> weak_this_factory_;
On 2014/03/17 03:17:54, Ami Fischman wrote:
> s/weak_ptr/WeakPtr/

Done.

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
File content/common/gpu/client/gpu_video_encode_accelerator_host.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/client/gpu_video_encode_accelerator_host.cc:75:
NOTIFY_ERROR(kPlatformFailureError) << "OnChannelError()";
On 2014/03/17 03:17:54, Ami Fischman wrote:
> ditto bug comment from GVDAH.cc

Done.

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/client/gpu_video_encode_accelerator_host.cc:87: DLOG(ERROR)
<< "impl_ destroyed";
On 2014/03/17 03:17:54, Ami Fischman wrote:
> "impl_" doesn't really connote CommandBufferProxyImpl to me...

Sure, but it also keeps us from having to update comments if the class name
changes.  I figure if you're debugging the message you'll be able to tell where
this is coming from.

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/client/gpu_video_encode_accelerator_host.cc:203: // can still
be draining, so we're forgiving and simply ignore them.
On 2014/03/17 03:17:54, Ami Fischman wrote:
> It's not very forgiving to try to send on channel_ when it's NULL because
> OnChannelError!
> 
> (I see you added some if(channel_) guards around some calls to Send(), but
it's
> unclear to me why that's better than just early-returning here if !channel_
like
> the code used to do)

There's some call-site that early-out already on !channel_, so I figured I'd
un-redundantify the checks.

Removed the comment.

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/gpu_command_buffer_stub.cc:758: decoder->Initialize(profile,
reply_message);
On 2014/03/17 03:17:54, Ami Fischman wrote:
> It is at best suspicious that the return value is ignored.

Aha!  Note that GpuVideoDecodeAccelerator _doesn't actually_ implement
VideoDecodeAccelelerator.  (A little confusing maybe.)  Its Initialize() returns
void.

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/gpu_command_buffer_stub.cc:773:
encoder->Initialize(input_format,
On 2014/03/17 03:17:54, Ami Fischman wrote:
> ditto
> 
> (should the interfaces use WARN_UNUSED_RESULT?)

See above.

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
File content/common/gpu/media/exynos_video_encode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/media/exynos_video_encode_accelerator.cc:39:
IOCTL_OR_ERROR_RETURN_VALUE(fd, type, arg, ((void)0))
On 2014/03/17 03:17:54, Ami Fischman wrote:
> s/((void)0)// ?

Works only as a GCC extension that way ;-)

This is more spec-compliant.

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/media/gpu_video_decode_accelerator.cc:240:
Send(init_done_msg);                                                     \
On 2014/03/17 03:17:54, Ami Fischman wrote:
> s/init_done_msg/msg/
> but why not just a helper method instead of a function?

Done.

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/media/gpu_video_decode_accelerator.cc:252:
WRITE_REPLY_AND_SEND(init_done_msg, -1);
On 2014/03/17 03:17:54, Ami Fischman wrote:
> I thought route ID of "0" was error.  But here I see -1.  Is that right?

On the renderer side it checks " < 0 " for error.

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
File content/common/gpu/media/gpu_video_encode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/media/gpu_video_encode_accelerator.cc:28: if (!stub.get()) {
On 2014/03/17 03:17:54, Ami Fischman wrote:
> .get() shouldn't be necessary (search weak_ptr.h for "Testable")

Done.

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/media/gpu_video_encode_accelerator.cc:63:
Send(init_done_msg);                                                     \
On 2014/03/17 03:17:54, Ami Fischman wrote:
> again, this should just be |msg|, and should be in a helper function not
macro.

Done.

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
File content/common/gpu/media/gpu_video_encode_accelerator.h (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/media/gpu_video_encode_accelerator.h:90: // |stub_| goes is
destroyed.
On 2014/03/17 03:17:54, Ami Fischman wrote:
> "goes is"

Done.

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
File content/common/gpu/media/v4l2_video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/media/v4l2_video_decode_accelerator.h:429: // The
WeakPtrFactory for |weak_this_|.
On 2014/03/17 03:17:54, Ami Fischman wrote:
> No point in having weak_this_ anymore, is there?
> (just GetWeakPtr() when you need it)

Careful what you wish for.  It's threading all the way down. :-)

weak_this_factory_ is used from the child thread, so it's unsafe to GetWeakPtr()
from the decoder thread when it posts tasks back.

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
File content/common/gpu/media/vaapi_video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/media/vaapi_video_decode_accelerator.h:271: // The
WeakPtrFactory for |weak_this_|.
On 2014/03/17 03:17:54, Ami Fischman wrote:
> ditto can drop

See before.

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
File content/common/gpu/media/video_encode_accelerator_unittest.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/media/video_encode_accelerator_unittest.cc:394: if
(!encoder_) {
On 2014/03/17 03:17:54, Ami Fischman wrote:
> unpossible

Ha.  I guess.

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/renderer...
File content/renderer/media/rtc_video_encoder.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/renderer...
content/renderer/media/rtc_video_encoder.cc:194:
NOTIFY_ERROR(media::VideoEncodeAccelerator::kInvalidArgumentError);
On 2014/03/17 03:17:54, Ami Fischman wrote:
> choice of error to return here seems vague; why not blame the platform for the
> error (e.g. out of resources)?

I think it's more likely that the encoder doesn't support this profile or other
input arguments.

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/renderer...
File content/renderer/pepper/pepper_video_capture_host.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/renderer...
content/renderer/pepper/pepper_video_capture_host.cc:14: #include
"media/base/video_frame.h"
On 2014/03/17 03:17:54, Ami Fischman wrote:
> ??

Actually necessary.  I removed some includes of video_decode_accelerator.h that
leaves media::VideoFrame undefined now in certain places.

A net positive.

https://chromiumcodereview.appspot.com/185403020/diff/270001/media/video/vide...
File media/video/video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/media/video/vide...
media/video/video_decode_accelerator.h:72: // Callback to notify about decoding
errors.
On 2014/03/17 03:17:54, Ami Fischman wrote:
> Doco this can only be called after Initialize() returned true at some point.

Done.

https://chromiumcodereview.appspot.com/185403020/diff/270001/media/video/vide...
File media/video/video_encode_accelerator.h (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/media/video/vide...
media/video/video_encode_accelerator.h:84: virtual void NotifyError(Error error)
= 0;
On 2014/03/17 03:17:54, Ami Fischman wrote:
> Ditto

Done.

[Ami GONE FROM CHROMIUM, 2014-03-18 23:53:18]

LGTM % nits (though I think there are a couple of bugs noted below)

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/270001/content/common/g...
content/common/gpu/gpu_command_buffer_stub.cc:758: decoder->Initialize(profile,
reply_message);
On 2014/03/18 22:38:35, sheu wrote:
> On 2014/03/17 03:17:54, Ami Fischman wrote:
> > It is at best suspicious that the return value is ignored.
> 
> Aha!  Note that GpuVideoDecodeAccelerator _doesn't actually_ implement
> VideoDecodeAccelelerator.  (A little confusing maybe.)  Its Initialize()
returns
> void.

lolnice

https://chromiumcodereview.appspot.com/185403020/diff/290001/content/common/g...
File content/common/gpu/client/gpu_video_decode_accelerator_host.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/290001/content/common/g...
content/common/gpu/client/gpu_video_decode_accelerator_host.cc:82: DLOG(ERROR)
<< "OnChannelError()";
ditto GVEAH

https://chromiumcodereview.appspot.com/185403020/diff/290001/content/common/g...
File content/common/gpu/client/gpu_video_encode_accelerator_host.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/290001/content/common/g...
content/common/gpu/client/gpu_video_encode_accelerator_host.cc:75: DLOG(ERROR)
<< "OnChannelError()";
Don't you want to notify the client in case of channel error?!?

https://chromiumcodereview.appspot.com/185403020/diff/290001/content/common/g...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/290001/content/common/g...
content/common/gpu/gpu_command_buffer_stub.cc:797:
context_group_->mailbox_manager()->PushTextureUpdates();
Assuming this is rebase.

https://chromiumcodereview.appspot.com/185403020/diff/290001/content/common/g...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/290001/content/common/g...
content/common/gpu/media/gpu_video_decode_accelerator.cc:204: DebugAutoLock
auto_lock(debug_uncleared_textures_lock_);
this is tricky business; it replaces a run-time DCHECK_IS_ON() test with a
compile-time DCHECK_IS_ON #if, but then the next line is conditionalized on the
runtime switch again.

I think this change makes things worse and would revert it, or, better yet, make
it a helper function
void DCheckNotAnUnclearedPicture(const Picture& picture) {
  if (!DCHECK_IS_ON())
    return;
  base::AutoLock auto_lock(&debug_uncleared_textures_lock_);
  CHECK_EQ(0u, uncleared_textures_.count(picture.picture_buffer_id()));
}


(and drop DebugAutoLock)

https://chromiumcodereview.appspot.com/185403020/diff/290001/content/renderer...
File content/renderer/media/rtc_video_encoder.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/290001/content/renderer...
content/renderer/media/rtc_video_encoder.cc:213: // If there are no free input
and output buffers, drop the frame to avoid a
Didn't a brilliant (and handsome!) man once tell you not to rebase while there
are outstanding comments?  Pretty sure that happened...

[sheu, 2014-03-19 20:38:24]

Adding OWNERs.

sievers@: PTAL at the content/common/gpu/client/* and content/common/gpu/gpu_*
bits, thanks!  Most changes should be just plumbing.

https://chromiumcodereview.appspot.com/185403020/diff/290001/content/common/g...
File content/common/gpu/client/gpu_video_decode_accelerator_host.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/290001/content/common/g...
content/common/gpu/client/gpu_video_decode_accelerator_host.cc:82: DLOG(ERROR)
<< "OnChannelError()";
On 2014/03/18 23:53:19, Ami Fischman wrote:
> ditto GVEAH

Done.

https://chromiumcodereview.appspot.com/185403020/diff/290001/content/common/g...
File content/common/gpu/client/gpu_video_encode_accelerator_host.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/290001/content/common/g...
content/common/gpu/client/gpu_video_encode_accelerator_host.cc:75: DLOG(ERROR)
<< "OnChannelError()";
On 2014/03/18 23:53:19, Ami Fischman wrote:
> Don't you want to notify the client in case of channel error?!?

Oh, right.  I was thinking for some reason this was GPU process-side (in which
case the channel is broke and there's no client to notify.

https://chromiumcodereview.appspot.com/185403020/diff/290001/content/common/g...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/290001/content/common/g...
content/common/gpu/gpu_command_buffer_stub.cc:797:
context_group_->mailbox_manager()->PushTextureUpdates();
On 2014/03/18 23:53:19, Ami Fischman wrote:
> Assuming this is rebase.

Forgot, sorry.  Yes it's rebase.

https://chromiumcodereview.appspot.com/185403020/diff/290001/content/common/g...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/290001/content/common/g...
content/common/gpu/media/gpu_video_decode_accelerator.cc:204: DebugAutoLock
auto_lock(debug_uncleared_textures_lock_);
On 2014/03/18 23:53:19, Ami Fischman wrote:
> this is tricky business; it replaces a run-time DCHECK_IS_ON() test with a
> compile-time DCHECK_IS_ON #if, but then the next line is conditionalized on
the
> runtime switch again.
> 
> I think this change makes things worse and would revert it, or, better yet,
make
> it a helper function
> void DCheckNotAnUnclearedPicture(const Picture& picture) {
>   if (!DCHECK_IS_ON())
>     return;
>   base::AutoLock auto_lock(&debug_uncleared_textures_lock_);
>   CHECK_EQ(0u, uncleared_textures_.count(picture.picture_buffer_id()));
> }
> 
> 
> (and drop DebugAutoLock)

Rebase change :-(

DCHECK_IS_ON is now a compile-time constant.  See http://crbug.com/350462 for
details.

https://chromiumcodereview.appspot.com/185403020/diff/290001/content/renderer...
File content/renderer/media/rtc_video_encoder.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/290001/content/renderer...
content/renderer/media/rtc_video_encoder.cc:213: // If there are no free input
and output buffers, drop the frame to avoid a
On 2014/03/18 23:53:19, Ami Fischman wrote:
> Didn't a brilliant (and handsome!) man once tell you not to rebase while there
> are outstanding comments?  Pretty sure that happened...

:-(

Forgot about my Monday morning sync/rebase.

[sheu, 2014-03-19 20:38:56]

Actually adding sievers@.  Description in previous comment, thanks!

[no sievers, 2014-03-19 21:33:15]

lgtm

https://chromiumcodereview.appspot.com/185403020/diff/310001/content/common/g...
File content/common/gpu/client/gpu_video_decode_accelerator_host.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/310001/content/common/g...
content/common/gpu/client/gpu_video_decode_accelerator_host.cc:31:
decoder_route_id_(-1),
MSG_ROUTING_NONE

https://chromiumcodereview.appspot.com/185403020/diff/310001/content/common/g...
content/common/gpu/client/gpu_video_decode_accelerator_host.cc:43: if (channel_)
if (channel && decoder_route_id_ != MSG_ROUTING_NONE) in case init failed

https://chromiumcodereview.appspot.com/185403020/diff/310001/content/common/g...
content/common/gpu/client/gpu_video_decode_accelerator_host.cc:78:
channel_->RemoveRoute(decoder_route_id_);
also maybe check for route_id to be valid

https://chromiumcodereview.appspot.com/185403020/diff/310001/content/common/g...
content/common/gpu/client/gpu_video_decode_accelerator_host.cc:93: if
(decoder_route_id_ < 0) {
In other places we do '!= MSG_ROUTING_NONE' to check if it's valid.

https://chromiumcodereview.appspot.com/185403020/diff/310001/content/common/g...
File content/common/gpu/client/gpu_video_decode_accelerator_host.h (right):

https://chromiumcodereview.appspot.com/185403020/diff/310001/content/common/g...
content/common/gpu/client/gpu_video_decode_accelerator_host.h:28: // Both
|channel| and |impl| are guaranteed to outlive |this|.  (See comments
Doesn't look like that's true, since you are observing destruction of |impl|.

https://chromiumcodereview.appspot.com/185403020/diff/310001/content/common/g...
content/common/gpu/client/gpu_video_decode_accelerator_host.h:73: // process. 
|channel_| outlives |impl_|, so |channel_| also outlives |this|.
see above. You should rather say that |channel| may be NULL if it was lost.

https://chromiumcodereview.appspot.com/185403020/diff/310001/content/common/g...
content/common/gpu/client/gpu_video_decode_accelerator_host.h:83: // registers
as a DeletionObserver of |impl_|, so |impl_| outlives |this|.
see above. You should rather say that |impl| may be NULL.

https://chromiumcodereview.appspot.com/185403020/diff/310001/content/common/g...
File content/common/gpu/client/gpu_video_encode_accelerator_host.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/310001/content/common/g...
content/common/gpu/client/gpu_video_encode_accelerator_host.cc:24:
encoder_route_id_(-1),
MSG_ROUTING_NONE

https://chromiumcodereview.appspot.com/185403020/diff/310001/content/common/g...
File content/common/gpu/client/gpu_video_encode_accelerator_host.h (right):

https://chromiumcodereview.appspot.com/185403020/diff/310001/content/common/g...
content/common/gpu/client/gpu_video_encode_accelerator_host.h:42: // Both
|channel| and |impl| are guaranteed to outlive |this|.  (See comments
see comments in gpu_video_decode_accelerator_host.h

[sheu, 2014-03-20 00:30:31]

https://chromiumcodereview.appspot.com/185403020/diff/310001/content/common/g...
File content/common/gpu/client/gpu_video_decode_accelerator_host.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/310001/content/common/g...
content/common/gpu/client/gpu_video_decode_accelerator_host.cc:31:
decoder_route_id_(-1),
On 2014/03/19 21:33:16, sievers wrote:
> MSG_ROUTING_NONE

Done.

https://chromiumcodereview.appspot.com/185403020/diff/310001/content/common/g...
content/common/gpu/client/gpu_video_decode_accelerator_host.cc:43: if (channel_)
On 2014/03/19 21:33:16, sievers wrote:
> if (channel && decoder_route_id_ != MSG_ROUTING_NONE) in case init failed

Done.

https://chromiumcodereview.appspot.com/185403020/diff/310001/content/common/g...
content/common/gpu/client/gpu_video_decode_accelerator_host.cc:78:
channel_->RemoveRoute(decoder_route_id_);
On 2014/03/19 21:33:16, sievers wrote:
> also maybe check for route_id to be valid

Done.

https://chromiumcodereview.appspot.com/185403020/diff/310001/content/common/g...
content/common/gpu/client/gpu_video_decode_accelerator_host.cc:93: if
(decoder_route_id_ < 0) {
On 2014/03/19 21:33:16, sievers wrote:
> In other places we do '!= MSG_ROUTING_NONE' to check if it's valid.

Done.

https://chromiumcodereview.appspot.com/185403020/diff/310001/content/common/g...
File content/common/gpu/client/gpu_video_decode_accelerator_host.h (right):

https://chromiumcodereview.appspot.com/185403020/diff/310001/content/common/g...
content/common/gpu/client/gpu_video_decode_accelerator_host.h:28: // Both
|channel| and |impl| are guaranteed to outlive |this|.  (See comments
On 2014/03/19 21:33:16, sievers wrote:
> Doesn't look like that's true, since you are observing destruction of |impl|.

The difference as I meant it is that the pointer is valid for the entire
lifetime, guaranteed by the fact that we observe destruction so we will never
see an invalid pointer.  I'm going to try to reword this.

https://chromiumcodereview.appspot.com/185403020/diff/310001/content/common/g...
content/common/gpu/client/gpu_video_decode_accelerator_host.h:73: // process. 
|channel_| outlives |impl_|, so |channel_| also outlives |this|.
On 2014/03/19 21:33:16, sievers wrote:
> see above. You should rather say that |channel| may be NULL if it was lost.

channel_ is set in the constructor, so it may be valid even if it is lost, if
this->Initialize() isn't called yet.

My meaning here is that the reference is valid (e.g. not dangling) whenever it
is not NULL.

https://chromiumcodereview.appspot.com/185403020/diff/310001/content/common/g...
content/common/gpu/client/gpu_video_decode_accelerator_host.h:83: // registers
as a DeletionObserver of |impl_|, so |impl_| outlives |this|.
On 2014/03/19 21:33:16, sievers wrote:
> see above. You should rather say that |impl| may be NULL.

Done.

https://chromiumcodereview.appspot.com/185403020/diff/310001/content/common/g...
File content/common/gpu/client/gpu_video_encode_accelerator_host.cc (right):

https://chromiumcodereview.appspot.com/185403020/diff/310001/content/common/g...
content/common/gpu/client/gpu_video_encode_accelerator_host.cc:24:
encoder_route_id_(-1),
On 2014/03/19 21:33:16, sievers wrote:
> MSG_ROUTING_NONE

Done.

https://chromiumcodereview.appspot.com/185403020/diff/310001/content/common/g...
File content/common/gpu/client/gpu_video_encode_accelerator_host.h (right):

https://chromiumcodereview.appspot.com/185403020/diff/310001/content/common/g...
content/common/gpu/client/gpu_video_encode_accelerator_host.h:42: // Both
|channel| and |impl| are guaranteed to outlive |this|.  (See comments
On 2014/03/19 21:33:16, sievers wrote:
> see comments in gpu_video_decode_accelerator_host.h

Done.

[sheu, 2014-03-20 01:10:16]

Last bits.  Pretty simple.

cevans@: PTAL at content/common/gpu/gpu_messages.h
bbudge@: PTAL at content/renderer/pepper/*.

[bbudge, 2014-03-20 17:18:58]

content/renderer/pepper LGTM

[sheu, 2014-03-21 23:21:52]

jschuh@: PTAL at content/common/gpu/gpu_messages.h, thanks.

[jschuh, 2014-03-22 00:29:49]

ipc security lgtm (note: no change in attack surface)

[sheu, 2014-03-22 00:48:27]

Yay red bots

Found the bug.  Fixed in:

    https://chromiumcodereview.appspot.com/208653009/

will try again after that lands.

[sheu, 2014-03-22 00:49:37]

(whoops, ignore the above.  Wrong CL.)

[sheu, 2014-03-22 01:01:21]

The CQ bit was checked by sheu@chromium.org

[commit-bot: I haz the power, 2014-03-22 01:02:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/185403020/350001

[commit-bot: I haz the power, 2014-03-22 01:55:19]

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

[commit-bot: I haz the power, 2014-03-22 01:55:19]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[sheu, 2014-03-22 08:13:14]

The CQ bit was checked by sheu@chromium.org

[commit-bot: I haz the power, 2014-03-22 08:13:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/185403020/350001

[commit-bot: I haz the power, 2014-03-22 08:13:32]

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

[commit-bot: I haz the power, 2014-03-22 08:13:32]

Failed to apply patch for chrome/renderer/media/cast_rtp_stream.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/renderer/media/cast_rtp_stream.cc
  Hunk #1 FAILED at 18.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/renderer/media/cast_rtp_stream.cc.rej

Patch:       chrome/renderer/media/cast_rtp_stream.cc
Index: chrome/renderer/media/cast_rtp_stream.cc
diff --git a/chrome/renderer/media/cast_rtp_stream.cc
b/chrome/renderer/media/cast_rtp_stream.cc
index
de71a43b618d43053995e73f45888d7c63dec191..413bcc8193b807de3e72d4730bd64bf8a321ce3c
100644
--- a/chrome/renderer/media/cast_rtp_stream.cc
+++ b/chrome/renderer/media/cast_rtp_stream.cc
@@ -18,6 +18,7 @@
 #include "media/base/audio_fifo.h"
 #include "media/base/bind_to_current_loop.h"
 #include "media/base/multi_channel_resampler.h"
+#include "media/base/video_frame.h"
 #include "media/cast/cast_config.h"
 #include "media/cast/cast_defines.h"
 #include "media/cast/cast_sender.h"

[sheu, 2014-03-22 08:28:52]

The CQ bit was checked by sheu@chromium.org

[commit-bot: I haz the power, 2014-03-22 08:29:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/185403020/390001

[commit-bot: I haz the power, 2014-03-22 09:07:32]

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

[commit-bot: I haz the power, 2014-03-22 09:07:33]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[sheu, 2014-03-23 21:48:13]

The CQ bit was checked by sheu@chromium.org

[commit-bot: I haz the power, 2014-03-23 21:48:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/185403020/420001

[commit-bot: I haz the power, 2014-03-23 22:03:29]

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

[commit-bot: I haz the power, 2014-03-23 22:03:30]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[sheu, 2014-03-24 23:27:18]

The CQ bit was checked by sheu@chromium.org

[commit-bot: I haz the power, 2014-03-24 23:28:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/185403020/440001

[commit-bot: I haz the power, 2014-03-25 00:19:50]

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

[commit-bot: I haz the power, 2014-03-25 00:19:51]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[sheu, 2014-03-25 00:20:23]

The CQ bit was checked by sheu@chromium.org

[commit-bot: I haz the power, 2014-03-25 00:21:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/185403020/440001

[commit-bot: I haz the power, 2014-03-25 00:53:22]

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

[commit-bot: I haz the power, 2014-03-25 00:53:23]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[sheu, 2014-04-02 20:20:57]

The CQ bit was checked by sheu@chromium.org

[commit-bot: I haz the power, 2014-04-02 20:21:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/185403020/560001

[commit-bot: I haz the power, 2014-04-03 06:36:11]

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

[commit-bot: I haz the power, 2014-04-03 06:36:12]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[sheu, 2014-04-03 06:53:34]

The CQ bit was checked by sheu@chromium.org

[commit-bot: I haz the power, 2014-04-03 06:55:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/185403020/560001

[commit-bot: I haz the power, 2014-04-03 08:32:41]

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

[commit-bot: I haz the power, 2014-04-03 08:32:42]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[sheu, 2014-04-03 08:52:59]

The CQ bit was checked by sheu@chromium.org

[commit-bot: I haz the power, 2014-04-03 08:54:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/185403020/560001

[commit-bot: I haz the power, 2014-04-03 12:12:10]

Change committed as 261365