Finish plumbing MojoVideoDecoder.

media/mojo/services/mojo_video_decoder.cc

Index: media/mojo/services/mojo_video_decoder.cc
diff --git a/media/mojo/services/mojo_video_decoder.cc b/media/mojo/services/mojo_video_decoder.cc
index ab0c1228772e362846d1eab50efc9df071cf5b2c..f684022d66a37a53979aa91a5b125adfaa751ce1 100644
--- a/media/mojo/services/mojo_video_decoder.cc
+++ b/media/mojo/services/mojo_video_decoder.cc
@@ -5,14 +5,25 @@
 #include "media/mojo/services/mojo_video_decoder.h"
 
 #include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/callback_helpers.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/single_thread_task_runner.h"
-#include "base/thread_task_runner_handle.h"
+#include "media/base/decoder_buffer.h"
+#include "media/base/video_frame.h"
+#include "media/mojo/common/media_type_converters.h"
 
 namespace media {
 
-MojoVideoDecoder::MojoVideoDecoder() {
+MojoVideoDecoder::MojoVideoDecoder(
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+    GpuVideoAcceleratorFactories* gpu_factories,
+    interfaces::VideoDecoderPtr remote_decoder)
+    : task_runner_(task_runner),
+      gpu_factories_(gpu_factories),
+      remote_decoder_info_(remote_decoder.PassInterface()),
+      binding_(this) {
   DVLOG(1) << __FUNCTION__;
 }
 
@@ -30,45 +41,153 @@ void MojoVideoDecoder::Initialize(const VideoDecoderConfig& config,
                                   const InitCB& init_cb,
                                   const OutputCB& output_cb) {
   DVLOG(1) << __FUNCTION__;
-  task_runner_ = base::ThreadTaskRunnerHandle::Get();
-
-  NOTIMPLEMENTED();
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  DCHECK(!cdm_context);
+
+  if (!remote_decoder_bound_)
+    BindRemoteDecoder();
+
+  if (has_connection_error_) {
+    task_runner_->PostTask(FROM_HERE, base::Bind(init_cb, false));
+    return;
+  }
+
+  init_cb_ = init_cb;
+  output_cb_ = output_cb;
+  remote_decoder_->Initialize(
+      interfaces::VideoDecoderConfig::From(config), low_delay,
+      base::Bind(&MojoVideoDecoder::OnInitializeDone, base::Unretained(this)));

[dcheng, 2016/05/11 07:37:45]

Why can't we just pass init_cb directly here? Similar question in other
callbacks, it seems weird to trampoline through a function that doesn't do very
much.

[xhwang, 2016/05/11 16:13:57]

Last time I checked, you can't implicitly convert a base::Callback to
mojo::Callback and vice versa.

[dcheng, 2016/05/11 17:02:50]

How is using base::Bind() to create a base::Callback different than just passing
the base::Callback directly here? Maybe I'm missing something.

[xhwang, 2016/05/11 17:14:58]

oops, this is actually different than what I was thinking. I was talking about
the other side, where I vaguely remember that we can't simply pass a
mojo::Callback where base::Callback is expected. So I had to use base::Bind to
do the conversion:

https://code.google.com/p/chromium/codesearch#chromium/src/media/mojo/service...

This code is actually different. It seems we are already doing base::Callback to
mojo::Callback conversion. To be honest I don't know how that works :)

[sandersd (OOO until July 31), 2016/05/11 18:20:18]

The truth is that it's an artifact of changes to the interface during
development, it's not doing anything critical anymore. However, I would rather
keep the debugging features it adds while I'm actively developing.
There is a templated conversion constructor for anything with a Run() method
(https://code.google.com/p/chromium/codesearch#chromium/src/mojo/public/cpp/bi...).

[dcheng, 2016/05/11 22:15:06]

When will we actually remove them? Code like this has a tendency to stick
around, and indirecting so many times through base::Bind just obfuscates the
control flow.

[sandersd (OOO until July 31), 2016/05/11 23:00:50]

TODO added.

+}
 
-  // Pretend to be able to decode anything.
-  task_runner_->PostTask(FROM_HERE, base::Bind(init_cb, true));
+void MojoVideoDecoder::OnInitializeDone(bool status) {
+  DVLOG(1) << __FUNCTION__;
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  base::ResetAndReturn(&init_cb_).Run(status);
 }
 
 void MojoVideoDecoder::Decode(const scoped_refptr<DecoderBuffer>& buffer,
                               const DecodeCB& decode_cb) {
-  DVLOG(3) << __FUNCTION__;
-  NOTIMPLEMENTED();
+  DVLOG(1) << __FUNCTION__;
+  DCHECK(task_runner_->BelongsToCurrentThread());
+
+  if (has_connection_error_) {
+    task_runner_->PostTask(FROM_HERE,
+                           base::Bind(decode_cb, DecodeStatus::DECODE_ERROR));
+    return;
+  }
+
+  interfaces::DecoderBufferPtr mojo_buffer =
+      interfaces::DecoderBuffer::From(buffer);
+
+  // TODO(sandersd): Destruct cleanly on error.
+  if (!buffer->end_of_stream()) {
+    uint32_t data_size = base::checked_cast<uint32_t>(buffer->data_size());

[dcheng, 2016/05/11 07:37:45]

This is... in the renderer process? It's a bit hard to tell, maybe there's some
sort of naming convention we can use? For example, we currently have a FooHost /
Foo convention for things like RenderFrameHost / RenderFrame. It would be nice
to keep that with Mojo.

[xhwang, 2016/05/11 16:13:57]

I don't see examples of keeping such convention when using Mojo :) Also in the
IPC world, the Host is always in the browser process. But in the mojo world, the
service can be hosted in any process (e.g. GPU, Utility), so Host would actually
be confusing.

[dcheng, 2016/05/11 17:02:50]

Well, we should figure one out. I'll start a thread on chromium-mojo. As it is,
it's pretty confusing.
That's already not true in IPC today (GPU, plugin) so I don't think this would
be more confusing, especially with more comments in the mojom to indicate where
things live (if possible)

[sandersd (OOO until July 31), 2016/05/12 18:10:36]

Since there isn't guidance from that thread yet, perhaps there is a lightweight
method that could work? Comment at the top of the file/header perhaps?

For this CL, mojo_video_decoder runs in the renderer (unprivileged), and
mojo_video_decoder_service runs in the GPU process (privileged). Currently all
media Mojo code is using this same division, Foo -> renderer, FooService -> not
renderer.

[sandersd (OOO until July 31), 2016/05/17 23:05:00]

I've added basic comments to the class headers. Let me know if there is more
information you want added.

+    DCHECK_GT(data_size, 0u);
+    uint32_t bytes_written = data_size;
+    CHECK_EQ(WriteDataRaw(decoder_buffer_pipe_.get(), buffer->data(),
+                          &bytes_written, MOJO_READ_DATA_FLAG_ALL_OR_NONE),
+             MOJO_RESULT_OK);
+    CHECK_EQ(bytes_written, data_size);
+  }
+
+  // TODO(sandersd): Support more than one decode at a time.
+  decode_cb_ = decode_cb;
+  remote_decoder_->Decode(
+      std::move(mojo_buffer),
+      base::Bind(&MojoVideoDecoder::OnDecodeDone, base::Unretained(this)));
+}
 
-  // Actually we can't decode anything.
-  task_runner_->PostTask(FROM_HERE,
-                         base::Bind(decode_cb, DecodeStatus::DECODE_ERROR));
+void MojoVideoDecoder::OnVideoFrameDecoded(interfaces::VideoFramePtr frame) {
+  DVLOG(1) << __FUNCTION__;
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  output_cb_.Run(frame.To<scoped_refptr<VideoFrame>>());
 }
 
-void MojoVideoDecoder::Reset(const base::Closure& closure) {
-  DVLOG(2) << __FUNCTION__;
-  NOTIMPLEMENTED();
+void MojoVideoDecoder::OnDecodeDone(interfaces::DecodeStatus status) {
+  DVLOG(1) << __FUNCTION__;
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  base::ResetAndReturn(&decode_cb_).Run(static_cast<DecodeStatus>(status));
+}
+
+void MojoVideoDecoder::Reset(const base::Closure& reset_cb) {
+  DVLOG(1) << __FUNCTION__;
+  DCHECK(task_runner_->BelongsToCurrentThread());
+
+  if (has_connection_error_) {
+    task_runner_->PostTask(FROM_HERE, reset_cb);
+    return;
+  }
+
+  reset_cb_ = reset_cb;
+  remote_decoder_->Reset(
+      base::Bind(&MojoVideoDecoder::OnResetDone, base::Unretained(this)));
+}
+
+void MojoVideoDecoder::OnResetDone() {
+  DVLOG(1) << __FUNCTION__;
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  base::ResetAndReturn(&reset_cb_).Run();
 }
 
 bool MojoVideoDecoder::NeedsBitstreamConversion() const {
   DVLOG(1) << __FUNCTION__;
-  NOTIMPLEMENTED();
   return false;
 }
 
 bool MojoVideoDecoder::CanReadWithoutStalling() const {
   DVLOG(1) << __FUNCTION__;
-  NOTIMPLEMENTED();
   return true;
 }
 
 int MojoVideoDecoder::GetMaxDecodeRequests() const {
   DVLOG(1) << __FUNCTION__;
-  NOTIMPLEMENTED();
   return 1;
 }
 
+void MojoVideoDecoder::BindRemoteDecoder() {
+  DVLOG(1) << __FUNCTION__;
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  DCHECK(!remote_decoder_bound_);
+
+  remote_decoder_.Bind(std::move(remote_decoder_info_));
+  remote_decoder_bound_ = true;
+
+  if (remote_decoder_.encountered_error()) {
+    has_connection_error_ = true;
+    return;
+  }
+
+  remote_decoder_.set_connection_error_handler(
+      base::Bind(&MojoVideoDecoder::OnConnectionError, base::Unretained(this)));
+
+  // TODO(sandersd): Better buffer sizing.
+  MojoCreateDataPipeOptions options;
+  options.struct_size = sizeof(MojoCreateDataPipeOptions);

[dcheng, 2016/05/11 07:37:45]

Style guide recommends using sizeof(options).

[sandersd (OOO until July 31), 2016/05/11 18:20:18]

Done.

+  options.flags = MOJO_CREATE_DATA_PIPE_OPTIONS_FLAG_NONE;
+  options.element_num_bytes = 1;
+  options.capacity_num_bytes = 2 * 1024 * 1024;
+  mojo::DataPipe decoder_buffer_pipe(options);
+
+  decoder_buffer_pipe_ = std::move(decoder_buffer_pipe.producer_handle);
+  remote_decoder_->Construct(binding_.CreateInterfacePtrAndBind(),
+                             std::move(decoder_buffer_pipe.consumer_handle));
+}
+
+void MojoVideoDecoder::OnConnectionError() {
+  DVLOG(1) << __FUNCTION__;
+  DCHECK(task_runner_->BelongsToCurrentThread());
+
+  has_connection_error_ = true;
+
+  // TODO(sandersd): Write a wrapper class (like BindToCurrentLoop) that handles
+  // the lifetime of callbacks like this.
+  if (!init_cb_.is_null())
+    base::ResetAndReturn(&init_cb_).Run(false);
+  // TODO(sandersd): If there is a pending reset, should these be aborted?
+  if (!decode_cb_.is_null())
+    base::ResetAndReturn(&decode_cb_).Run(DecodeStatus::DECODE_ERROR);
+  if (!reset_cb_.is_null())
+    base::ResetAndReturn(&reset_cb_).Run();

[dcheng, 2016/05/11 07:37:45]

I think it'd be better if there was a separate callback for connection error
rather than signalling every callback. I'm not sure adding another callback
abstraction to //media is good.

[sandersd (OOO until July 31), 2016/05/11 18:20:18]

We're constrained to implement the media::VideoDecoder interface here, and I see
no reason to add such a callback to it. This code isn't that terrible, and it
would somewhat defeat the point if the rest of media code needed to understand
what a remote decoder was.

+}
+
 }  // namespace media