VAVDA: Fix locking.

content/common/gpu/media/vaapi_video_decode_accelerator.cc

 // Copyright (c) 2012 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 "base/bind.h"
 #include "base/command_line.h"
 #include "base/debug/trace_event.h"
 #include "base/logging.h"
 #include "base/stl_util.h"
 #include "base/string_util.h"
@@ skipping 111 matching lines @@
   DCHECK_EQ(message_loop_, MessageLoop::current());
 
   DVLOG(4) << "Mapping new input buffer id: " << bitstream_buffer.id()
            << " size: " << (int)bitstream_buffer.size();
 
   scoped_ptr<base::SharedMemory> shm(
       new base::SharedMemory(bitstream_buffer.handle(), true));
   RETURN_AND_NOTIFY_ON_FAILURE(shm->Map(bitstream_buffer.size()),
                               "Failed to map input buffer", UNREADABLE_INPUT,);
 
+  base::AutoLock auto_lock(lock_);

[Ami GONE FROM CHROMIUM, 2012/08/03 20:38:16]

This change isn't strictly necessary b/c the extra protection it affords is
around a new linked_ptr<InputBuffer> wrapping a new InputBuffer.

[piman, 2012/08/03 21:05:02]

This change is absolutely necessary (we found the issue because this wasn't
done).

If the AutoLock is below, then it will go out of scope before input_buffer. Then
when input_buffer goes out of scope, it will modify its linked list, racing with
modifications to the linked list in the other thread (e.g. GetInputBuffer, which
will assign a new linked_ptr that will also modify the linked list).

[Ami GONE FROM CHROMIUM, 2012/08/03 21:06:41]

Good point!

+
   // Set up a new input buffer and queue it for later.
   linked_ptr<InputBuffer> input_buffer(new InputBuffer());
   input_buffer->shm.reset(shm.release());
   input_buffer->id = bitstream_buffer.id();
   input_buffer->size = bitstream_buffer.size();
 
-  base::AutoLock auto_lock(lock_);
   input_buffers_.push(input_buffer);
   input_ready_.Signal();
 }
 
 void VaapiVideoDecodeAccelerator::InitialDecodeTask() {
   DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
 
   // Since multiple Decode()'s can be in flight at once, it's possible that a
   // Decode() that seemed like an initial one is actually later in the stream
   // and we're already kDecoding.  Let the normal DecodeTask take over in that
   // case.
   {
     base::AutoLock auto_lock(lock_);
     if (state_ != kInitialized && state_ != kIdle)
       return;
   }
 
   // Try to initialize or resume playback after reset.
   for (;;) {
     if (!GetInputBuffer())
       return;
-    DCHECK(curr_input_buffer_.get());
 
-    VaapiH264Decoder::DecResult res = decoder_.DecodeInitial(
-        curr_input_buffer_->id);
+    int32 id = 0;
+    {
+      base::AutoLock auto_lock(lock_);
+      DCHECK(curr_input_buffer_.get());
+      id = curr_input_buffer_->id;
+    }
+
+    VaapiH264Decoder::DecResult res = decoder_.DecodeInitial(id);
     switch (res) {
       case VaapiH264Decoder::kReadyToDecode:
         if (state_ == kInitialized) {
           base::AutoLock auto_lock(lock_);
           state_ = kPicturesRequested;
           int num_pics = decoder_.GetRequiredNumOfPictures();
           gfx::Size size(decoder_.pic_width(), decoder_.pic_height());
           DVLOG(1) << "Requesting " << num_pics << " pictures of size: "
                    << size.width() << "x" << size.height();
           message_loop_->PostTask(FROM_HERE, base::Bind(
@@ skipping 66 matching lines @@
           curr_input_buffer_->size);
       return true;
 
     default:
       // We got woken up due to being destroyed/reset, ignore any already
       // queued inputs.
       return false;
   }
 }
 
-void VaapiVideoDecodeAccelerator::ReturnCurrInputBuffer() {
+void VaapiVideoDecodeAccelerator::ReturnCurrInputBufferLocked() {
+  lock_.AssertAcquired();
   DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
+  DCHECK(curr_input_buffer_.get());
 
-  base::AutoLock auto_lock(lock_);
-  DCHECK(curr_input_buffer_.get());
   int32 id = curr_input_buffer_->id;
   curr_input_buffer_.reset();
   DVLOG(4) << "End of input buffer " << id;
   message_loop_->PostTask(FROM_HERE, base::Bind(
       &Client::NotifyEndOfBitstreamBuffer, client_, id));
 }
 
+void VaapiVideoDecodeAccelerator::ReturnCurrInputBuffer() {
+  base::AutoLock auto_lock(lock_);
+  ReturnCurrInputBufferLocked();
+}
+
 bool VaapiVideoDecodeAccelerator::GetOutputBuffers() {
   DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
 
   base::AutoLock auto_lock(lock_);
 
   while (output_buffers_.empty() &&
          (state_ == kDecoding || state_ == kFlushing)) {
     output_ready_.Wait();
   }
 
@@ skipping 13 matching lines @@
 
   // Main decode task.
   DVLOG(4) << "Decode task";
 
   // Try to decode what stream data is (still) in the decoder until we run out
   // of it.
   for (;;) {
     if (!GetInputBuffer())
       // Early exit requested.
       return;
-    DCHECK(curr_input_buffer_.get());
 
-    VaapiH264Decoder::DecResult res =
-        decoder_.DecodeOneFrame(curr_input_buffer_->id);
+    int32 id = 0;
+    {
+      base::AutoLock auto_lock(lock_);
+      DCHECK(curr_input_buffer_.get());
+      id = curr_input_buffer_->id;
+    }
+
+    VaapiH264Decoder::DecResult res = decoder_.DecodeOneFrame(id);
     switch (res) {
       case VaapiH264Decoder::kNeedMoreStreamData:
         ReturnCurrInputBuffer();
         break;
 
       case VaapiH264Decoder::kDecodedFrame:
         // May still have more stream data, continue decoding.
         break;
 
       case VaapiH264Decoder::kNoOutputAvailable:
@@ skipping 138 matching lines @@
 }
 
 void VaapiVideoDecodeAccelerator::ResetTask() {
   DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
 
   // All the decoding tasks from before the reset request from client are done
   // by now, as this task was scheduled after them and client is expected not
   // to call Decode() after Reset() and before NotifyResetDone.
   decoder_.Reset();
 
+  base::AutoLock auto_lock(lock_);
+
   // Return current input buffer, if present.
   if (curr_input_buffer_.get())
-    ReturnCurrInputBuffer();
+    ReturnCurrInputBufferLocked();
 
   // And let client know that we are done with reset.
   message_loop_->PostTask(FROM_HERE, base::Bind(
       &VaapiVideoDecodeAccelerator::FinishReset, weak_this_));
 }
 
 void VaapiVideoDecodeAccelerator::Reset() {
   DCHECK_EQ(message_loop_, MessageLoop::current());
   DVLOG(1) << "Got reset request";
 
@@ skipping 73 matching lines @@
                "Input id", input_id, "Picture id", output_id);
   DVLOG(4) << "Outputting picture, input id: " << input_id
            << " output id: " << output_id;
 
   // Forward the request to the main thread.
   DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
   message_loop_->PostTask(FROM_HERE, base::Bind(
       &VaapiVideoDecodeAccelerator::SyncAndNotifyPictureReady, weak_this_,
       input_id, output_id));
 }