Add Mojo interfaces for GpuJpegDecodeAccelerator and GpuJpegDecodeAcceleratorHost

media/gpu/mojo/jpeg_decoder.mojom

Index: media/gpu/mojo/jpeg_decoder.mojom
diff --git a/media/gpu/mojo/jpeg_decoder.mojom b/media/gpu/mojo/jpeg_decoder.mojom
new file mode 100644
index 0000000000000000000000000000000000000000..ec80ea61eafda7e086c861f6adbefbd6d0e4084d
--- /dev/null
+++ b/media/gpu/mojo/jpeg_decoder.mojom
@@ -0,0 +1,37 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+module media.mojom;
+
+import "media/capture/mojo/video_capture_types.mojom";
+
+enum Error {
+  NO_ERRORS,
+  INVALID_ARGUMENT,
+  UNREADABLE_INPUT,
+  PARSE_JPEG_FAILED,
+  UNSUPPORTED_JPEG,
+  PLATFORM_FAILURE,
+};

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

I would move struct JpegDecodeInfo here, what'd be
the rationale of having it elsewhere if it's only used 
in this interface?

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

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.

[Chandan, 2017/05/29 13:02:33]

Done.

+
+interface GpuJpegDecodeAcceleratorClient {
+  // Report decode status.
+  OnDecodeAck(int32 bitstream_buffer_id, Error error);
+};
+
+interface GpuJpegDecodeAccelerator {
+  // Create and initialize a hardware jpeg decoder.
+  // Created decoder should be freed with Destroy() when no longer needed.
+  CreateJpegDecoder(GpuJpegDecodeAcceleratorClient client) => (bool success);

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

Why not:
  CreateJpegDecoder() =>
    (GpuJpegDecodeAcceleratorClient client)
and return a null GpuJpegDecodeAcceleratorClientPtr if 
somethings goes wrong?

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

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().

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

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

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

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 13:02:33]

Done.

+
+  // Decode one JPEG image from shared memory |input_buffer_handle| with size
+  // |input_buffer_size|. The input buffer is associated with |input_buffer_id|
+  // and the size of JPEG image is |coded_size|. Decoded I420 frame data will
+  // be put onto shared memory associated with |output_video_frame_handle|
+  // with size limit |output_buffer_size|.
+  Decode(media.mojom.JpegDecodeInfo info);

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

Same idea here:

  Decode() =>
    (JpegDecodeInfo info)

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

[Chandan, 2017/05/29 13:02:33]

Deleted media.mojom.

+
+  // Destroy request to the decoder.
+  Destroy();

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

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]

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:13]

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...

[Chandan, 2017/05/29 13:02:33]

Done.

+};