Add Mojo interfaces for GpuJpegDecodeAccelerator and GpuJpegDecodeAcceleratorHost

Add Mojo interfaces for GpuJpegDecodeAccelerator and GpuJpegDecodeAcceleratorHost

This CL is the first step towards mojification of GpuJpegDecodeAccelerator(GJDA)
and GpuJpegDecodeAcceleratorHost(GJDAH).

1. Defines media::BitstreamBuffer and AcceleratedJpegDecoderMsg_Decode_Params for mojo.
2. GJDA and GJDAH implement these interfaces. Currently, these implementations are empty.

BUG=699255
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/2905823002
Cr-Commit-Position: refs/heads/master@{#476712}
Committed: https://chromium.googlesource.com/chromium/src/+/eed40bc79df20716a280b62fb3d1f1f0a40df0a1

[Chandan, 2017-05-25 12:44:55]

The CQ bit was checked by c.padhi@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-25 12:45:04]

Dry run: CQ is trying da patch.

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

[Chandan, 2017-05-25 13:41:28]

The CQ bit was checked by c.padhi@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-25 13:41:42]

Dry run: CQ is trying da patch.

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

[Chandan, 2017-05-25 15:02:14]

c.padhi@samsung.com changed reviewers:
+ chfremer@chromium.org, wuchengli@chromium.org

[Chandan, 2017-05-25 15:02:14]

PTAL. Thank you.

[commit-bot: I haz the power, 2017-05-25 15:13:07]

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

[commit-bot: I haz the power, 2017-05-25 15:13:08]

Dry run: This issue passed the CQ dry run.

[chfremer, 2017-05-25 21:46:28]

lgtm

My feeling is that this CL is almost a bit too small. I would like to also see
the interfaces where these things will get used in, but I am sure this will be
part of a follow-up.

[Chandan, 2017-05-26 02:28:10]

On 2017/05/25 21:46:28, chfremer wrote:
> lgtm
> 
> My feeling is that this CL is almost a bit too small. I would like to also see
> the interfaces where these things will get used in, but I am sure this will be
> part of a follow-up.

Sure. I will come up with follow-up CLs shortly.

[Chandan, 2017-05-26 02:28:22]

The CQ bit was checked by c.padhi@samsung.com

[commit-bot: I haz the power, 2017-05-26 02:29:30]

CQ is trying da patch.

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

[Chandan, 2017-05-26 02:36:33]

c.padhi@samsung.com changed reviewers:
+ tsepez@chromium.org

[Chandan, 2017-05-26 02:36:34]

tsepez@, PTAL. Thank you.

[commit-bot: I haz the power, 2017-05-26 02:40:16]

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

[commit-bot: I haz the power, 2017-05-26 02:40:18]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Chandan, 2017-05-26 13:29:01]

Description was changed from

==========
Define media::BitstreamBuffer and AcceleratedJpegDecoderMsg_Decode_Params for
mojo

This CL is the first step towards mojification of GpuJpegDecodeAccelerator/Host.

BUG=699255
==========

to

==========
Define media::BitstreamBuffer and AcceleratedJpegDecoderMsg_Decode_Params for
mojo

This CL is the first step towards mojification of GpuJpegDecodeAccelerator/Host.

BUG=699255
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
==========

[Chandan, 2017-05-26 13:31:07]

Description was changed from

==========
Define media::BitstreamBuffer and AcceleratedJpegDecoderMsg_Decode_Params for
mojo

This CL is the first step towards mojification of GpuJpegDecodeAccelerator/Host.

BUG=699255
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

==========
Add Mojo interfaces for GpuJpegDecodeAccelerator and
GpuJpegDecodeAcceleratorHost

This CL is the first step towards mojification of GpuJpegDecodeAccelerator(GJDA)
and GpuJpegDecodeAcceleratorHost(GJDAH).

1. Defines media::BitstreamBuffer and AcceleratedJpegDecoderMsg_Decode_Params
for mojo.
2. GJDA and GJDAH implement these interfaces. Currently, these implementations
are empty.
3. Removes base::NonThreadSafe from GJDA and GJDAH.

BUG=699255
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
==========

[Chandan, 2017-05-26 13:32:40]

The CQ bit was checked by c.padhi@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 13:32:51]

Dry run: CQ is trying da patch.

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

[mcasas, 2017-05-26 14:39:59]

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

[mcasas, 2017-05-26 14:40:00]

Some thoughts.  Also please use Polygerrit ;-)
https://polygerrit.appspot.com/

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

https://codereview.chromium.org/2905823002/diff/40001/media/gpu/ipc/service/g...
media/gpu/ipc/service/gpu_jpeg_decode_accelerator.cc:459:
CreateJpegDecoderCallback callback) {}
Until you implement these methods, write
NOTIMPLEMENTED() << ...
and mention the bug number to be tracked.

https://codereview.chromium.org/2905823002/diff/40001/media/gpu/mojo/jpeg_dec...
File media/gpu/mojo/jpeg_decoder.mojom (right):

https://codereview.chromium.org/2905823002/diff/40001/media/gpu/mojo/jpeg_dec...
media/gpu/mojo/jpeg_decoder.mojom:16: };
I would move struct JpegDecodeInfo here, what'd be
the rationale of having it elsewhere if it's only used 
in this interface?

https://codereview.chromium.org/2905823002/diff/40001/media/gpu/mojo/jpeg_dec...
media/gpu/mojo/jpeg_decoder.mojom:26:
CreateJpegDecoder(GpuJpegDecodeAcceleratorClient client) => (bool success);
Why not:
  CreateJpegDecoder() =>
    (GpuJpegDecodeAcceleratorClient client)
and return a null GpuJpegDecodeAcceleratorClientPtr if 
somethings goes wrong?

https://codereview.chromium.org/2905823002/diff/40001/media/gpu/mojo/jpeg_dec...
media/gpu/mojo/jpeg_decoder.mojom:33: Decode(media.mojom.JpegDecodeInfo info);
Same idea here:

  Decode() =>
    (JpegDecodeInfo info)

(you don't need media.mojom) and return a nullptr 
media.mojom.JpegDecodeInfoPtr if something goes wrong?

https://codereview.chromium.org/2905823002/diff/40001/media/gpu/mojo/jpeg_dec...
media/gpu/mojo/jpeg_decoder.mojom:36: Destroy();
This method is confusing: does it destroy a Decode() 
request, a |client|, a GpuDecodeAccelerator...? It might
even be not needed given mojo move-only ownership
semantics. 

If the idea is to Destroy() a GpuJpegDecodeAccelerator,
then this method is not necessary: the client of this API
should kill the unique_ptr<> holding the Accelerator, and
the Accelerator should act upon pipe destruction, see
https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/interface_ptr.h...

[Chandan, 2017-05-26 15:10:11]

https://codereview.chromium.org/2905823002/diff/40001/media/gpu/mojo/jpeg_dec...
File media/gpu/mojo/jpeg_decoder.mojom (right):

https://codereview.chromium.org/2905823002/diff/40001/media/gpu/mojo/jpeg_dec...
media/gpu/mojo/jpeg_decoder.mojom:16: };
On 2017/05/26 14:39:59, mcasas wrote:
> I would move struct JpegDecodeInfo here, what'd be
> the rationale of having it elsewhere if it's only used 
> in this interface?

Initial idea behind this CL was only to add these mojo structs. However, today I
updated this CL with interface changes as well and in the process missed out on
this. Thanks for pointing this out.

https://codereview.chromium.org/2905823002/diff/40001/media/gpu/mojo/jpeg_dec...
media/gpu/mojo/jpeg_decoder.mojom:26:
CreateJpegDecoder(GpuJpegDecodeAcceleratorClient client) => (bool success);
On 2017/05/26 14:39:59, mcasas wrote:
> Why not:
>   CreateJpegDecoder() =>
>     (GpuJpegDecodeAcceleratorClient client)
> and return a null GpuJpegDecodeAcceleratorClientPtr if 
> somethings goes wrong?

You mean return a null GpuJpegDecodeAcceleratorClientPtr in the callback itself
instead of the boolean?
And what about the GpuJpegDecodeAcceleratorClientPtr that is passed as input to
the method? The idea was to make GpuJpegDecodeAccelerator use this input to call
the ack OnDecodeAck().

https://codereview.chromium.org/2905823002/diff/40001/media/gpu/mojo/jpeg_dec...
media/gpu/mojo/jpeg_decoder.mojom:36: Destroy();
On 2017/05/26 14:39:59, mcasas wrote:
> This method is confusing: does it destroy a Decode() 
> request, a |client|, a GpuDecodeAccelerator...? It might
> even be not needed given mojo move-only ownership
> semantics. 
> 
> If the idea is to Destroy() a GpuJpegDecodeAccelerator,
> then this method is not necessary: the client of this API
> should kill the unique_ptr<> holding the Accelerator, and
> the Accelerator should act upon pipe destruction, see
>
https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/interface_ptr.h...

I think existing implementation in GpuJpegDecodeAccelerator removes a client. It
might not be required as we migrate to mojo.
I had mapped the existing IPC messages with these interface methods as it is for
now. Would clean up this as I progress further?

[mcasas, 2017-05-26 15:36:14]

https://codereview.chromium.org/2905823002/diff/40001/media/gpu/mojo/jpeg_dec...
File media/gpu/mojo/jpeg_decoder.mojom (right):

https://codereview.chromium.org/2905823002/diff/40001/media/gpu/mojo/jpeg_dec...
media/gpu/mojo/jpeg_decoder.mojom:26:
CreateJpegDecoder(GpuJpegDecodeAcceleratorClient client) => (bool success);
On 2017/05/26 15:10:11, Chandan wrote:
> On 2017/05/26 14:39:59, mcasas wrote:
> > Why not:
> >   CreateJpegDecoder() =>
> >     (GpuJpegDecodeAcceleratorClient client)
> > and return a null GpuJpegDecodeAcceleratorClientPtr if 
> > somethings goes wrong?
> 
> You mean return a null GpuJpegDecodeAcceleratorClientPtr in the callback
itself
> instead of the boolean?
> And what about the GpuJpegDecodeAcceleratorClientPtr that is passed as input
to
> the method? The idea was to make GpuJpegDecodeAccelerator use this input to
call
> the ack OnDecodeAck().

OIC, so it's an inout param. Hmmm. See my comments below.

https://codereview.chromium.org/2905823002/diff/40001/media/gpu/mojo/jpeg_dec...
media/gpu/mojo/jpeg_decoder.mojom:36: Destroy();
On 2017/05/26 15:10:11, Chandan wrote:
> On 2017/05/26 14:39:59, mcasas wrote:
> > This method is confusing: does it destroy a Decode() 
> > request, a |client|, a GpuDecodeAccelerator...? It might
> > even be not needed given mojo move-only ownership
> > semantics. 
> > 
> > If the idea is to Destroy() a GpuJpegDecodeAccelerator,
> > then this method is not necessary: the client of this API
> > should kill the unique_ptr<> holding the Accelerator, and
> > the Accelerator should act upon pipe destruction, see
> >
>
https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/interface_ptr.h...
> 
> I think existing implementation in GpuJpegDecodeAccelerator removes a client.
It
> might not be required as we migrate to mojo.
> I had mapped the existing IPC messages with these interface methods as it is
for
> now. Would clean up this as I progress further?

If it's a verbatim copy, then it's fine to progress as is , but 
please add a 
// TODO(c.padhi@samsung.com):... https://crbug.com/xyzxyz
so we don't forget to clean this up.

IPC based messages didn't have an easy time sending
objects nor tying the lifetime of objects to the lifetime of
a channel, but mojo is really good at that, so patterns such 
as
{
  auto client = service->Connect();
  DCHECK(client);
  client->DoStuff();
}
are encouraged, since they "clean themselves" up when
|client| goes out of scope (closing the connection, which
can be used to destroy the "server" side resources in a 
controlled way).  This was nearly impossible in IPC, and
so you see all kinds of explicit cleanup logic.

An example of service doing things "by the book" is webusb,
where a manager creates a device [1] which is then 
manipulated [2] (UsbDevice has a Reset() which doesn't
imply destruction, FTR).


[1]
https://cs.chromium.org/chromium/src/device/usb/public/interfaces/device_mana...
[2]
https://cs.chromium.org/chromium/src/device/usb/public/interfaces/device.mojo...

[commit-bot: I haz the power, 2017-05-26 16:13:08]

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

[commit-bot: I haz the power, 2017-05-26 16:13:09]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2017-05-26 16:42:44]

Where is the existing "IPC" equivalent?  Can any of that be removed along with
this change?

[chfremer, 2017-05-26 18:12:13]

https://codereview.chromium.org/2905823002/diff/40001/media/gpu/mojo/jpeg_dec...
File media/gpu/mojo/jpeg_decoder.mojom (right):

https://codereview.chromium.org/2905823002/diff/40001/media/gpu/mojo/jpeg_dec...
media/gpu/mojo/jpeg_decoder.mojom:26:
CreateJpegDecoder(GpuJpegDecodeAcceleratorClient client) => (bool success);
Since this does not seem to produce and return an independent object, it looks
to me like this should probably be called "Initialize" (similar to what seems to
be the case in class GpuJpegDecodeAcceleratorHost). Accordingly, the method
Destroy() should probably be called "Uninitialize".

[Chandan, 2017-05-29 10:44:34]

The CQ bit was checked by c.padhi@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-29 10:44:46]

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-05-29 11:50:23]

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

[commit-bot: I haz the power, 2017-05-29 11:50:24]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Chandan, 2017-05-29 13:02:34]

PTAL @ps4.

@Tom Sepez, you can find the existing IPCs here:
https://cs.chromium.org/chromium/src/media/gpu/ipc/common/media_messages.h?l=211
This CL has empty implementations for the interface methods. The IPCs will be
removed in follow up CLs as we implement these methods.

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

https://codereview.chromium.org/2905823002/diff/40001/media/gpu/ipc/service/g...
media/gpu/ipc/service/gpu_jpeg_decode_accelerator.cc:459:
CreateJpegDecoderCallback callback) {}
On 2017/05/26 14:39:59, mcasas wrote:
> Until you implement these methods, write
> NOTIMPLEMENTED() << ...
> and mention the bug number to be tracked.

Done.

https://codereview.chromium.org/2905823002/diff/40001/media/gpu/mojo/jpeg_dec...
File media/gpu/mojo/jpeg_decoder.mojom (right):

https://codereview.chromium.org/2905823002/diff/40001/media/gpu/mojo/jpeg_dec...
media/gpu/mojo/jpeg_decoder.mojom:16: };
On 2017/05/26 14:39:59, mcasas wrote:
> I would move struct JpegDecodeInfo here, what'd be
> the rationale of having it elsewhere if it's only used 
> in this interface?

Done.

https://codereview.chromium.org/2905823002/diff/40001/media/gpu/mojo/jpeg_dec...
media/gpu/mojo/jpeg_decoder.mojom:26:
CreateJpegDecoder(GpuJpegDecodeAcceleratorClient client) => (bool success);
On 2017/05/26 18:12:13, chfremer wrote:
> Since this does not seem to produce and return an independent object, it looks
> to me like this should probably be called "Initialize" (similar to what seems
to
> be the case in class GpuJpegDecodeAcceleratorHost). Accordingly, the method
> Destroy() should probably be called "Uninitialize".

Done.

https://codereview.chromium.org/2905823002/diff/40001/media/gpu/mojo/jpeg_dec...
media/gpu/mojo/jpeg_decoder.mojom:33: Decode(media.mojom.JpegDecodeInfo info);
On 2017/05/26 14:39:59, mcasas wrote:
> Same idea here:
> 
>   Decode() =>
>     (JpegDecodeInfo info)
> 
> (you don't need media.mojom) and return a nullptr 
> media.mojom.JpegDecodeInfoPtr if something goes wrong?

Deleted media.mojom.

https://codereview.chromium.org/2905823002/diff/40001/media/gpu/mojo/jpeg_dec...
media/gpu/mojo/jpeg_decoder.mojom:36: Destroy();
On 2017/05/26 15:36:13, mcasas wrote:
> On 2017/05/26 15:10:11, Chandan wrote:
> > On 2017/05/26 14:39:59, mcasas wrote:
> > > This method is confusing: does it destroy a Decode() 
> > > request, a |client|, a GpuDecodeAccelerator...? It might
> > > even be not needed given mojo move-only ownership
> > > semantics. 
> > > 
> > > If the idea is to Destroy() a GpuJpegDecodeAccelerator,
> > > then this method is not necessary: the client of this API
> > > should kill the unique_ptr<> holding the Accelerator, and
> > > the Accelerator should act upon pipe destruction, see
> > >
> >
>
https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/interface_ptr.h...
> > 
> > I think existing implementation in GpuJpegDecodeAccelerator removes a
client.
> It
> > might not be required as we migrate to mojo.
> > I had mapped the existing IPC messages with these interface methods as it is
> for
> > now. Would clean up this as I progress further?
> 
> If it's a verbatim copy, then it's fine to progress as is , but 
> please add a 
> // TODO(c.padhi@samsung.com):... https://crbug.com/xyzxyz
> so we don't forget to clean this up.
 
Done.

[Chandan, 2017-05-30 08:32:26]

The CQ bit was checked by c.padhi@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-30 08:32:42]

Dry run: CQ is trying da patch.

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

[Chandan, 2017-05-30 09:00:38]

Patchset #5 (id:80001) has been deleted

[Chandan, 2017-05-30 10:01:46]

The CQ bit was checked by c.padhi@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-30 10:02:07]

Dry run: CQ is trying da patch.

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

[Chandan, 2017-05-30 11:19:33]

Patchset #5 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-05-30 11:33:31]

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

[commit-bot: I haz the power, 2017-05-30 11:33:32]

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

[Chandan, 2017-05-30 12:54:25]

PTAL. Thank you.

[Tom Sepez, 2017-05-30 17:27:42]

lgtm

[chfremer, 2017-05-30 18:07:25]

PatchSet 5 lgtm % nit

https://codereview.chromium.org/2905823002/diff/120001/media/gpu/mojo/jpeg_de...
File media/gpu/mojo/jpeg_decoder.mojom (right):

https://codereview.chromium.org/2905823002/diff/120001/media/gpu/mojo/jpeg_de...
media/gpu/mojo/jpeg_decoder.mojom:34: Initialize(GpuJpegDecodeAcceleratorClient
client) => (bool success);
nit: Comment is slightly confusing. If we say "Creates a hardware decoder" it
sounds like we can create more than one and use them simultaneously. But since
Uninitialize() does not take any parameters, the API really only allows for a
single decoder at a time.

I recommend removing the method-level comments for Initialize() and
Uninitialize() and instead use an interface-level comment to explaing that
Initialize() must be called before using Decode() and Uninitialize() must be
called before releasing the instance (assuming you do not want to drop the
method altogether and do the uninitialization automatically on releasing).

[mcasas, 2017-05-30 18:15:19]

On 2017/05/30 18:07:25, chfremer wrote:
> PatchSet 5 lgtm % nit
> 
>
https://codereview.chromium.org/2905823002/diff/120001/media/gpu/mojo/jpeg_de...
> File media/gpu/mojo/jpeg_decoder.mojom (right):
> 
>
https://codereview.chromium.org/2905823002/diff/120001/media/gpu/mojo/jpeg_de...
> media/gpu/mojo/jpeg_decoder.mojom:34:
Initialize(GpuJpegDecodeAcceleratorClient
> client) => (bool success);
> nit: Comment is slightly confusing. If we say "Creates a hardware decoder" it
> sounds like we can create more than one and use them simultaneously. But since
> Uninitialize() does not take any parameters, the API really only allows for a
> single decoder at a time.
> 
> I recommend removing the method-level comments for Initialize() and
> Uninitialize() and instead use an interface-level comment to explaing that
> Initialize() must be called before using Decode() and Uninitialize() must be
> called before releasing the instance (assuming you do not want to drop the
> method altogether and do the uninitialization automatically on releasing).

I discussed the naming situation with the s13n team and it's OK
to do the mojification/servicification in two steps:

a) convert existing IPC to mojo leaving the syntax and
 naming as similar as possible
b) rename mojo interfaces to better reflect the semantics.

My recommendation to speed up reviews is to follow up this
pattern.

[Chandan, 2017-05-31 12:00:40]

The CQ bit was checked by c.padhi@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-31 12:00:57]

Dry run: CQ is trying da patch.

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

[Chandan, 2017-05-31 13:19:57]

c.padhi@samsung.com changed reviewers:
+ dalecurtis@chromium.org

[Chandan, 2017-05-31 13:19:58]

Thanks for the review.
I think we need a lgtm from media/gpu OWNERS as well for this CL to land.

+dalecurtis
wuchengli@, dalecurtis@: PTAL.

https://codereview.chromium.org/2905823002/diff/120001/media/gpu/mojo/jpeg_de...
File media/gpu/mojo/jpeg_decoder.mojom (right):

https://codereview.chromium.org/2905823002/diff/120001/media/gpu/mojo/jpeg_de...
media/gpu/mojo/jpeg_decoder.mojom:34: Initialize(GpuJpegDecodeAcceleratorClient
client) => (bool success);
On 2017/05/30 18:07:24, chfremer wrote:
> nit: Comment is slightly confusing. If we say "Creates a hardware decoder" it
> sounds like we can create more than one and use them simultaneously. But since
> Uninitialize() does not take any parameters, the API really only allows for a
> single decoder at a time.
> 
> I recommend removing the method-level comments for Initialize() and
> Uninitialize() and instead use an interface-level comment to explaing that
> Initialize() must be called before using Decode() and Uninitialize() must be
> called before releasing the instance (assuming you do not want to drop the
> method altogether and do the uninitialization automatically on releasing).

Uninitialize() is most likely to be removed. Hence, I did not add any comment
w.r.t it for now. In case, we keep Uninitialize(), I will update accordingly.

[commit-bot: I haz the power, 2017-05-31 13:51:07]

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

[commit-bot: I haz the power, 2017-05-31 13:51:09]

Dry run: This issue passed the CQ dry run.

[DaleCurtis, 2017-05-31 20:38:42]

dalecurtis@chromium.org changed reviewers:
+ sandersd@chromium.org
- dalecurtis@chromium.org

[DaleCurtis, 2017-05-31 20:38:43]

=>sandersd for review.

[sandersd (OOO until July 31), 2017-06-01 00:07:19]

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

[sandersd (OOO until July 31), 2017-06-01 00:07:20]

+xhwang@ to review addition of //media/gpu/mojo directory.

What is the fundamental reason for this addition? Previously I have found
//media/mojo + //media/gpu/ipc to cover all the cases.

[sandersd (OOO until July 31), 2017-06-01 00:15:24]

On 2017/06/01 00:07:20, sandersd wrote:
> +xhwang@ to review addition of //media/gpu/mojo directory.
> 
> What is the fundamental reason for this addition? Previously I have found
> //media/mojo + //media/gpu/ipc to cover all the cases.

To clarify: we put our mojo stubs and proxies in //media/mojo, and require that
//media not depend on //media/mojo. Perhaps a better choice here is to move the
impls into that tree.

I'm not sure from this CL how the JPEG service will be registered. If it's via
the media service, then we may want to go as far as splitting it into a mojo IPC
half and a decoder impl half, like is done for the MojoAudioDecoder and
MojoVideoDecoder cases. (Not a happy though, admittedly, but it is the current
style for media services.)

[Chandan, 2017-06-01 03:30:23]

On 2017/06/01 00:15:24, sandersd wrote:
> On 2017/06/01 00:07:20, sandersd wrote:
> > +xhwang@ to review addition of //media/gpu/mojo directory.
> > 
> > What is the fundamental reason for this addition? Previously I have found
> > //media/mojo + //media/gpu/ipc to cover all the cases.
> 
> To clarify: we put our mojo stubs and proxies in //media/mojo, and require
that
> //media not depend on //media/mojo. Perhaps a better choice here is to move
the
> impls into that tree.
> 
> I'm not sure from this CL how the JPEG service will be registered. If it's via
> the media service, then we may want to go as far as splitting it into a mojo
IPC
> half and a decoder impl half, like is done for the MojoAudioDecoder and
> MojoVideoDecoder cases. (Not a happy though, admittedly, but it is the current
> style for media services.)

Since GJDAH and GJDA are present in //media/gpu/ipc, I chose to put their mojom
interfaces in //media/gpu/mojo.
I will move the jpeg_decoder.mojom to //media/mojo as per your suggestion.

In follow up CLs, the idea is to register GJDA interface(service) with
GpuChildThread's InterfaceRegistry in gpu process for exposing it to GJDAH in
browser process. We can move GJDAH and GJDA out of //media/gpu/ipc into
//media/mojo/clients and //media/mojo/services respectively?
I am open to suggestions in this regard.

Also, if you could shed some light on the possibility of registering JPEG
service via media service?

The intention is to restrict this CL to defining mojo structures and interfaces
only and take up the implementation in subsequent CLs.

[xhwang, 2017-06-01 05:17:03]

On 2017/06/01 03:30:23, Chandan wrote:
> On 2017/06/01 00:15:24, sandersd wrote:
> > On 2017/06/01 00:07:20, sandersd wrote:
> > > +xhwang@ to review addition of //media/gpu/mojo directory.
> > > 
> > > What is the fundamental reason for this addition? Previously I have found
> > > //media/mojo + //media/gpu/ipc to cover all the cases.
> > 
> > To clarify: we put our mojo stubs and proxies in //media/mojo, and require
> that
> > //media not depend on //media/mojo. Perhaps a better choice here is to move
> the
> > impls into that tree.
> > 
> > I'm not sure from this CL how the JPEG service will be registered. If it's
via
> > the media service, then we may want to go as far as splitting it into a mojo
> IPC
> > half and a decoder impl half, like is done for the MojoAudioDecoder and
> > MojoVideoDecoder cases. (Not a happy though, admittedly, but it is the
current
> > style for media services.)
> 
> Since GJDAH and GJDA are present in //media/gpu/ipc, I chose to put their
mojom
> interfaces in //media/gpu/mojo.
> I will move the jpeg_decoder.mojom to //media/mojo as per your suggestion.
> 
> In follow up CLs, the idea is to register GJDA interface(service) with
> GpuChildThread's InterfaceRegistry in gpu process for exposing it to GJDAH in
> browser process. We can move GJDAH and GJDA out of //media/gpu/ipc into
> //media/mojo/clients and //media/mojo/services respectively?
> I am open to suggestions in this regard.
> 
> Also, if you could shed some light on the possibility of registering JPEG
> service via media service?
> 
> The intention is to restrict this CL to defining mojo structures and
interfaces
> only and take up the implementation in subsequent CLs.

MediaService exposes mojo interfaces for the HTML5 <video> media pipeline
running in the renderer process. Your use case seems very different from that,
so keeping things in media/gpu sgtm.

Is this part of migrating media/gpu/ipc to mojo? If so, media/gpu/mojo looks
good to me as well. But you probably want to have TODO() somewhere to move the
client/service to media/gpu/mojo as well. Or, you can put everything under
media/gpu/ipc, since mojo is another way to do IPC. After everything is
migrated, you can rename media/gpu/ipc to media/gpu/mojo.

[xhwang, 2017-06-01 05:17:19]

https://codereview.chromium.org/2905823002/diff/140001/media/gpu/mojo/jpeg_de...
File media/gpu/mojo/jpeg_decoder.mojom (right):

https://codereview.chromium.org/2905823002/diff/140001/media/gpu/mojo/jpeg_de...
media/gpu/mojo/jpeg_decoder.mojom:32: interface GpuJpegDecodeAccelerator {
Depending on how this interface interacts with GpuJpegDecodeAcceleratorClient
interface, you might want to use associated interfaces:

https://www.chromium.org/developers/design-documents/mojo/associated-interfaces

https://codereview.chromium.org/2905823002/diff/140001/media/gpu/mojo/jpeg_de...
media/gpu/mojo/jpeg_decoder.mojom:37: // and the size of JPEG image is
|coded_size|. Decoded I420 frame data will
These seem to belong to line 18.

https://codereview.chromium.org/2905823002/diff/140001/media/gpu/mojo/jpeg_de...
media/gpu/mojo/jpeg_decoder.mojom:38: // be put onto shared memory associated
with |output_video_frame_handle|
|output_video_frame_handle| not defined anywhere, do you mean |output_handle|?

https://codereview.chromium.org/2905823002/diff/140001/media/gpu/mojo/jpeg_de...
media/gpu/mojo/jpeg_decoder.mojom:40: Decode(JpegDecodeInfo info);
Is it a one-to-one mapping between Decode() and OnDecodeAck()? If so, we should
use a callback. If not, it might worth a comment about the relation between the
two.

[Chandan, 2017-06-01 05:54:26]

https://codereview.chromium.org/2905823002/diff/140001/media/gpu/mojo/jpeg_de...
File media/gpu/mojo/jpeg_decoder.mojom (right):

https://codereview.chromium.org/2905823002/diff/140001/media/gpu/mojo/jpeg_de...
media/gpu/mojo/jpeg_decoder.mojom:32: interface GpuJpegDecodeAccelerator {
On 2017/06/01 05:17:19, xhwang wrote:
> Depending on how this interface interacts with GpuJpegDecodeAcceleratorClient
> interface, you might want to use associated interfaces:
> 
>
https://www.chromium.org/developers/design-documents/mojo/associated-interfaces

GpuJpegDecodeAccelerator will use GpuJpegDecodeAcceleratorClient only to make
the OnDecodeAck() call. Do we need to use associated interfaces for this?

https://codereview.chromium.org/2905823002/diff/140001/media/gpu/mojo/jpeg_de...
media/gpu/mojo/jpeg_decoder.mojom:37: // and the size of JPEG image is
|coded_size|. Decoded I420 frame data will
On 2017/06/01 05:17:19, xhwang wrote:
> These seem to belong to line 18.

I have mapped the existing browser to gpu IPC messages with interface methods of
GpuJpegDecodeAccelerator as it is for now. Borrowed this comment from
https://cs.chromium.org/chromium/src/media/gpu/ipc/common/media_messages.h?l=220.

https://codereview.chromium.org/2905823002/diff/140001/media/gpu/mojo/jpeg_de...
media/gpu/mojo/jpeg_decoder.mojom:37: // and the size of JPEG image is
|coded_size|. Decoded I420 frame data will
On 2017/06/01 05:17:19, xhwang wrote:
> These seem to belong to line 18.

May be I need to rename these fields here similar to those in JpegDecodeInfo
above.

https://codereview.chromium.org/2905823002/diff/140001/media/gpu/mojo/jpeg_de...
media/gpu/mojo/jpeg_decoder.mojom:40: Decode(JpegDecodeInfo info);
On 2017/06/01 05:17:19, xhwang wrote:
> Is it a one-to-one mapping between Decode() and OnDecodeAck()? If so, we
should
> use a callback. If not, it might worth a comment about the relation between
the
> two.

Decode() and OnDecodeAck() are seperate calls, each one mapped to an existing
IPC message. Please refer
https://cs.chromium.org/chromium/src/media/gpu/ipc/common/media_messages.h?l=220

[Chandan, 2017-06-01 05:59:42]

On 2017/06/01 05:17:03, xhwang wrote:
> On 2017/06/01 03:30:23, Chandan wrote:
> > On 2017/06/01 00:15:24, sandersd wrote:
> > > On 2017/06/01 00:07:20, sandersd wrote:
> > > > +xhwang@ to review addition of //media/gpu/mojo directory.
> > > > 
> > > > What is the fundamental reason for this addition? Previously I have
found
> > > > //media/mojo + //media/gpu/ipc to cover all the cases.
> > > 
> > > To clarify: we put our mojo stubs and proxies in //media/mojo, and require
> > that
> > > //media not depend on //media/mojo. Perhaps a better choice here is to
move
> > the
> > > impls into that tree.
> > > 
> > > I'm not sure from this CL how the JPEG service will be registered. If it's
> via
> > > the media service, then we may want to go as far as splitting it into a
mojo
> > IPC
> > > half and a decoder impl half, like is done for the MojoAudioDecoder and
> > > MojoVideoDecoder cases. (Not a happy though, admittedly, but it is the
> current
> > > style for media services.)
> > 
> > Since GJDAH and GJDA are present in //media/gpu/ipc, I chose to put their
> mojom
> > interfaces in //media/gpu/mojo.
> > I will move the jpeg_decoder.mojom to //media/mojo as per your suggestion.
> > 
> > In follow up CLs, the idea is to register GJDA interface(service) with
> > GpuChildThread's InterfaceRegistry in gpu process for exposing it to GJDAH
in
> > browser process. We can move GJDAH and GJDA out of //media/gpu/ipc into
> > //media/mojo/clients and //media/mojo/services respectively?
> > I am open to suggestions in this regard.
> > 
> > Also, if you could shed some light on the possibility of registering JPEG
> > service via media service?
> > 
> > The intention is to restrict this CL to defining mojo structures and
> interfaces
> > only and take up the implementation in subsequent CLs.
> 
> MediaService exposes mojo interfaces for the HTML5 <video> media pipeline
> running in the renderer process. Your use case seems very different from that,
> so keeping things in media/gpu sgtm.
> 
> Is this part of migrating media/gpu/ipc to mojo? If so, media/gpu/mojo looks
> good to me as well. But you probably want to have TODO() somewhere to move the
> client/service to media/gpu/mojo as well. Or, you can put everything under
> media/gpu/ipc, since mojo is another way to do IPC. After everything is
> migrated, you can rename media/gpu/ipc to media/gpu/mojo.

Yes, this CL is the first step towards migrating GpuJpegDecodeAccelerator/Host
to Mojo.
I was going to move GpuJpegDecodeAccelerator/Host to media/gpu/mojo in a follow
up CL.
I will add TODOs in gpu_jpeg_decode_accelerator/host.h files for now. Thank you.

[Chandan, 2017-06-01 11:53:30]

The CQ bit was checked by c.padhi@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-01 11:53:39]

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-06-01 13:06:11]

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

[commit-bot: I haz the power, 2017-06-01 13:06:12]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Chandan, 2017-06-01 13:37:50]

PTAL @ps7. Thank you.

https://codereview.chromium.org/2905823002/diff/160001/media/gpu/mojo/jpeg_de...
File media/gpu/mojo/jpeg_decoder.mojom (right):

https://codereview.chromium.org/2905823002/diff/160001/media/gpu/mojo/jpeg_de...
media/gpu/mojo/jpeg_decoder.mojom:33: [Sync]
Sorry, didn't realize that the existing equivalent IPC message is synchronous.
Hence, added [Sync] attribute here.
https://cs.chromium.org/chromium/src/media/gpu/ipc/common/media_messages.h?ty...

[xhwang, 2017-06-01 16:59:01]

https://codereview.chromium.org/2905823002/diff/160001/media/gpu/mojo/jpeg_de...
File media/gpu/mojo/jpeg_decoder.mojom (right):

https://codereview.chromium.org/2905823002/diff/160001/media/gpu/mojo/jpeg_de...
media/gpu/mojo/jpeg_decoder.mojom:19: struct JpegDecodeInfo {
This struct is not documented. You probably want to move most of the comments
above Decode() here.

https://codereview.chromium.org/2905823002/diff/160001/media/gpu/mojo/jpeg_de...
media/gpu/mojo/jpeg_decoder.mojom:28: OnDecodeAck(int32 bitstream_buffer_id,
Error error);
I see that you copied the old comments here. But I don't feel the these comments
are clear enough. For example, do we always expect one and only one
OnDecodeAck() call for each Decode() call?

https://codereview.chromium.org/2905823002/diff/160001/media/gpu/mojo/jpeg_de...
media/gpu/mojo/jpeg_decoder.mojom:33: [Sync]
On 2017/06/01 13:37:50, Chandan wrote:
> Sorry, didn't realize that the existing equivalent IPC message is synchronous.
> Hence, added [Sync] attribute here.
>
https://cs.chromium.org/chromium/src/media/gpu/ipc/common/media_messages.h?ty...

Is there a compelling reason why this must be synchronous?

Sync mojo calls are strongly discouraged:
https://www.chromium.org/developers/design-documents/mojo/synchronous-calls

https://codereview.chromium.org/2905823002/diff/160001/media/gpu/mojo/jpeg_de...
media/gpu/mojo/jpeg_decoder.mojom:40: // |output_handle| with size limit
|output_buffer_size|.
I know you are copying this from the IPC message. But now since we are touching
it, it's the chance to make it better.

Does it make sense to separate input and output instead of putting them in one
opaque |info|?

Does it make sense to return the output in OnDecodeAck() call, or in a callback
of Decode()?

https://codereview.chromium.org/2905823002/diff/160001/media/gpu/mojo/jpeg_de...
media/gpu/mojo/jpeg_decoder.mojom:40: // |output_handle| with size limit
|output_buffer_size|.
For |output_buffer_size|, if it's a limit, it should be called *limit...

https://codereview.chromium.org/2905823002/diff/160001/media/mojo/interfaces/...
File media/mojo/interfaces/media_types.mojom (right):

https://codereview.chromium.org/2905823002/diff/160001/media/mojo/interfaces/...
media/mojo/interfaces/media_types.mojom:176: struct BitstreamBuffer {
sandersd: Will you also need this for the mojo VideoDecoder work? I wonder
whether we should just put this under media/gpu/mojo as well...

[sandersd (OOO until July 31), 2017-06-01 18:13:34]

https://codereview.chromium.org/2905823002/diff/160001/media/mojo/interfaces/...
File media/mojo/interfaces/media_types.mojom (right):

https://codereview.chromium.org/2905823002/diff/160001/media/mojo/interfaces/...
media/mojo/interfaces/media_types.mojom:176: struct BitstreamBuffer {
On 2017/06/01 16:59:01, xhwang wrote:
> sandersd: Will you also need this for the mojo VideoDecoder work? I wonder
> whether we should just put this under media/gpu/mojo as well...

No, MVD transports DecoderBuffers.

I am implementing GPU-side support for converting DecoderBuffers to
BitstreamBuffers, for compatibility with VDAs, but it's an ugly hack due to
BitstreamBuffer forcing us to use platform SHM.

I plan to implement an SHM virtual interface and change BitstreamBuffer to use
it, so that I can substitute the platform SHM type for a non-shared buffer.

[Chandan, 2017-06-01 18:38:23]

https://codereview.chromium.org/2905823002/diff/160001/media/gpu/mojo/jpeg_de...
File media/gpu/mojo/jpeg_decoder.mojom (right):

https://codereview.chromium.org/2905823002/diff/160001/media/gpu/mojo/jpeg_de...
media/gpu/mojo/jpeg_decoder.mojom:19: struct JpegDecodeInfo {
On 2017/06/01 16:59:01, xhwang wrote:
> This struct is not documented. You probably want to move most of the comments
> above Decode() here.

Sure. I will add comments wherever necessary in this file.
If anything specific, please suggest.

https://codereview.chromium.org/2905823002/diff/160001/media/gpu/mojo/jpeg_de...
media/gpu/mojo/jpeg_decoder.mojom:28: OnDecodeAck(int32 bitstream_buffer_id,
Error error);
On 2017/06/01 16:59:01, xhwang wrote:
> I see that you copied the old comments here. But I don't feel the these
comments
> are clear enough. For example, do we always expect one and only one
> OnDecodeAck() call for each Decode() call?

I am new to this part of the code. However, as I see in the code, we may have
one OnDecodeAck() call for each Decode() call.

https://codereview.chromium.org/2905823002/diff/160001/media/gpu/mojo/jpeg_de...
media/gpu/mojo/jpeg_decoder.mojom:33: [Sync]
On 2017/06/01 16:59:01, xhwang wrote:
> On 2017/06/01 13:37:50, Chandan wrote:
> > Sorry, didn't realize that the existing equivalent IPC message is
synchronous.
> > Hence, added [Sync] attribute here.
> >
>
https://cs.chromium.org/chromium/src/media/gpu/ipc/common/media_messages.h?ty...
> 
> Is there a compelling reason why this must be synchronous?
> 
> Sync mojo calls are strongly discouraged:
> https://www.chromium.org/developers/design-documents/mojo/synchronous-calls

Since the existing implementation is synchronous, I chose to keep it that way.
As I see, JpegDecodeAccelerator interface expects Initialize(..) to be
synchronous.
Please refer:
https://cs.chromium.org/chromium/src/media/video/jpeg_decode_accelerator.h?ty...
I am not too sure though since I am not aware of the history as why this was
made synchronous.

https://codereview.chromium.org/2905823002/diff/160001/media/gpu/mojo/jpeg_de...
media/gpu/mojo/jpeg_decoder.mojom:40: // |output_handle| with size limit
|output_buffer_size|.
On 2017/06/01 16:59:01, xhwang wrote:
> I know you are copying this from the IPC message. But now since we are
touching
> it, it's the chance to make it better.
Sure.
> Does it make sense to separate input and output instead of putting them in one
> opaque |info|?
I think we can do this. Two separate structs in that case?
> Does it make sense to return the output in OnDecodeAck() call, or in a
callback
> of Decode()?
We can do this too. A callback of Decode() would eliminate the need of a
separate OnDecodeAck(). I will check further.

[xhwang, 2017-06-01 18:45:54]

On 2017/06/01 18:13:34, sandersd wrote:
>
https://codereview.chromium.org/2905823002/diff/160001/media/mojo/interfaces/...
> File media/mojo/interfaces/media_types.mojom (right):
> 
>
https://codereview.chromium.org/2905823002/diff/160001/media/mojo/interfaces/...
> media/mojo/interfaces/media_types.mojom:176: struct BitstreamBuffer {
> On 2017/06/01 16:59:01, xhwang wrote:
> > sandersd: Will you also need this for the mojo VideoDecoder work? I wonder
> > whether we should just put this under media/gpu/mojo as well...
> 
> No, MVD transports DecoderBuffers.
> 
> I am implementing GPU-side support for converting DecoderBuffers to
> BitstreamBuffers, for compatibility with VDAs, but it's an ugly hack due to
> BitstreamBuffer forcing us to use platform SHM.
> 
> I plan to implement an SHM virtual interface and change BitstreamBuffer to use
> it, so that I can substitute the platform SHM type for a non-shared buffer.

Thanks! In that case, please just move this definition to media/gpu/mojo as
well.

[xhwang, 2017-06-01 18:49:19]

https://codereview.chromium.org/2905823002/diff/160001/media/gpu/mojo/jpeg_de...
File media/gpu/mojo/jpeg_decoder.mojom (right):

https://codereview.chromium.org/2905823002/diff/160001/media/gpu/mojo/jpeg_de...
media/gpu/mojo/jpeg_decoder.mojom:33: [Sync]
On 2017/06/01 18:38:23, Chandan wrote:
> On 2017/06/01 16:59:01, xhwang wrote:
> > On 2017/06/01 13:37:50, Chandan wrote:
> > > Sorry, didn't realize that the existing equivalent IPC message is
> synchronous.
> > > Hence, added [Sync] attribute here.
> > >
> >
>
https://cs.chromium.org/chromium/src/media/gpu/ipc/common/media_messages.h?ty...
> > 
> > Is there a compelling reason why this must be synchronous?
> > 
> > Sync mojo calls are strongly discouraged:
> > https://www.chromium.org/developers/design-documents/mojo/synchronous-calls
> 
> Since the existing implementation is synchronous, I chose to keep it that way.
> As I see, JpegDecodeAccelerator interface expects Initialize(..) to be
> synchronous.
> Please refer:
>
https://cs.chromium.org/chromium/src/media/video/jpeg_decode_accelerator.h?ty...
> I am not too sure though since I am not aware of the history as why this was
> made synchronous.

I see. It's probably because it's copying the pattern used in VDA::Initialize()
which is also sync IIRIC.

You can keep it sync here. But please file a bug and add a TODO here to revisit
this code to make it async.

https://codereview.chromium.org/2905823002/diff/160001/media/gpu/mojo/jpeg_de...
media/gpu/mojo/jpeg_decoder.mojom:40: // |output_handle| with size limit
|output_buffer_size|.
On 2017/06/01 18:38:23, Chandan wrote:
> On 2017/06/01 16:59:01, xhwang wrote:
> > I know you are copying this from the IPC message. But now since we are
> touching
> > it, it's the chance to make it better.
> Sure.
> > Does it make sense to separate input and output instead of putting them in
one
> > opaque |info|?
> I think we can do this. Two separate structs in that case?

You don't need a struct if you only have 4 parameters...

> > Does it make sense to return the output in OnDecodeAck() call, or in a
> callback
> > of Decode()?
> We can do this too. A callback of Decode() would eliminate the need of a
> separate OnDecodeAck(). I will check further.

Thanks!

[Chandan, 2017-06-02 08:45:49]

The CQ bit was checked by c.padhi@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-02 08:46:01]

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-06-02 08:48:12]

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

[commit-bot: I haz the power, 2017-06-02 08:48:13]

Dry run: Try jobs failed on following builders:
  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_...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[Chandan, 2017-06-02 11:45:35]

The CQ bit was checked by c.padhi@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-02 11:45:46]

Dry run: CQ is trying da patch.

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

[Chandan, 2017-06-02 11:46:23]

Description was changed from

==========
Add Mojo interfaces for GpuJpegDecodeAccelerator and
GpuJpegDecodeAcceleratorHost

This CL is the first step towards mojification of GpuJpegDecodeAccelerator(GJDA)
and GpuJpegDecodeAcceleratorHost(GJDAH).

1. Defines media::BitstreamBuffer and AcceleratedJpegDecoderMsg_Decode_Params
for mojo.
2. GJDA and GJDAH implement these interfaces. Currently, these implementations
are empty.
3. Removes base::NonThreadSafe from GJDA and GJDAH.

BUG=699255
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

==========
Add Mojo interfaces for GpuJpegDecodeAccelerator and
GpuJpegDecodeAcceleratorHost

This CL is the first step towards mojification of GpuJpegDecodeAccelerator(GJDA)
and GpuJpegDecodeAcceleratorHost(GJDAH).

1. Defines media::BitstreamBuffer and AcceleratedJpegDecoderMsg_Decode_Params
for mojo.
2. GJDA and GJDAH implement these interfaces. Currently, these implementations
are empty.

BUG=699255
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
==========

[commit-bot: I haz the power, 2017-06-02 12:31:32]

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

[commit-bot: I haz the power, 2017-06-02 12:31:34]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Chandan, 2017-06-02 12:39:08]

Patchset #9 (id:200001) has been deleted

[Chandan, 2017-06-02 12:41:50]

The CQ bit was checked by c.padhi@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-06-02 12:42:03]

Dry run: CQ is trying da patch.

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

[Chandan, 2017-06-02 14:08:44]

PTAL @ps9. Thank you.

[commit-bot: I haz the power, 2017-06-02 15:12:03]

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

[commit-bot: I haz the power, 2017-06-02 15:12:05]

Dry run: This issue passed the CQ dry run.

[xhwang, 2017-06-02 16:40:16]

Thanks! This looks much cleaner!

I agree with chfremer@ that this CL is a bit too small. It'll be great if you
can show people how you are gonna register the service and connect to it (e.g.
in another prototype CL). But since you already have approval from other
reviewers, let's land this and I am sure you'll send a follow up CL soon :)

lgtm

[Chandan, 2017-06-02 17:38:47]

On 2017/06/02 16:40:16, xhwang wrote:
> Thanks! This looks much cleaner!
> 
> I agree with chfremer@ that this CL is a bit too small. It'll be great if you
> can show people how you are gonna register the service and connect to it (e.g.
> in another prototype CL). But since you already have approval from other
> reviewers, let's land this and I am sure you'll send a follow up CL soon :)
> 
> lgtm

Sure I will.
Thank you for the review.

[Chandan, 2017-06-02 17:39:07]

The CQ bit was checked by c.padhi@samsung.com

[Chandan, 2017-06-02 17:39:08]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
chfremer@chromium.org
Link to the patchset: https://codereview.chromium.org/2905823002/#ps220001
(title: "rebase")

[commit-bot: I haz the power, 2017-06-02 17:39:30]

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-06-02 17:45:12]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1496425147545060,
"parent_rev": "d34708521f2fac67995c25a20b4667a838b87f18", "commit_rev":
"eed40bc79df20716a280b62fb3d1f1f0a40df0a1"}

[commit-bot: I haz the power, 2017-06-02 17:45:20]

Description was changed from

==========
Add Mojo interfaces for GpuJpegDecodeAccelerator and
GpuJpegDecodeAcceleratorHost

This CL is the first step towards mojification of GpuJpegDecodeAccelerator(GJDA)
and GpuJpegDecodeAcceleratorHost(GJDAH).

1. Defines media::BitstreamBuffer and AcceleratedJpegDecoderMsg_Decode_Params
for mojo.
2. GJDA and GJDAH implement these interfaces. Currently, these implementations
are empty.

BUG=699255
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

==========
Add Mojo interfaces for GpuJpegDecodeAccelerator and
GpuJpegDecodeAcceleratorHost

This CL is the first step towards mojification of GpuJpegDecodeAccelerator(GJDA)
and GpuJpegDecodeAcceleratorHost(GJDAH).

1. Defines media::BitstreamBuffer and AcceleratedJpegDecoderMsg_Decode_Params
for mojo.
2. GJDA and GJDAH implement these interfaces. Currently, these implementations
are empty.

BUG=699255
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/2905823002
Cr-Commit-Position: refs/heads/master@{#476712}
Committed:
https://chromium.googlesource.com/chromium/src/+/eed40bc79df20716a280b62fb3d1...
==========

[commit-bot: I haz the power, 2017-06-02 17:45:23]

Committed patchset #9 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/eed40bc79df20716a280b62fb3d1...