Add Mojo interfaces for GpuJpegDecodeAccelerator and GpuJpegDecodeAcceleratorHost

media/gpu/mojo/jpeg_decoder.mojom

+// 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/mojo/interfaces/media_types.mojom";
+import "ui/gfx/geometry/mojo/geometry.mojom";
+
+enum Error {
+  NO_ERRORS,
+  INVALID_ARGUMENT,
+  UNREADABLE_INPUT,
+  PARSE_JPEG_FAILED,
+  UNSUPPORTED_JPEG,
+  PLATFORM_FAILURE,
+};
+
+struct JpegDecodeInfo {

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

This struct is not documented. You probably want to move most of the comments
above Decode() here.

[Chandan, 2017/06/01 18:38:22]

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

+  BitstreamBuffer input_buffer;
+  gfx.mojom.Size coded_size;
+  handle<shared_buffer> output_handle;
+  uint32 output_buffer_size;
+};
+
+interface GpuJpegDecodeAcceleratorClient {
+  // Report decode status.
+  OnDecodeAck(int32 bitstream_buffer_id, Error error);

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

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?

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

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.

+};
+
+// Initialize() must be called before using Decode().
+interface GpuJpegDecodeAccelerator {
+  [Sync]

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

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]

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

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

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.

[xhwang, 2017/06/01 18:49:18]

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.

+  Initialize(GpuJpegDecodeAcceleratorClient client) => (bool success);
+
+  // Decode one JPEG image from shared memory |input_buffer.memory_handle| with
+  // size |input_buffer.size|. The input buffer is associated with
+  // |input_buffer.id|and the and the size of JPEG image is |coded_size|.
+  // Decoded I420 frame data will be put onto shared memory associated with
+  // |output_handle| with size limit |output_buffer_size|.

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

For |output_buffer_size|, if it's a limit, it should be called *limit...

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

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

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

Sure.
I think we can do this. Two separate structs in that case?
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:49:18]

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

+  Decode(JpegDecodeInfo info);
+
+  // TODO(c.padhi): This method might not be required, see
+  // http://crbug.com/699255.
+  Uninitialize();
+};