Handle HW decode failures in RTCVideoDecoder

content/renderer/media/rtc_video_decoder.cc

 // Copyright 2013 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 "content/renderer/media/rtc_video_decoder.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/logging.h"
@@ skipping 142 matching lines @@
 
   base::AutoLock auto_lock(lock_);
 
   if (state_ == UNINITIALIZED || !decode_complete_callback_) {
     LOG(ERROR) << "The decoder has not initialized.";
     return WEBRTC_VIDEO_CODEC_UNINITIALIZED;
   }
 
   if (state_ == DECODE_ERROR) {
     LOG(ERROR) << "Decoding error occurred.";
+    // Try restarting the session. It is OK to drop the frames until a keyframe
+    // is received.
+    factories_->GetTaskRunner()->PostTask(
+        FROM_HERE, base::Bind(&RTCVideoDecoder::ResetInternal,

[mcasas, 2016/03/23 18:29:56]

Shouldn't we make sure that the
|state_ = RESETTING| either 
a) here
b) ResetInternal()

so that when several |inputImage|s come back
to back, we don't post several ResetInternal()s ?

[emircan, 2016/03/25 02:33:31]

Agreed. I will set it here(a) since all |state_| modifications happen under a
lock.

+                              weak_factory_.GetWeakPtr()));
     return WEBRTC_VIDEO_CODEC_ERROR;

[mcasas, 2016/03/23 18:29:56]

Following offline conversation, please add
some kind of counter of the times we tried to
restart the underlying VDA, and if > say, 5,
then stick to replying 
|WEBRTC_VIDEO_CODEC_FALLBACK_SOFTWARE|.

[emircan, 2016/03/25 02:33:31]

Done.

   }
 
   if (missingFrames || !inputImage._completeFrame) {
     DLOG(ERROR) << "Missing or incomplete frames.";
     // Unlike the SW decoder in libvpx, hw decoder cannot handle broken frames.
     // Return an error to request a key frame.
     return WEBRTC_VIDEO_CODEC_ERROR;
   }
 
   // Most platforms' VDA implementations support mid-stream resolution change
@@ skipping 425 matching lines @@
     if (!shm_buffer)
       return;
     SaveToDecodeBuffers_Locked(input_image, std::move(shm_buffer), buffer_data);
     delete[] input_image._buffer;
     pending_buffers_.pop_front();
   }
 }
 
 void RTCVideoDecoder::ResetInternal() {
   DCheckGpuVideoAcceleratorFactoriesTaskRunnerIsCurrent();
   DVLOG(2) << "ResetInternal";

[mcasas, 2016/03/23 18:29:56]

nit: Move this to before the DCHECK
and use DVLOG(2) << __FUNCTION__;

[emircan, 2016/03/25 02:33:31]

Done.

+
   if (vda_)
     vda_->Reset();
+  else

[pbos, 2016/03/22 21:20:43]

{}s for if-elses

[mcasas, 2016/03/23 18:29:56]

Not for both bodies being one-liners.

https://google.github.io/styleguide/cppguide.html#Conditionals

+    CreateVDA(vda_codec_profile_, nullptr);
 }
 
 // static
 void RTCVideoDecoder::ReleaseMailbox(
     base::WeakPtr<RTCVideoDecoder> decoder,
     media::GpuVideoAcceleratorFactories* factories,
     int64_t picture_buffer_id,
     uint32_t texture_id,
     const gpu::SyncToken& release_sync_token) {
   DCHECK(factories->GetTaskRunner()->BelongsToCurrentThread());
@@ skipping 51 matching lines @@
   DCheckGpuVideoAcceleratorFactoriesTaskRunnerIsCurrent();
 
   if (!IsProfileSupported(profile)) {
     DVLOG(1) << "Unsupported profile " << profile;
   } else {
     vda_ = factories_->CreateVideoDecodeAccelerator();
 
     media::VideoDecodeAccelerator::Config config(profile);
     if (vda_ && !vda_->Initialize(config, this))
       vda_.release()->Destroy();
+    vda_codec_profile_ = profile;
   }
 
-  waiter->Signal();
+  if (waiter)
+    waiter->Signal();
 }
 
 void RTCVideoDecoder::DestroyTextures() {
   DCheckGpuVideoAcceleratorFactoriesTaskRunnerIsCurrent();
 
   // Not destroying PictureBuffers in |picture_buffers_at_display_| yet, since
   // their textures may still be in use by the user of this RTCVideoDecoder.
   for (const auto& picture_buffer_at_display : picture_buffers_at_display_)
     assigned_picture_buffers_.erase(picture_buffer_at_display.first);
 
   for (const auto& assigned_picture_buffer : assigned_picture_buffers_)
     factories_->DeleteTexture(assigned_picture_buffer.second.texture_id());
 
   assigned_picture_buffers_.clear();
 }
 
 void RTCVideoDecoder::DestroyVDA() {
   DVLOG(2) << "DestroyVDA";
   DCheckGpuVideoAcceleratorFactoriesTaskRunnerIsCurrent();
   if (vda_)
     vda_.release()->Destroy();
   DestroyTextures();
+
   base::AutoLock auto_lock(lock_);
+
+  // Put the buffers back in case we restart the decoder.
+  for (const auto& buffer : bitstream_buffers_in_decoder_)
+    PutSHM_Locked(scoped_ptr<base::SharedMemory>(buffer.second));

[mcasas, 2016/03/23 18:29:57]

nit: totally not your problem and prob
beyond this CL scope, but this method
should be called 
PutSHM_ThreadSafe() and not _Locked
since the latter reads to me as it is
going to acquire a lock, potentially the
same as in l.719 and then bam!

[emircan, 2016/03/25 02:33:31]

I agree the name is confusing. They specify "lock_.AssertAcquired();" in the
functions at least, so that we can tell Locked stands for "call this only when
you have it locked". This class can defintiely use some refactoring and I will
revisit it for sure.

+  bitstream_buffers_in_decoder_.clear();
+
   state_ = UNINITIALIZED;
 }
 
 scoped_ptr<base::SharedMemory> RTCVideoDecoder::GetSHM_Locked(size_t min_size) {
   // Reuse a SHM if possible.
   if (!available_shm_segments_.empty() &&
       available_shm_segments_.back()->mapped_size() >= min_size) {
     scoped_ptr<base::SharedMemory> buffer(available_shm_segments_.back());
     available_shm_segments_.pop_back();
     return buffer;
@@ skipping 88 matching lines @@
 }
 
 void RTCVideoDecoder::ClearPendingBuffers() {
   // Delete WebRTC input buffers.
   for (const auto& pending_buffer : pending_buffers_)
     delete[] pending_buffer.first._buffer;
   pending_buffers_.clear();
 }
 
 }  // namespace content