Handle HW decode failures in RTCVideoDecoder

content/renderer/media/rtc_video_decoder.cc

Index: content/renderer/media/rtc_video_decoder.cc
diff --git a/content/renderer/media/rtc_video_decoder.cc b/content/renderer/media/rtc_video_decoder.cc
index 069f418225ddb88e775ee34db6041b3dd122fd5e..d5b36809b2eda5d3d82f7f66b912dd88df5a95ef 100644
--- a/content/renderer/media/rtc_video_decoder.cc
+++ b/content/renderer/media/rtc_video_decoder.cc
@@ -160,6 +160,11 @@ int32_t RTCVideoDecoder::Decode(
 
   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.

   }
 
@@ -607,8 +612,11 @@ void RTCVideoDecoder::MovePendingBuffersToDecodeBuffers() {
 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
@@ -680,9 +688,11 @@ void RTCVideoDecoder::CreateVDA(media::VideoCodecProfile profile,
     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() {
@@ -705,7 +715,14 @@ void RTCVideoDecoder::DestroyVDA() {
   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;
 }