MojoVideoDecoder: Plumb metadata methods.

media/mojo/clients/mojo_video_decoder.cc

Index: media/mojo/clients/mojo_video_decoder.cc
diff --git a/media/mojo/clients/mojo_video_decoder.cc b/media/mojo/clients/mojo_video_decoder.cc
index ee293e6471ae67dcc86671e9a25b07f4e21dfe30..1267cc4a3c1158a29b89332457525eb35bd5b5e2 100644
--- a/media/mojo/clients/mojo_video_decoder.cc
+++ b/media/mojo/clients/mojo_video_decoder.cc
@@ -32,9 +32,11 @@ MojoVideoDecoder::MojoVideoDecoder(
 
 MojoVideoDecoder::~MojoVideoDecoder() {
   DVLOG(1) << __FUNCTION__;
+  Stop();
 }
 
 std::string MojoVideoDecoder::GetDisplayName() const {
+  // TODO(sandersd): Build the name including information from the remote end.
   return "MojoVideoDecoder";
 }
 
@@ -55,6 +57,7 @@ void MojoVideoDecoder::Initialize(const VideoDecoderConfig& config,
     return;
   }
 
+  initialized_ = false;
   init_cb_ = init_cb;
   output_cb_ = output_cb;
   remote_decoder_->Initialize(
@@ -62,17 +65,21 @@ void MojoVideoDecoder::Initialize(const VideoDecoderConfig& config,
       base::Bind(&MojoVideoDecoder::OnInitializeDone, base::Unretained(this)));
 }
 
-// TODO(sandersd): Remove this indirection once a working decoder has been
-// brought up.
-void MojoVideoDecoder::OnInitializeDone(bool status) {
+void MojoVideoDecoder::OnInitializeDone(bool status,
+                                        bool needs_bitstream_conversion,
+                                        int32_t max_decode_requests) {
   DVLOG(1) << __FUNCTION__;
   DCHECK(task_runner_->BelongsToCurrentThread());
+  initialized_ = true;
+  needs_bitstream_conversion_ = needs_bitstream_conversion;
+  max_decode_requests_ = max_decode_requests;
+  can_read_without_stalling_ = true;
   base::ResetAndReturn(&init_cb_).Run(status);
 }
 
 void MojoVideoDecoder::Decode(const scoped_refptr<DecoderBuffer>& buffer,
                               const DecodeCB& decode_cb) {
-  DVLOG(1) << __FUNCTION__;
+  DVLOG(2) << __FUNCTION__;
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   if (has_connection_error_) {
@@ -89,23 +96,30 @@ void MojoVideoDecoder::Decode(const scoped_refptr<DecoderBuffer>& buffer,
     return;
   }
 
-  // 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)));
+  uint64_t decode_id = decode_counter_++;
+  pending_decodes_[decode_id] = decode_cb;

[slan, 2016/10/18 20:46:25]

What if we bind |decode_cb| directly to OnDecodeDone? This would get rid of the
need for pending_decoder_ and decode_counter_:

remote_decoder_->Decode(
    std::move(mojo_buffer),
    base::Bind(&MojoVideoDecoder::OnDecodeDone,
               base::Bind(&MojoVideoDecoder::OnDecodeDone, 
                          base::Unretained(this), decode_cb));

...

void MojoVideoDecoder::OnDecodeDone(
    DecodeCB decode_cb,
    DecodeStatus status,
    bool can_read_without_stalling) {
...

[slan, 2016/10/18 20:49:41]

Sorry, this is a typo, you know what I mean :)

[sandersd (OOO until July 31), 2016/10/18 21:09:56]

The problem this solves is having a list of outstanding decode callbacks to call
in Stop(). It it could be just a std::set of callbacks, that would also work,
but I didn't want to go there with a copyable type.

[slan, 2016/10/18 22:06:44]

Right, I overlooked that bit. Your method SGTM. 

+  remote_decoder_->Decode(std::move(mojo_buffer),
+                          base::Bind(&MojoVideoDecoder::OnDecodeDone,
+                                     base::Unretained(this), decode_id));

[dcheng, 2016/10/18 20:39:05]

Can we just bind the callback directly instead of retaining this information in
a map?

[dcheng, 2016/10/19 02:42:39]

OK, thanks for the explanation. +rockot, do you know of a lot of other instances
of this in the codebase? It seems a bit awkward to have to do this, but if
there's no other way...

[Ken Rockot(use gerrit already), 2016/10/19 05:50:34]

It's not a pattern that has come up often. This approach seems awkward but
reasonable.

Another way it's been solved is to have a scoped callback type which runs with
some default arguments if destroyed before being explicitly run. That might be
seen as a bit too magical though.

In this case I think maybe it's worth considering whether MojoVideoDecoder
really needs to take responsibility for ensuring all the callbacks are called.
Could it just expose some other way of subscribing to a generic "The
MojoVideoDecoder is going away now" event, and clients who care can care?

 }
 
 void MojoVideoDecoder::OnVideoFrameDecoded(mojom::VideoFramePtr frame) {
-  DVLOG(1) << __FUNCTION__;
+  DVLOG(2) << __FUNCTION__;
   DCHECK(task_runner_->BelongsToCurrentThread());
   output_cb_.Run(frame.To<scoped_refptr<VideoFrame>>());
 }
 
-void MojoVideoDecoder::OnDecodeDone(DecodeStatus status) {
-  DVLOG(1) << __FUNCTION__;
+void MojoVideoDecoder::OnDecodeDone(uint64_t decode_id,
+                                    DecodeStatus status,
+                                    bool can_read_without_stalling) {
+  DVLOG(2) << __FUNCTION__;
   DCHECK(task_runner_->BelongsToCurrentThread());
-  base::ResetAndReturn(&decode_cb_).Run(status);
+  DCHECK(pending_decodes_.count(decode_id));
+  DCHECK(pending_decodes_.size() > 1 || can_read_without_stalling);

[dcheng, 2016/10/18 20:39:05]

Should these actually be DCHECKs? Presumably, the actually decoder lives in a
less privileged process.

[sandersd (OOO until July 31), 2016/10/18 21:09:56]

In fact it lives in a more privileged process (the GPU process). But the point
is well taken, this API could conceivably be to a lower-privileged process, and
that would be a logical goal in the future.

I'm interpreting |can_read_without_stalling| as a developer mistake that we want
to catch as soon as possible, but the danger of getting it wrong is not a
security one. In actual practice, I expect most decoders to always have
|can_read_without_stalling| = true.

In both cases it would be safe to call Stop() instead, and that matches the
existing behavior of GpuVideoDecoder. Would you prefer I did that instead?

[dcheng, 2016/10/19 02:42:39]

Hmm, can you link to the GpuVideoDecoder you're referring to?

[sandersd (OOO until July 31), 2016/10/19 17:51:24]

https://cs.chromium.org/chromium/src/media/filters/gpu_video_decoder.cc?l=717

+  can_read_without_stalling_ = can_read_without_stalling;
+  DecodeCB decode_cb = pending_decodes_[decode_id];
+  pending_decodes_.erase(decode_id);
+  decode_cb.Run(status);
 }
 
 void MojoVideoDecoder::Reset(const base::Closure& reset_cb) {
@@ -125,26 +139,30 @@ void MojoVideoDecoder::Reset(const base::Closure& reset_cb) {
 void MojoVideoDecoder::OnResetDone() {
   DVLOG(1) << __FUNCTION__;
   DCHECK(task_runner_->BelongsToCurrentThread());
+  can_read_without_stalling_ = true;
   base::ResetAndReturn(&reset_cb_).Run();
 }
 
 bool MojoVideoDecoder::NeedsBitstreamConversion() const {
-  DVLOG(1) << __FUNCTION__;
-  return false;
+  DVLOG(3) << __FUNCTION__;
+  DCHECK(initialized_);
+  return needs_bitstream_conversion_;
 }
 
 bool MojoVideoDecoder::CanReadWithoutStalling() const {
-  DVLOG(1) << __FUNCTION__;
-  return true;
+  DVLOG(3) << __FUNCTION__;
+  DCHECK(initialized_);
+  return can_read_without_stalling_;
 }
 
 int MojoVideoDecoder::GetMaxDecodeRequests() const {
-  DVLOG(1) << __FUNCTION__;
-  return 1;
+  DVLOG(3) << __FUNCTION__;
+  DCHECK(initialized_);
+  return max_decode_requests_;
 }
 
 void MojoVideoDecoder::BindRemoteDecoder() {
-  DVLOG(1) << __FUNCTION__;
+  DVLOG(3) << __FUNCTION__;
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(!remote_decoder_bound_);
 
@@ -152,8 +170,9 @@ void MojoVideoDecoder::BindRemoteDecoder() {
   remote_decoder_bound_ = true;
 
   remote_decoder_.set_connection_error_handler(
-      base::Bind(&MojoVideoDecoder::OnConnectionError, base::Unretained(this)));
+      base::Bind(&MojoVideoDecoder::Stop, base::Unretained(this)));
 
+  // TODO(sandersd): Does this need its own error handler?
   mojom::VideoDecoderClientAssociatedPtrInfo client_ptr_info;
   client_binding_.Bind(&client_ptr_info, remote_decoder_.associated_group());
 
@@ -166,19 +185,19 @@ void MojoVideoDecoder::BindRemoteDecoder() {
                              std::move(remote_consumer_handle));
 }
 
-void MojoVideoDecoder::OnConnectionError() {
-  DVLOG(1) << __FUNCTION__;
+void MojoVideoDecoder::Stop() {
+  DVLOG(2) << __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);
+
+  for (const auto& pending_decode : pending_decodes_)
+    pending_decode.second.Run(DecodeStatus::DECODE_ERROR);
+  pending_decodes_.clear();
+
   if (!reset_cb_.is_null())
     base::ResetAndReturn(&reset_cb_).Run();
 }