Finish plumbing MojoVideoDecoder.

media/mojo/services/mojo_video_decoder.cc

 // Copyright 2016 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.
 
 #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__;
 }
 
 MojoVideoDecoder::~MojoVideoDecoder() {
   DVLOG(1) << __FUNCTION__;
 }
 
 std::string MojoVideoDecoder::GetDisplayName() const {
   return "MojoVideoDecoder";
 }
 
 void MojoVideoDecoder::Initialize(const VideoDecoderConfig& config,
                                   bool low_delay,
                                   CdmContext* cdm_context,
                                   const InitCB& init_cb,
                                   const OutputCB& output_cb) {
   DVLOG(1) << __FUNCTION__;
-  task_runner_ = base::ThreadTaskRunnerHandle::Get();
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  DCHECK(!cdm_context);
 
-  NOTIMPLEMENTED();
+  if (!remote_decoder_bound_)
+    BindRemoteDecoder();
 
-  // Pretend to be able to decode anything.
-  task_runner_->PostTask(FROM_HERE, base::Bind(init_cb, true));
+  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.

+}
+
+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());
 
-  // Actually we can't decode anything.
-  task_runner_->PostTask(FROM_HERE,
-                         base::Bind(decode_cb, DecodeStatus::DECODE_ERROR));
+  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)));
 }
 
-void MojoVideoDecoder::Reset(const base::Closure& closure) {
-  DVLOG(2) << __FUNCTION__;
-  NOTIMPLEMENTED();
+void MojoVideoDecoder::OnVideoFrameDecoded(interfaces::VideoFramePtr frame) {
+  DVLOG(1) << __FUNCTION__;
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  output_cb_.Run(frame.To<scoped_refptr<VideoFrame>>());
+}
+
+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