Add VAVDA, the VAAPI Video Decode Accelerator for Intel CPUs.

content/common/gpu/media/vaapi_video_decode_accelerator.cc

Index: content/common/gpu/media/vaapi_video_decode_accelerator.cc
diff --git a/content/common/gpu/media/vaapi_video_decode_accelerator.cc b/content/common/gpu/media/vaapi_video_decode_accelerator.cc
new file mode 100644
index 0000000000000000000000000000000000000000..8dfc0682a75cda305c751f52c616619cd8d58f76
--- /dev/null
+++ b/content/common/gpu/media/vaapi_video_decode_accelerator.cc
@@ -0,0 +1,447 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.

[Pawel Osciak, 2012/05/03 16:22:07]

As the whole threading is rewritten with the next CL, I'm not addressing all
comments.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "vaapi_video_decode_accelerator.h"
+
+#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"
+#include "base/synchronization/waitable_event.h"
+#include "gpu/command_buffer/service/gpu_switches.h"
+#include "content/public/common/content_switches.h"
+#include "content/common/gpu/gpu_channel.h"
+#include "media/video/picture.h"
+#include "third_party/libva/va/va.h"
+#include "ui/gfx/gl/gl_bindings.h"
+
+#define RETURN_AND_NOTIFY_ON_FAILURE(result, log, error_code, ret)  \
+  do {                                                              \
+    if (!(result)) {                                                \
+      DVLOG(1) << log;                                              \
+      StopOnError(error_code);                                      \
+      return ret;                                                   \
+    }                                                               \
+  } while (0)
+
+using content::VaapiH264Decoder;
+
+void VaapiVideoDecodeAccelerator::StopOnError(
+    media::VideoDecodeAccelerator::Error error) {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+
+  DVLOG(1) << "Stopping on error " << error;
+
+  decoder_thread_.Stop();

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

This will drain the thread's messageloop & wait for the thread to exit,
synchronously.  Did you mean to do that?
In particular, the decoder thread could be waiting for a new input and never
return here.

+
+  if (client_)
+    client_->NotifyError(error);
+  client_ = NULL;
+}
+
+VaapiVideoDecodeAccelerator::VaapiVideoDecodeAccelerator(
+    media::VideoDecodeAccelerator::Client* client)
+  : input_ready_(true, false),  // manually Reset() and initially not signalled

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

indent is off (and for the rest of the initializers below, too).

[Pawel Osciak, 2012/05/03 16:22:07]

Done.

+    message_loop_(MessageLoop::current()),
+    client_(client),
+    pictures_requested_(false),
+    after_reset_(false),
+    resetting_(false),
+    decoder_thread_("VaapiDecoderThread") {
+}
+
+VaapiVideoDecodeAccelerator::~VaapiVideoDecodeAccelerator() {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+}
+
+bool VaapiVideoDecodeAccelerator::Initialize(
+    media::VideoCodecProfile profile) {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+  bool res = true;

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

decl at first use, below.

[Pawel Osciak, 2012/05/03 16:22:07]

Done.

+
+  DVLOG(2) << "Initializing VAVDA, profile: " << profile;
+
+  res = CommandLine::ForCurrentProcess()->HasSwitch(switches::kEnableVaapi);

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

ISTM this would make more sense at a higher layer.
Either GVDA.cc, or even better yet, before reaching the GPU process in RVI
(http://code.google.com/searchframe#OAMlx_jo-ck/src/content/renderer/render_vi...)

[Pawel Osciak, 2012/05/03 16:22:07]

I'll make a TODO out of it.

+  if (!res) {
+    DVLOG(1) << "Vaapi HW acceleration disabled";
+    goto done;

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

seriously?  goto?

Are you just testing me for being awake? :)

Note that when Initialize returns false it *shouldn't* call NotifyInitializeDone
(instead, GVDA will call NotifyError on its behalf).  So you shouldn't need the
goto at all.

[Pawel Osciak, 2012/05/03 16:22:07]

Well, it's not forbidden by the code style :P
That was my understanding as well. But if I don't do that and just return false,
it will crash with sometimes different errors (some races are obviously going
on) instead of falling back to SW decode. I'd been struggling with this about a
month ago and the only reliable way I found was this. But now I revisited
briefly and it looks like while just returning false still crashes the process,
calling NotifyError from VAVDA seems to be enough, although I'm pretty sure I
was having problems even with that last time, otherwise I wouldn't have settled
with NotifyInitializeDone.


I wrote the above before syncing to ToT again, and now it stopped working in
both cases (just returning false or returning false + Notify). Wasn't able to
figure out why yet...

+  }
+
+  res = decoder_.Initialize(profile, x_display_, glx_context_,
+      base::Bind(&VaapiVideoDecodeAccelerator::OutputPicCallback,
+                 base::Unretained(this)));

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

Unretained is usually the wrong thing to do.  (see my comment in the .h file; I
think you want an unadorned "this" here)

[Pawel Osciak, 2012/05/03 16:22:07]

Done.

+  if (!res) {
+    DVLOG(1) << "Failed initializing decoder";
+    goto done;
+  }
+
+  res = decoder_thread_.Start();
+  if (!res)
+    DVLOG(1) << "Failed starting decoding thread";
+
+done:
+  MessageLoop::current()->PostTask(FROM_HERE,

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

you mean to say:
message_loop_->PostTask(FROM_HERE, base::Bind(
    &Client::NotifyInitializeDone, client_);

[Pawel Osciak, 2012/05/03 16:22:07]

As we discussed, we'd have to do Unretained(client_), which could have at least
theoretically been unsafe.

+      base::Bind(&VaapiVideoDecodeAccelerator::NotifyInitializeDone, this));
+  return res;

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

s/res/true/ assuming you early-return false on failure.

[Pawel Osciak, 2012/05/03 16:22:07]

Done.

+}
+
+void VaapiVideoDecodeAccelerator::NotifyInitializeDone() {

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

drop

+  DCHECK_EQ(message_loop_, MessageLoop::current());
+  if (client_)
+    client_->NotifyInitializeDone();
+}
+
+void VaapiVideoDecodeAccelerator::SetGlxState(Display* x_display,

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

I'm not sure there's a reason to make this a separate setter, instead of making
these params of the ctor.
(I know this mimics OVDA::SetEglState(), but I can't remember why I had that
that way or if it was unintentional bogosity).

[Pawel Osciak, 2012/05/03 16:22:07]

I have no preference. I just made it symmetrical to the existing ones. I think
it could be removed along with others.

+                                              GLXContext glx_context) {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+  x_display_ = x_display;
+  glx_context_ = glx_context;
+}
+
+void VaapiVideoDecodeAccelerator::NotifyInputBufferRead(int input_buffer_id) {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+
+  DVLOG(4) << "Notifying end of input buffer " << input_buffer_id;
+  if (client_)
+    client_->NotifyEndOfBitstreamBuffer(input_buffer_id);
+}
+
+void VaapiVideoDecodeAccelerator::SyncAndNotifyPictureReady(int32 input_id,
+                                                            int32 output_id) {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+
+  // Sync the contents of the texture.
+  if (!decoder_.PutPicToTexture(output_id)) {

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

Use the macro?

[Pawel Osciak, 2012/05/03 16:22:07]

Yep, and now we have use for macro not returning false.

+    DVLOG(1) << "Failed putting picture to texture";
+    StopOnError(PLATFORM_FAILURE);
+    return;
+  }
+
+  // And notify the client a picture is ready to be displayed.
+  media::Picture picture(output_id, input_id);
+  DVLOG(4) << "Notifying output picture id " << output_id
+           << " for input "<< input_id << " is ready";
+  if (client_)
+    client_->PictureReady(picture);
+}
+
+void VaapiVideoDecodeAccelerator::RequestPictureBuffers(int num_pics,
+                                                        int width,
+                                                        int height) {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+
+  DVLOG(1) << "Requesting " << num_pics << " pictures of size: "
+           << width << "x" << height;
+  if (client_)
+    client_->ProvidePictureBuffers(num_pics, gfx::Size(width, height));
+}
+
+void VaapiVideoDecodeAccelerator::TryGetNewInputBuffer() {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+  DCHECK(!input_ready_.IsSignaled());
+
+  // If current buffer is still set, return it to the client.

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

It seems strange to me that you only notify the client that an input has been
fully decoded when the decoder asks for the *next* chunk.  My guess is that this
could lead to livelock at EOS (without markers in the stream) b/c the decoder is
ready for more work, but the client knows it has nothing more to feed the
decoder and is just waiting for news that it is done with the last buffer to
send a flush.
Can you instead notify the client when the buffer is actually consumed?

[Pawel Osciak, 2012/05/03 16:22:07]

As we discussed on the call, this is not the case. The decoder will keep
decoding until it reaches the end of the buffer, return kNeedMoreStreamData and
VAVDA will then signal NotifyInputBufferRead immediately, without any additional
VDA::Decode() calls from the client.

+  if (curr_input_buffer_.get()) {
+    MessageLoop::current()->PostTask(FROM_HERE, base::Bind(
+        &VaapiVideoDecodeAccelerator::NotifyInputBufferRead, this,
+        curr_input_buffer_->id));
+
+    curr_input_buffer_.reset();
+  }
+
+  // If there is no more input buffers ready, return. We will come back

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

s/is/are/

+  // here the next time client provides us with a new input buffer.
+  if (input_buffers_.empty())
+    return;
+
+  // Otherwise pop a new input buffer and set it up for the decoder thread.
+  curr_input_buffer_.reset(input_buffers_.front());
+  input_buffers_.pop();
+
+  DVLOG(4) << "New current bitstream buffer, id: " << curr_input_buffer_->id
+           << " size: " << (int)curr_input_buffer_->size;
+
+  // Decoder is waiting on |input_ready_|, so we are safe to do this.

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

This is some very fiddly logic!  You are effectively using the combination of
WaitableEvent and the fact that *this* method must run on the render thread as a
guarantee of mutual exclusion.  

Please see the "However" warning on
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/synchronization/waita...
I think you'd be better off with a Lock+ConditionVariable here.

For example, I wonder if this is correct in the presence of Reset() and
already-posted TGNIB tasks.

+  decoder_.SetStream((uint8*)curr_input_buffer_->shm->memory(),

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

static_cast

[Pawel Osciak, 2012/05/03 16:22:07]

Done.

+                     curr_input_buffer_->size);
+
+  // Wake up the decoder thread and let it continue.
+  input_ready_.Signal();
+}
+
+void VaapiVideoDecodeAccelerator::MapAndQueueNewInputBuffer(
+    const media::BitstreamBuffer& bitstream_buffer) {
+  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));
+  CHECK(shm.get());

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

You never need to CHECK the result of a new in chromium.  A failure to allocate
will terminate the process.
(here and elsewhere)

[Pawel Osciak, 2012/05/03 16:22:07]

Done.

[Pawel Osciak, 2012/05/03 16:22:07]

Done.

+  RETURN_AND_NOTIFY_ON_FAILURE(shm->Map(bitstream_buffer.size()),
+                              "Failed to map input buffer", UNREADABLE_INPUT,);
+
+  // Set up a new input buffer and queue it for later.
+  InputBuffer* input_buffer = new InputBuffer();
+  CHECK(input_buffer);
+  input_buffer->shm.reset(shm.release());
+  input_buffer->id = bitstream_buffer.id();
+  input_buffer->size = bitstream_buffer.size();
+  input_buffers_.push(input_buffer);
+}
+
+void VaapiVideoDecodeAccelerator::WaitForInput() {
+  DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
+
+  // Set us up for sleep, request new input buffer from the main thread
+  // and go to sleep waiting for good news.
+  input_ready_.Reset();
+  message_loop_->PostTask(FROM_HERE,
+      base::Bind(&VaapiVideoDecodeAccelerator::TryGetNewInputBuffer, this));
+  input_ready_.Wait();
+}
+
+void VaapiVideoDecodeAccelerator::DecodeTask() {
+  DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
+  VaapiH264Decoder::DecResult res;
+
+  // Main decode task.
+  DVLOG(4) << "Decode task";
+
+  DCHECK(input_ready_.IsSignaled());
+
+  if (resetting_) {
+    // Will get back here once we are done with reset. This is needed so we
+    // don't sleep indefinitely blocking the decoder from being forced
+    // to reset.
+    DVLOG(4) << "Resetting, leaving DecodeTask";
+    return;
+  }
+
+  // Could happen after reset, when we are still waiting for a new buffer.
+  if (!curr_input_buffer_.get())
+    return;
+
+  if (!pictures_requested_ || after_reset_) {
+    // Try to initialize or resume playback after reset.
+    res = decoder_.DecodeInitial(curr_input_buffer_->id);
+    switch (res) {
+      case VaapiH264Decoder::kReadyToDecode:
+        if (!pictures_requested_) {
+          // Decoder decoded initial stream information and is ready
+          // to receive output pictures. Request them from the client.
+          message_loop_->PostTask(FROM_HERE, base::Bind(
+              &VaapiVideoDecodeAccelerator::RequestPictureBuffers, this,
+              decoder_.GetRequiredNumOfPictures(),
+              decoder_.pic_width(), decoder_.pic_height()));
+          pictures_requested_ = true;
+        } else {
+          // We are after reset and successfully found a point from which
+          // we can resume normal decoding, so post a task for that.
+          DCHECK(after_reset_);
+          after_reset_ = false;
+          decoder_thread_.message_loop()->PostTask(FROM_HERE,

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

?? we're already on the decoder's thread.  Why post another task?

[Pawel Osciak, 2012/05/03 16:22:07]

The design was one task per frame. But not relevant anymore.

+              base::Bind(&VaapiVideoDecodeAccelerator::DecodeTask, this));
+        }
+        break;

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

This method might become slightly less confusing if you s/break/return/ from
here down (to emph. that there's on common code that executes after each of the
switch cases).

+
+      case VaapiH264Decoder::kNeedMoreStreamData:
+        DVLOG(2) << "Waiting for more stream data to (re)initialize";
+        // May sleep.
+        WaitForInput();
+        break;
+
+      case VaapiH264Decoder::kDecodeError:
+        DVLOG(1) << "Error decoding stream";
+        message_loop_->PostTask(FROM_HERE,
+            base::Bind(&VaapiVideoDecodeAccelerator::StopOnError, this,
+                       PLATFORM_FAILURE));
+        break;
+
+      default:

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

drop default cases everywhere in this CL unless they're needed (let compiler
warn you of missing cases).

[Pawel Osciak, 2012/05/03 16:22:07]

Actually, this one is needed. If the decoder returned kDecodedFrame or
kNoOutputAvailable it would've been a bug.

+        NOTREACHED() << "Unexpected result from the decoder";
+    }
+  } else {
+    // Try to decode what stream data is in the decoder until we get
+    // a frame or run out of input stream.
+    res = decoder_.DecodeOneFrame(curr_input_buffer_->id);
+    switch (res) {
+      case VaapiH264Decoder::kNeedMoreStreamData:
+        // May sleep.
+        WaitForInput();
+        // fallthrough
+      case VaapiH264Decoder::kDecodedFrame:
+        // (still) have more stream, try to decode more
+        decoder_thread_.message_loop()->PostTask(FROM_HERE,

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

ditto already on decoder thread

[Pawel Osciak, 2012/05/03 16:22:07]

This time, kReadyToDecode would've been a bug.

+            base::Bind(&VaapiVideoDecodeAccelerator::DecodeTask, this));
+        break;
+
+      case VaapiH264Decoder::kNoOutputAvailable:
+        // The decoder does not have any output buffers available, so return.
+        // We will come back once the client gives us back a picture buffer
+        // after displaying it.
+        break;
+
+      case VaapiH264Decoder::kDecodeError:
+        DVLOG(1) << "Error decoding stream";
+        message_loop_->PostTask(FROM_HERE,
+            base::Bind(&VaapiVideoDecodeAccelerator::StopOnError, this,
+                       PLATFORM_FAILURE));
+        break;
+
+      default:
+        NOTREACHED() << "Unexpected result from the decoder";
+    }
+  }
+}
+
+
+void VaapiVideoDecodeAccelerator::Decode(
+    const media::BitstreamBuffer& bitstream_buffer) {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+
+  TRACE_EVENT1("Video Decoder", "VAVDA::Decode", "Buffer id",
+               bitstream_buffer.id());
+
+  // We got a new input buffer from the client, map it and queue for later use.
+  MapAndQueueNewInputBuffer(bitstream_buffer);
+
+  // Set it up already if there is no current one.
+  if (!curr_input_buffer_.get())
+    TryGetNewInputBuffer();
+
+  decoder_thread_.message_loop()->PostTask(FROM_HERE,
+      base::Bind(&VaapiVideoDecodeAccelerator::DecodeTask, this));
+}
+
+void VaapiVideoDecodeAccelerator::AssignPictureBuffers(
+    const std::vector<media::PictureBuffer>& buffers) {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+
+  // Here we call sensitive decoder functions out of its thread, but
+  // the decoder thread is not running yet, so we can safely do this.

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

Initialize() calls decoder_thread_.Start(), and Initialize must complete before
AssignPictureBuffers is called (in fact, Decode() must have been called before
APB is called, or else the decoder wouldn't know what dimension pictures to ask
for).

+  for (size_t i = 0; i < buffers.size(); ++i) {
+    DVLOG(2) << "Assigning picture id " << buffers[i].id()
+             << " to texture id " << buffers[i].texture_id();
+    decoder_.AssignPictureBuffer(buffers[i].id(), buffers[i].texture_id());
+  }
+
+  decoder_thread_.message_loop()->PostTask(FROM_HERE,

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

Why is this the right thing to do?

+      base::Bind(&VaapiVideoDecodeAccelerator::DecodeTask, this));
+}
+
+void VaapiVideoDecodeAccelerator::ReusePictureTask(int32 picture_buffer_id) {
+  DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
+
+  decoder_.ReusePictureBuffer(picture_buffer_id);
+
+  decoder_thread_.message_loop()->PostTask(FROM_HERE,

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

already on the decoder thread.

+      base::Bind(&VaapiVideoDecodeAccelerator::DecodeTask, this));

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

Why is this right?

+}
+
+void VaapiVideoDecodeAccelerator::ReusePictureBuffer(int32 picture_buffer_id) {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+  TRACE_EVENT1("Video Decoder", "VAVDA::ReusePictureBuffer",
+               "Picture id", picture_buffer_id);
+
+  decoder_thread_.message_loop()->PostTask(FROM_HERE,
+      base::Bind(&VaapiVideoDecodeAccelerator::ReusePictureTask, this,
+                 picture_buffer_id));
+}
+
+void VaapiVideoDecodeAccelerator::FlushTask() {
+  DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
+
+  decoder_.Flush();

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

This only emits already-decoded pictures, but the contract of VDA::Flush() is
that it NotifyFlushDone()s only when all inputs are done decoding, which can
include inputs queued in VAVDA here.

Generally the fact that this impl is synchronous should be a red flag.

+  decoder_.Reset();

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

Why is Reset() necessary?

+
+  message_loop_->PostTask(FROM_HERE,
+      base::Bind(&VaapiVideoDecodeAccelerator::NotifyFlushDone, this));
+
+  WaitForInput();

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

why?

+}
+
+void VaapiVideoDecodeAccelerator::Flush() {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+
+  DVLOG(1) << "Got flush request";
+  decoder_thread_.message_loop()->PostTask(FROM_HERE,
+      base::Bind(&VaapiVideoDecodeAccelerator::FlushTask, this));
+}
+
+void VaapiVideoDecodeAccelerator::NotifyFlushDone() {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+  if (client_)
+    client_->NotifyFlushDone();
+  DVLOG(1) << "Flush done";
+}
+
+void VaapiVideoDecodeAccelerator::ResetTask() {
+  DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
+
+  // All the decoding tasks should be done by now, since the client
+  // does not give more input and we have scheduled ourselves after
+  // we got a reset request.
+  decoder_.Reset();
+
+  // Make sure next time decode tasks are run they will reinitialize.
+  after_reset_ = true;
+
+  // And let client know that we are done with reset.
+  message_loop_->PostTask(FROM_HERE, base::Bind(
+      &VaapiVideoDecodeAccelerator::NotifyResetDone, this));
+
+  //WaitForInput();

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

??

+}
+
+void VaapiVideoDecodeAccelerator::Reset() {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+
+  DVLOG(1) << "Got reset request";
+  decoder_thread_.message_loop()->PostTask(FROM_HERE,
+      base::Bind(&VaapiVideoDecodeAccelerator::ResetTask, this));
+  resetting_ = true;

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

this should go before the above call to avoid racing.

+  // Release waiting decoder thread, if any. Any additional already posted
+  // decoding threads will exit immediately due to resetting_ set to true.

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

s/threads/tasks/

+  input_ready_.Signal();
+}
+
+void VaapiVideoDecodeAccelerator::NotifyResetDone() {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+
+  if (client_)
+    client_->NotifyResetDone();
+  resetting_ = false;
+  DVLOG(1) << "Reset done";
+}
+
+void VaapiVideoDecodeAccelerator::Destroy() {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+
+  decoder_thread_.Stop();
+  // No decoder tasks running anymore, so safe to destroy.

[Ami GONE FROM CHROMIUM, 2012/04/09 21:35:53]

decoder_ is also accessed on the main thread, not just on the decoder thread,
and you don't have any guards to prevent already-queued tasks from attempting to
use a Destroy()ed decoder.

+  decoder_.Destroy();
+  client_ = NULL;
+}
+
+//static
+void VaapiVideoDecodeAccelerator::OutputPicCallback(
+    VaapiVideoDecodeAccelerator* vavda,
+    int32 input_id,
+    int32 output_id) {
+  TRACE_EVENT2("Video Decoder", "VAVDA::OutputPicCallback",
+               "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(vavda->decoder_thread_.message_loop(), MessageLoop::current());
+  vavda->message_loop_->PostTask(FROM_HERE,
+      base::Bind(&VaapiVideoDecodeAccelerator::SyncAndNotifyPictureReady,
+                 vavda, input_id, output_id));
+}
+