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

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 "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;                                              \
+      Destroy();                                                    \

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

Does this want to:
DCHECK_EQ(message_loop_, MessageLoop::current());
?

[Pawel Osciak, 2012/05/06 17:49:19]

The functions will re-post themselves as needed.

+      NotifyError(error_code);                                      \
+      return ret;                                                   \
+    }                                                               \
+  } while (0)
+
+using content::VaapiH264Decoder;
+
+void VaapiVideoDecodeAccelerator::NotifyError(Error error) {
+  if (message_loop_ != MessageLoop::current()) {

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

I think this can never be true, since the only caller is the R_A_N_O_F() macro,
and that calls Destroy(), and that requires being on message_loop_?

[Pawel Osciak, 2012/05/06 17:49:19]

True, Destroy() should be re-posting itself.

+    message_loop_->PostTask(FROM_HERE, base::Bind(
+        &VaapiVideoDecodeAccelerator::NotifyError, this, error));
+    return;
+  }
+
+  DVLOG(1) << "Stopping on error " << error;

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

S/Stopping on/Notifying of/

[Pawel Osciak, 2012/05/06 17:49:19]

Done.

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

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

drop comment (belongs to old WE impl)

[Pawel Osciak, 2012/05/06 17:49:19]

Done.

+    : state_(kUninitialized),
+      input_ready_(&lock_),
+      output_ready_(&lock_),
+      message_loop_(MessageLoop::current()),
+      client_(client),
+      decoder_thread_("VaapiDecoderThread") {
+}
+
+VaapiVideoDecodeAccelerator::~VaapiVideoDecodeAccelerator() {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+}
+
+bool VaapiVideoDecodeAccelerator::Initialize(
+    media::VideoCodecProfile profile) {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+
+  DCHECK_EQ(state_, kUninitialized);
+  DCHECK(client_);

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

move to ctor

[Pawel Osciak, 2012/05/06 17:49:19]

Done.

+  DVLOG(2) << "Initializing VAVDA, profile: " << profile;
+
+  // TODO(posciak): try moving the flag check up to higher layers, possibly
+  // out of the GPU process.
+  bool res = CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableVaapi);
+  RETURN_AND_NOTIFY_ON_FAILURE(res, "Vaapi HW acceleration disabled",
+                               PLATFORM_FAILURE, false);
+
+  res = decoder_.Initialize(profile, x_display_, glx_context_,
+      base::Bind(&VaapiVideoDecodeAccelerator::OutputPicCallback, this));

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

indent style is inconsistent with first arg above.

[Pawel Osciak, 2012/05/06 17:49:19]

Done.

+  RETURN_AND_NOTIFY_ON_FAILURE(res, "Failed initializing decoder",
+                               PLATFORM_FAILURE, false);
+
+  res = decoder_thread_.Start();
+  RETURN_AND_NOTIFY_ON_FAILURE(res, "Failed starting decoder thread",
+                               PLATFORM_FAILURE, false);
+
+  state_ = kInitialized;
+
+  message_loop_->PostTask(FROM_HERE, base::Bind(
+      &VaapiVideoDecodeAccelerator::NotifyInitializeDone, this));

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

GVDA is in charge of calling NotifyInitializeDone() (if this method returns
true) or NotifyError() (if this method returns false).  You shouldn't have to
post this task here, and you should be able to outright delete
VAVDA::NotifyInitializeDone() below.

(you can confirm by adding a log stmt to GVD::NotifyInitializeDone and ensuring
it's only called once per <video> tag).

[Pawel Osciak, 2012/05/06 17:49:19]

As per chat, this is required, at least for now.

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

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

Add TODO to move these to be ctor params?

[Pawel Osciak, 2012/05/06 17:49:19]

Done.

+                                              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;
+  DCHECK(client_);

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

DCHECK() means it is a programming invariant that the condition is always true
(independent of user data).
Following a DCHECK() with an if() means one of them is wrong or spurious.
In this case, client_ can be NULL if an error happened and then an input buffer
was drained (possible due to delayed delivery of tasks in one thread vs. the
other?), so I think the DCHECK should be removed.

[Pawel Osciak, 2012/05/06 17:49:19]

Yeah, debugging artifact.

+  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.
+  RETURN_AND_NOTIFY_ON_FAILURE(decoder_.PutPicToTexture(output_id),
+                               "Failed putting picture to texture",
+                               PLATFORM_FAILURE, );
+
+  // 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";
+  DCHECK(client_);

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

Ditto remove the DCHECK.  
Please make a pass over the CL looking for these DCHECK+if pairs.

[Pawel Osciak, 2012/05/06 17:49:19]

Done.

+  if (client_)
+    client_->PictureReady(picture);
+}
+
+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));
+  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.
+  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());
+
+  // 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);
+    switch (res) {
+      case VaapiH264Decoder::kReadyToDecode:
+        message_loop_->PostTask(FROM_HERE, base::Bind(
+            &VaapiVideoDecodeAccelerator::ReadyToDecode, this,
+                decoder_.GetRequiredNumOfPictures(),
+                decoder_.pic_width(), decoder_.pic_height()));
+        return;
+
+      case VaapiH264Decoder::kNeedMoreStreamData:
+        ReturnCurrInputBuffer();
+        break;
+
+      case VaapiH264Decoder::kDecodeError:
+        RETURN_AND_NOTIFY_ON_FAILURE(false, "Error in decoding",
+                                     PLATFORM_FAILURE, );
+
+      default:
+        RETURN_AND_NOTIFY_ON_FAILURE(false, "Unexpected result from decoder",

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

s/decoder",/decoder: " << res,/
will also log the unexpected result code.

[Pawel Osciak, 2012/05/06 17:49:19]

Done.

+                                     PLATFORM_FAILURE, );
+    }
+  }
+}
+
+bool VaapiVideoDecodeAccelerator::GetInputBuffer() {
+  DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
+
+  base::AutoLock auto_lock(lock_);
+
+  if (curr_input_buffer_.get())
+    return true;
+
+  while (input_buffers_.empty() &&
+         (state_ == kDecoding || state_ == kInitialized || state_ == kIdle))
+    input_ready_.Wait();

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

multi-line while/if/for statement requires braces around body.

[Pawel Osciak, 2012/05/06 17:49:19]

Done.

+
+  if (state_ != kDecoding && state_ != kInitialized && state_ != kIdle)
+    return false;
+
+  curr_input_buffer_ = input_buffers_.front();
+  input_buffers_.pop();
+
+  DVLOG(4) << "New current bitstream buffer, id: " << curr_input_buffer_->id
+           << " size: " << (int)curr_input_buffer_->size;

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

(int) unnecessary
(and if it was, it should be using static_cast<int>())

[Pawel Osciak, 2012/05/06 17:49:19]

Done.

+
+  decoder_.SetStream(static_cast<uint8*>(curr_input_buffer_->shm->memory()),
+                     curr_input_buffer_->size);
+  return true;
+}
+
+void VaapiVideoDecodeAccelerator::ReturnCurrInputBuffer() {
+  DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
+
+  DCHECK(curr_input_buffer_.get());
+  int32 id = curr_input_buffer_->id;
+  curr_input_buffer_.reset();
+  message_loop_->PostTask(FROM_HERE, base::Bind(
+        &VaapiVideoDecodeAccelerator::NotifyInputBufferRead, this, id));
+}
+
+bool VaapiVideoDecodeAccelerator::GetOutputBuffers() {

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

This method doesn't call RequestPictureBuffers.  Would it be more accurately
named WaitForReusedPictureBuffers()?

[Pawel Osciak, 2012/05/06 17:49:19]

GetInputBuffer() above doesn't request them either... I was considering the same
thing, but settled with this name to emphasize that its result is getting
input/output buffers already set up in decoder, regardless of whether it waited
or not.

+  DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
+
+  base::AutoLock auto_lock(lock_);
+
+  while (output_buffers_.empty() && state_ == kDecoding)
+    output_ready_.Wait();
+
+  if (state_ != kDecoding)
+    return false;
+
+  while (!output_buffers_.empty()) {
+    decoder_.ReusePictureBuffer(output_buffers_.front());
+    output_buffers_.pop();
+  }
+
+  return true;
+}
+
+void VaapiVideoDecodeAccelerator::DecodeTask() {
+  DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
+  VaapiH264Decoder::DecResult res;

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

declare at first use below?

[Pawel Osciak, 2012/05/06 17:49:19]

Done.

+
+  // Main decode task.
+  DVLOG(4) << "Decode task";
+  DCHECK_EQ(state_, kDecoding);
+
+  // 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());
+
+    res = decoder_.DecodeOneFrame(curr_input_buffer_->id);
+    switch (res) {
+      case VaapiH264Decoder::kNeedMoreStreamData:
+        ReturnCurrInputBuffer();
+        break;
+
+      case VaapiH264Decoder::kDecodedFrame:
+        // May still have more stream data, continue decoding.
+        break;
+
+      case VaapiH264Decoder::kNoOutputAvailable:
+        // No more output buffers in the decoder, try getting more or go to
+        // sleep waiting for them.
+        if (!GetOutputBuffers())
+          return;
+        break;
+
+      case VaapiH264Decoder::kDecodeError:
+        RETURN_AND_NOTIFY_ON_FAILURE(false, "Error decoding stream",
+                                     PLATFORM_FAILURE, );
+        return;
+
+      default:
+        RETURN_AND_NOTIFY_ON_FAILURE(false,
+            "Unexpected result from the decoder", PLATFORM_FAILURE, );

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

inconsistent indent style

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

s/",/" << res,/

[Pawel Osciak, 2012/05/06 17:49:19]

Now I know :)

[Pawel Osciak, 2012/05/06 17:49:19]

Done.

+        return;
+    }
+  }
+}
+
+void VaapiVideoDecodeAccelerator::ReadyToDecode(int num_pics,
+                                                int width,
+                                                int height) {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+
+  base::AutoLock auto_lock(lock_);
+  switch (state_) {
+    case kInitialized:
+      DVLOG(1) << "Requesting " << num_pics << " pictures of size: "
+               << width << "x" << height;
+      DCHECK(client_);
+      if (client_)
+        client_->ProvidePictureBuffers(num_pics, gfx::Size(width, height));

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

FWIW, passing a gfx::Size instead of two ints is less error-prone, so I'd
recommend doing it to this method, as well.

[Pawel Osciak, 2012/05/06 17:49:19]

Done.

+      state_ = kPicturesRequested;
+      break;
+    case kIdle:
+      state_ = kDecoding;
+      decoder_thread_.message_loop()->PostTask(FROM_HERE,
+          base::Bind(&VaapiVideoDecodeAccelerator::DecodeTask, this));
+      break;
+    default:
+      NOTREACHED() << "Invalid state";

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

RETURN_AND_NOTIFY_ON_FAILURE ?

[Pawel Osciak, 2012/05/06 17:49:19]

Well, this would be a a bug, so I thought NOTREACHED() i.e. DCHECK() was better.

+  }
+}
+
+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);
+
+  switch (state_) {

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

This is reading state_ without holding the lock_.
(in general, if you can do a run under tsan that'd be nice)

[Pawel Osciak, 2012/05/06 17:49:19]

Done.

+    case kInitialized:
+      // Initial decode to get the required size of output buffers.
+      decoder_thread_.message_loop()->PostTask(FROM_HERE,
+          base::Bind(&VaapiVideoDecodeAccelerator::InitialDecodeTask, this));
+      break;
+
+    case kPicturesRequested:
+      // Waiting for pictures, return.
+      break;
+
+    case kDecoding:
+      break;
+
+    case kIdle:
+      // Need to get decoder into suitable stream location to resume.
+      decoder_thread_.message_loop()->PostTask(FROM_HERE,
+          base::Bind(&VaapiVideoDecodeAccelerator::InitialDecodeTask, this));
+      break;
+
+    default:
+      return;

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

silently?  shouldn't this complain?

[Pawel Osciak, 2012/05/06 17:49:19]

This means we are shutting down/destroying or otherwise not interested in
running. But a good idea to have a DVLOG here.

+  }
+}
+
+void VaapiVideoDecodeAccelerator::AssignPictureBuffers(
+    const std::vector<media::PictureBuffer>& buffers) {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+
+  base::AutoLock auto_lock(lock_);
+  DCHECK_EQ(state_, kPicturesRequested);
+
+  for (size_t i = 0; i < buffers.size(); ++i) {
+    DVLOG(2) << "Assigning picture id " << buffers[i].id()
+             << " to texture id " << buffers[i].texture_id();
+
+    bool res = decoder_.AssignPictureBuffer(buffers[i].id(),
+        buffers[i].texture_id());

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

inconsistent indent.

[Pawel Osciak, 2012/05/06 17:49:19]

Done.

+    RETURN_AND_NOTIFY_ON_FAILURE(res, "Failed assigning picture buffer",

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

could also << the buffer's id & the texture's id (and the index i)

[Pawel Osciak, 2012/05/06 17:49:19]

Done.

+        PLATFORM_FAILURE, );
+  }
+
+  state_ = kDecoding;
+  decoder_thread_.message_loop()->PostTask(FROM_HERE,
+      base::Bind(&VaapiVideoDecodeAccelerator::DecodeTask, this));
+}
+
+void VaapiVideoDecodeAccelerator::ReusePictureBuffer(int32 picture_buffer_id) {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+  TRACE_EVENT1("Video Decoder", "VAVDA::ReusePictureBuffer", "Picture id",
+               picture_buffer_id);
+
+  base::AutoLock auto_lock(lock_);
+  output_buffers_.push(picture_buffer_id);
+  output_ready_.Signal();
+}
+
+void VaapiVideoDecodeAccelerator::FlushTask() {
+  DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
+  DVLOG(1) << "Flush task";
+
+  // First flush all the pictures that haven't been outputted, notifying the
+  // client to output them.
+  decoder_.Flush();

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

If this fails shouldn't this NotifyError instead of silently continuing and
claiming NotifyFlushDone() later?

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

I think you missed this comment from my previous review:

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.

[Pawel Osciak, 2012/05/06 17:49:19]

Good idea.

[Pawel Osciak, 2012/05/06 17:49:19]

This task is posted by Reset, so it gets queued after any existing decode task.
Reset sets state to kFlushing when doing that. The Decode task will keep
decoding the stream buffers already on input queue, but will never wait and once
it runs out, it will return and the ResetTask will execute cleaning up.

+
+  // Put the decoder in idle state, ready to resume.
+  decoder_.Reset();

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

Should Decoder::Flush always call its own Reset()?

[Pawel Osciak, 2012/05/06 17:49:19]

No, there are cases when it flushes itself without resetting (IDR frames).

+
+  message_loop_->PostTask(FROM_HERE,
+      base::Bind(&VaapiVideoDecodeAccelerator::FinishFlush, this));
+}
+
+void VaapiVideoDecodeAccelerator::Flush() {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+  DVLOG(1) << "Got flush request";
+
+  base::AutoLock auto_lock(lock_);
+  state_ = kFlushing;
+  // Queue a flush task after all existing decoding tasks to clean up.
+  decoder_thread_.message_loop()->PostTask(FROM_HERE,
+      base::Bind(&VaapiVideoDecodeAccelerator::FlushTask, this));
+
+  input_ready_.Signal();
+  output_ready_.Signal();

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

Both this pair of Signal()'s and the corresponding pair in Reset() below are
"the right thing to do" because of the state_ assignment above.  It would be
much clearer to put the Signal()'s right below the state_=kFlushing line (and
kResetting, below).

[Pawel Osciak, 2012/05/06 17:49:19]

I actually wanted to emphasize state being changed before posting the task, but
yeah, not really clear and doesn't really do anything.

+}
+
+void VaapiVideoDecodeAccelerator::FinishFlush() {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+
+  base::AutoLock auto_lock(lock_);
+
+  if (state_ != kFlushing)
+    return;  // We could've gotten destroyed already.

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

DCHECK that?

[Pawel Osciak, 2012/05/06 17:49:19]

This is an intentional early exit in case we get a Destroy() before we are done
with flushing. My understanding is it can happen and is not a bug, and we don't
NotifyFlushDone then.

+
+  state_ = kIdle;
+
+  if (client_)
+    client_->NotifyFlushDone();
+
+  DVLOG(1) << "Flush finished";
+}
+
+void VaapiVideoDecodeAccelerator::ResetTask() {
+  DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
+
+  // All the decoding tasks from before the reset request from client are done

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

english up this comment

[Pawel Osciak, 2012/05/06 17:49:19]

Done.

+  // by now, since the client should not we got a reset request.
+  decoder_.Reset();
+
+  base::AutoLock auto_lock(lock_);
+  // Return current input buffer, if present.
+  if (curr_input_buffer_.get())
+      ReturnCurrInputBuffer();

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

indent -2

[Pawel Osciak, 2012/05/06 17:49:19]

Done.

+
+  // And let client know that we are done with reset.
+  message_loop_->PostTask(FROM_HERE, base::Bind(
+      &VaapiVideoDecodeAccelerator::FinishReset, this));
+}
+
+void VaapiVideoDecodeAccelerator::Reset() {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+  DVLOG(1) << "Got reset request";
+
+  // This will make any new decode tasks exit early.
+  base::AutoLock auto_lock(lock_);
+  state_ = kResetting;
+
+  decoder_thread_.message_loop()->PostTask(FROM_HERE,
+      base::Bind(&VaapiVideoDecodeAccelerator::ResetTask, this));
+
+  input_ready_.Signal();
+  output_ready_.Signal();
+}
+
+void VaapiVideoDecodeAccelerator::FinishReset() {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+
+  base::AutoLock auto_lock(lock_);
+  if (state_ != kResetting)
+    return;  // We could've gotten destroyed already.

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

DCHECK that

[Pawel Osciak, 2012/05/06 17:49:19]

This is an intentional early exit, if we get a destroy request during reset we
just want to get out immediately.

+
+  // Drop all remaining input buffers, if present.
+  while (!input_buffers_.empty())
+    input_buffers_.pop();
+
+  state_ = kIdle;
+
+  if (client_)
+    client_->NotifyResetDone();
+
+  DVLOG(1) << "Reset finished";
+}
+
+void VaapiVideoDecodeAccelerator::DestroyTask() {
+  DCHECK_EQ(decoder_thread_.message_loop(), MessageLoop::current());
+
+  base::AutoLock auto_lock(lock_);
+  decoder_.Destroy();
+  state_ = kUninitialized;
+}
+
+void VaapiVideoDecodeAccelerator::Destroy() {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+
+  if (state_ == kUninitialized || state_ == kDestroying)

[Ami GONE FROM CHROMIUM, 2012/05/03 23:22:53]

This reads state_ not under lock_.
Move the auto_lock at l.495 to above this.
Please make a pass to ensure state_ &friends are never read without lock_ held.

[Pawel Osciak, 2012/05/06 17:49:19]

All the places had been properly checked :)
This is intentional, so that RETURN_AND_NOTIFY_ON_FAILURE would work from
anywhere. Note that state_ is only changed on the child thread, so it can't
change here (there is small exception of DestroyTask, but it switches from
kDestroying to kUninitialized and if it's running that means we've been
destroy()ed already and not expecting any other calls).
I didn't want to have a separate function on child thread to just do that change
transition, but it'll be much clearer I guess, so I'll add one.

Note that I don't really have to take the lock on child thread for the state
transitions due to the above (with the assumption of atomicity of assignments to
state of course), but I do anyway.

+    return;
+
+  DVLOG(1) << "Destroying VAVDA";
+
+  base::AutoLock auto_lock(lock_);
+  state_ = kDestroying;
+  input_ready_.Signal();
+  output_ready_.Signal();
+
+  decoder_thread_.message_loop()->PostTask(FROM_HERE,
+      base::Bind(&VaapiVideoDecodeAccelerator::DestroyTask, this));
+  client_ = NULL;
+}
+
+void VaapiVideoDecodeAccelerator::OutputPicCallback(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(decoder_thread_.message_loop(), MessageLoop::current());
+  message_loop_->PostTask(FROM_HERE,
+      base::Bind(&VaapiVideoDecodeAccelerator::SyncAndNotifyPictureReady,
+                 this, input_id, output_id));
+}
+