Initial implementation of the DXVA 2.0 H.264 hardware decoder for pepper for Windows. The decodin...

content/common/gpu/media/dxva_video_decode_accelerator.cc

+// Copyright (c) 2011 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.
+

[Ami GONE FROM CHROMIUM, 2011/12/13 07:24:56]

Assert this file is only built on OS_WIN:
#if !defined(OS_WIN)
#error Oops
#endif 
?

(I thought I had something similar in OVDA but I guess not)

[ananta, 2011/12/13 23:29:15]

Done.

[ananta, 2011/12/13 23:29:15]

Done.

+#include "content/common/gpu/media/dxva_video_decode_accelerator.h"
+
+#include <ks.h>
+#include <codecapi.h>
+#include <d3dx9tex.h>
+#include <mfapi.h>
+#include <mferror.h>
+#include <wmcodecdsp.h>
+
+#include "base/lazy_instance.h"

[Ami GONE FROM CHROMIUM, 2011/12/13 07:24:56]

remove

[ananta, 2011/12/13 23:29:15]

Done.

[ananta, 2011/12/13 23:29:15]

Done.

+#include "base/logging.h"
+#include "base/memory/scoped_handle.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop.h"

[Ami GONE FROM CHROMIUM, 2011/12/13 07:24:56]

can remove if you take my NonThreadSafe suggestion

[ananta, 2011/12/13 23:29:15]

Done.

[ananta, 2011/12/13 23:29:15]

Done.

+#include "base/shared_memory.h"
+#include "base/time.h"
+#include "media/video/video_decode_accelerator.h"
+#include "third_party/angle/include/GLES2/gl2.h"
+#include "third_party/angle/include/GLES2/gl2ext.h"
+
+namespace {

[Ami GONE FROM CHROMIUM, 2011/12/13 07:24:56]

Since almost all contents of this are static and anonymous namespaces add
tooling friction, I'd remove it.

[ananta, 2011/12/13 23:29:15]

Done.

[ananta, 2011/12/13 23:29:15]

Done.

+
+static const int kNumPictureBuffers = 5;
+static const int kNumInputs = 100;

[Ami GONE FROM CHROMIUM, 2011/12/13 07:24:56]

Are these magic DXVA constants?  Where do they come from?
Add comments?

+
+IMFSample* CreateEmptySample() {

[Ami GONE FROM CHROMIUM, 2011/12/13 07:24:56]

static

[ananta, 2011/12/13 23:29:15]

Done.

+  HRESULT hr = E_FAIL;
+  base::win::ScopedComPtr<IMFSample> sample;
+  hr = MFCreateSample(sample.Receive());
+  if (FAILED(hr)) {

[Ami GONE FROM CHROMIUM, 2011/12/13 07:24:56]

This seems like a pretty common pattern (similar to OVDA).
You might want to do something like
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/common/gpu/me...

[ananta, 2011/12/13 23:29:15]

Done.

+    NOTREACHED() << "Unable to create an empty sample";
+    return NULL;
+  }
+  return sample.Detach();
+}
+
+// Creates a Media Foundation sample with one buffer of length |buffer_length|
+// on a |align|-byte boundary. Alignment must be a perfect power of 2 or 0.
+// If |align| is 0, then no alignment is specified.

[Ami GONE FROM CHROMIUM, 2011/12/13 07:24:56]

drop this line?

[ananta, 2011/12/13 23:29:15]

Done.

[ananta, 2011/12/13 23:29:15]

Done.

+IMFSample* CreateEmptySampleWithBuffer(int buffer_length, int align) {
+  CHECK_GT(buffer_length, 0);
+  base::win::ScopedComPtr<IMFSample> sample;
+  sample.Attach(CreateEmptySample());
+  if (!sample.get())
+    return NULL;
+  base::win::ScopedComPtr<IMFMediaBuffer> buffer;
+  HRESULT hr = E_FAIL;
+  if (align == 0) {
+    // Note that MFCreateMemoryBuffer is same as MFCreateAlignedMemoryBuffer
+    // with the align argument being 0.
+    hr = MFCreateMemoryBuffer(buffer_length, buffer.Receive());
+  } else {
+    hr = MFCreateAlignedMemoryBuffer(buffer_length,
+                                     align - 1,
+                                     buffer.Receive());
+  }
+  if (FAILED(hr)) {
+    NOTREACHED() << "Unable to create an empty buffer";
+    return NULL;
+  }
+  hr = sample->AddBuffer(buffer.get());
+  if (FAILED(hr)) {
+    NOTREACHED() << "Failed to add empty buffer to sample";
+    return NULL;
+  }
+  return sample.Detach();
+}
+
+// Creates a Media Foundation sample with one buffer containing a copy of the
+// given Annex B stream data.
+// If duration and sample time are not known, provide 0.
+// |min_size| specifies the minimum size of the buffer (might be required by
+// the decoder for input). The times here should be given in 100ns units.

[Ami GONE FROM CHROMIUM, 2011/12/13 07:24:56]

Units of 100ns is pretty unchrome-y.  
Is it not a good idea to use TimeDelta here?

[ananta, 2011/12/13 23:29:15]

Unnecessary now. Removed these parameters as we don't receive them from the
plugin.

[ananta, 2011/12/13 23:29:15]

Removed these parameters as they are not needed.

+// |alignment| specifies the buffer in the sample to be aligned. If no
+// alignment is required, provide 0 or 1.

[Ami GONE FROM CHROMIUM, 2011/12/13 07:24:56]

"0 or 1"?  Any reason to allow/suggest both?

[ananta, 2011/12/13 23:29:15]

Bad comment. Fixed.

[ananta, 2011/12/13 23:29:15]

Fixed

+static IMFSample* CreateInputSample(const uint8* stream, int size,
+                                    int64 timestamp, int64 duration,
+                                    int min_size, int alignment) {
+  CHECK(stream);
+  CHECK_GT(size, 0);
+  base::win::ScopedComPtr<IMFSample> sample;
+  sample.Attach(CreateEmptySampleWithBuffer(std::max(min_size, size),
+                                            alignment));
+  if (!sample.get()) {
+    NOTREACHED() << "Failed to create empty buffer for input";
+    return NULL;
+  }
+  HRESULT hr = E_FAIL;
+  if (duration > 0) {
+    hr = sample->SetSampleDuration(duration);
+    if (FAILED(hr)) {
+      NOTREACHED() << "Failed to set sample duration";
+      return NULL;
+    }
+  }
+  if (timestamp > 0) {
+    hr = sample->SetSampleTime(timestamp);
+    if (FAILED(hr)) {
+      NOTREACHED() << "Failed to set sample time";
+      return NULL;
+    }
+  }
+  base::win::ScopedComPtr<IMFMediaBuffer> buffer;
+  hr = sample->GetBufferByIndex(0, buffer.Receive());
+  if (FAILED(hr)) {
+    NOTREACHED() << "Failed to get buffer in sample";
+    return NULL;
+  }
+  DWORD max_length = 0, current_length = 0;

[Ami GONE FROM CHROMIUM, 2011/12/13 07:24:56]

comma is unchrome-y.

[ananta, 2011/12/13 23:29:15]

Done.

[ananta, 2011/12/13 23:29:15]

Done.

+  uint8* destination = NULL;
+  hr = buffer->Lock(&destination, &max_length, &current_length);
+  if (FAILED(hr)) {
+    NOTREACHED() << "Failed to lock buffer";
+    return NULL;
+  }
+  CHECK_EQ(current_length, 0u);
+  CHECK_GE(static_cast<int>(max_length), size);
+  memcpy(destination, stream, size);
+  CHECK(SUCCEEDED(buffer->Unlock()));
+  hr = buffer->SetCurrentLength(size);
+  if (FAILED(hr)) {
+    NOTREACHED() << "Failed to set current length to " << size;
+    return NULL;
+  }
+  hr = sample->SetUINT32(MFSampleExtension_CleanPoint, TRUE);
+  if (FAILED(hr)) {
+    NOTREACHED() << "Failed to mark sample as key sample";
+    return NULL;
+  }
+  return sample.Detach();
+}
+
+}  // namespace
+
+DXVAVideoDecodeAccelerator::DXVAVideoDecodeAccelerator(
+    media::VideoDecodeAccelerator::Client* client,
+    base::ProcessHandle renderer_process)
+    : client_(client),
+      message_loop_(MessageLoop::current()->message_loop_proxy()),
+      surface_width_(0),
+      surface_height_(0),
+      state_(kUninitialized),
+      input_stream_info_(),
+      output_stream_info_(),

[Ami GONE FROM CHROMIUM, 2011/12/13 07:24:56]

This and the previous line are unnecessary, I think.

[ananta, 2011/12/13 23:29:15]

Done.

[ananta, 2011/12/13 23:29:15]

Done.

+      pictures_requested_(false),
+      renderer_process_(renderer_process),
+      dev_manager_reset_token_(0) {
+  input_buffer_frame_times_.reserve(kNumInputs);
+
+#if !defined(NDEBUG)
+  decode_start_time_ = 0;
+  inputs_before_decode_ = 0;
+#endif  // !defined(NDEBUG)
+}
+
+DXVAVideoDecodeAccelerator::~DXVAVideoDecodeAccelerator() {
+  client_ = NULL;
+  message_loop_ = NULL;
+}
+
+bool DXVAVideoDecodeAccelerator::Initialize(Profile profile) {

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

profile is unused in this file.  Does the MF decoder not have that as an input
param somewhere?

[ananta, 2011/12/13 23:29:15]

We don't need to pass it specifically.

+  DCHECK_EQ(message_loop_, MessageLoop::current()->message_loop_proxy());
+
+  if (state_ != kUninitialized) {
+    NOTREACHED() << "Initialize: invalid state: "

[Ami GONE FROM CHROMIUM, 2011/12/13 07:24:56]

One line.

[ananta, 2011/12/13 23:29:15]

Done.

+                 << state_;
+    return false;
+  }
+
+  HRESULT hr = MFStartup(MF_VERSION, MFSTARTUP_FULL);
+  if (FAILED(hr)) {
+    NOTREACHED() << "MFStartup failed. Error:"

[Ami GONE FROM CHROMIUM, 2011/12/13 07:24:56]

Single line (here and below; I'm going to stop commenting on that).

[ananta, 2011/12/13 23:29:15]

Done.

+                 << std::hex << std::showbase << hr;

[cpu_(ooo_6.6-7.5), 2011/12/13 19:28:57]

is the object behind NOTREACHED an actual iostream? in other words do std::hex
and std::showbase do anything?

[Ami GONE FROM CHROMIUM, 2011/12/13 19:30:23]

Yes; NOTREACHED is (always) a DCHECK(false) under the covers.

+    return false;
+  }
+  if (!CreateD3DDevManager())
+    return false;
+  if (!InitDecoder())
+    return false;
+  if (!GetStreamsInfoAndBufferReqs())
+    return false;
+  if (SendMFTMessage(MFT_MESSAGE_NOTIFY_BEGIN_STREAMING, 0)) {

[Ami GONE FROM CHROMIUM, 2011/12/13 07:24:56]

If you reverse the test (!) you get to:
- drop the braces
- be symmetric w/ the other early-returns above.
- de-indent 3 lines in exchange for indenting 1 line.

[ananta, 2011/12/13 23:29:15]

Done.

+    state_ = DXVAVideoDecodeAccelerator::kNormal;
+    client_->NotifyInitializeDone();
+    return true;
+  }
+  return false;
+}
+
+void DXVAVideoDecodeAccelerator::Decode(
+    const media::BitstreamBuffer& bitstream_buffer) {
+  DCHECK_EQ(message_loop_, MessageLoop::current()->message_loop_proxy());
+
+  if (state_ == DXVAVideoDecodeAccelerator::kUninitialized) {

[Ami GONE FROM CHROMIUM, 2011/12/13 07:24:56]

What about the other states (resetting/flushing)?
You might want to do the ovda_test generification sooner rather than later (it
tests a bunch of teardown scenarios, among other things).

[ananta, 2011/12/13 23:29:15]

Will do that once this change clears up. Fixed this check to just allow decoding
in the normal and stopped state.

+    NOTREACHED() << "ConsumeVideoSample: invalid state";
+    return;
+  }
+
+  HANDLE shared_memory_handle = NULL;
+  if (!::DuplicateHandle(renderer_process_,

[Ami GONE FROM CHROMIUM, 2011/12/13 07:24:56]

Is this really necessary?  I expected the fact that we tell the IPC machinery
that the arg is a shm handle would mean it would get dup'd into this process
automatically:
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/common/gpu/gp...

If this isn't necessary, then I think you can drop process_handle_ from this
class.

[apatrick_chromium, 2011/12/13 19:51:01]

Last time I checked, the IPC system didn't know how to automatically dup shared
memory handles on Windows. I think this is necessary.

[ananta, 2011/12/13 23:29:15]

The shared memory handle is not duplicated into the receiving process by the IPC
machinery.

+                         bitstream_buffer.handle(),
+                         ::GetCurrentProcess(),
+                         &shared_memory_handle,
+                         0,
+                         FALSE,
+                         DUPLICATE_SAME_ACCESS)) {
+    NOTREACHED() << "Failed to open duplicate shared mem handle";
+    return;
+  }
+
+  base::SharedMemory shm(shared_memory_handle, true);
+  if (!shm.Map(bitstream_buffer.size())) {
+    NOTREACHED() << "Failed in SharedMemory::Map()";
+    return;
+  }
+
+  base::Time::Exploded exploded;
+  base::Time::Now().LocalExplode(&exploded);
+  base::win::ScopedComPtr<IMFSample> sample;
+  sample.Attach(CreateInputSample(reinterpret_cast<const uint8*>(shm.memory()),
+                                  bitstream_buffer.size(),
+                                  exploded.second * 10000000,

[Ami GONE FROM CHROMIUM, 2011/12/13 07:24:56]

Wait, what?  This is putting Now() into the decoder???
I assumed that param to CreateInputSample was about the video-stream timestamp,
not the wall-clock time.  What is this about?

[ananta, 2011/12/13 23:29:15]

Removed. I had put that only for debugging i.e. to find out whether the
buffering in the decoder was due to these missing attributes. Turned out to be
wrong.

+                                  400000,

[Ami GONE FROM CHROMIUM, 2011/12/13 07:24:56]

Where does 400k come from?

[ananta, 2011/12/13 23:29:15]

From thin air. :) Removed this parameter.

+                                  input_stream_info_.cbSize,
+                                  input_stream_info_.cbAlignment));
+  if (!sample.get()) {
+    NOTREACHED() << "Failed to create an input sample";
+    return;
+  } else {
+#if !defined(NDEBUG)
+    inputs_before_decode_++;
+    if (!decode_start_time_)
+      decode_start_time_ = ::GetTickCount();
+#endif  // !defined(NDEBUG)
+    SendMFTMessage(MFT_MESSAGE_NOTIFY_START_OF_STREAM, 0);

[Ami GONE FROM CHROMIUM, 2011/12/13 07:24:56]

No error-checking necessary?

[ananta, 2011/12/13 23:29:15]

Done.

+
+    if (FAILED(decoder_->ProcessInput(0, sample.get(), 0))) {
+      NOTREACHED() << "Failed to process input";
+      return;
+    }
+  }
+  if (state_ != DXVAVideoDecodeAccelerator::kEosDrain) {
+    if (!SendMFTMessage(MFT_MESSAGE_NOTIFY_END_OF_STREAM, 0)) {
+      NOTREACHED() << "Failed to send eos message to MFT";
+    } else {
+      state_ = DXVAVideoDecodeAccelerator::kEosDrain;

[Ami GONE FROM CHROMIUM, 2011/12/13 07:24:56]

You're draining the decoder after each sample?  That seems wrong.

[ananta, 2011/12/13 23:29:15]

I am not draining the decoder. The state is marked as eosdrain. The command
being sent to the decoder above is end of stream.

+    }
+  }
+  input_buffer_frame_times_.push_back(bitstream_buffer.id());
+  DoDecode();
+  client_->NotifyEndOfBitstreamBuffer(bitstream_buffer.id());

[Ami GONE FROM CHROMIUM, 2011/12/13 07:24:56]

This is supposed to be called when the buffer has been fully decoded, not when
it's been handed to the decoder.  Is something above here synchronous?  (it's a
problem either way)

[ananta, 2011/12/13 23:29:15]

We already handed the decoder the buffer to be decoded. It makes a copy of the
buffer internally. Unfortunately we need to inform the client that the input has
been processed. (Read flash). If not it stops sending us input packets as it
runs out of free buffers. The MFT decoder does do a bit of buffering before it
gives as a valid decoded sample.

+}
+
+void DXVAVideoDecodeAccelerator::AssignPictureBuffers(
+    const std::vector<media::PictureBuffer>& buffers) {
+  DCHECK_EQ(message_loop_, MessageLoop::current()->message_loop_proxy());
+  // Copy the picture buffers provided by the client to the available list,
+  // and mark these buffers as available for use.
+  for (size_t buffer_index = 0; buffer_index < buffers.size(); ++buffer_index) {
+    DXVAPictureBuffer picture_buffer;

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

Use the ctor instead (but see below and just drop these 3 lines instead).

[ananta, 2011/12/13 23:29:15]

Done.

+    picture_buffer.available = true;
+    picture_buffer.picture_buffer = buffers[buffer_index];
+
+    DCHECK(available_pictures_.find(buffers[buffer_index].id()) ==
+           available_pictures_.end());
+    available_pictures_[buffers[buffer_index].id()] = picture_buffer;

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

A more efficient way is to replace the above 3 lines with:
bool inserted = available_pictures_.insert(std::make_pair(
    buffers[buffer_index].id(), DXVAPictureBuffer(true,
buffers[buffer_index])).second;
DCHECK(inserted);

(more efficient b/c it only does the lookup once instead of twice (once for the
find and once for the operator[]))

[ananta, 2011/12/13 23:29:15]

The find above is just checking whether the picture id already exists in the map
which should not happen. Leaving as is. 

+  }
+  int buffer_index = 0;
+  PendingOutputSamples::iterator sample_index =
+      pending_output_samples_.begin();
+  HRESULT hr = E_FAIL;

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

None of the above 4 lines are used.
I would have expected the cros gcc (4.6) to error out on these unused variables.
 Do you know why it didn't?

[ananta, 2011/12/13 23:29:15]

Dunno why. These statements were left out by mistake while refactoring this
function to move the common code out to ProcessPendingSamples. Removed these
statements.

+
+  ProcessPendingSamples();
+}
+
+void DXVAVideoDecodeAccelerator::ReusePictureBuffer(
+    int32 picture_buffer_id) {
+  DCHECK_EQ(message_loop_, MessageLoop::current()->message_loop_proxy());
+
+  DCHECK(available_pictures_.find(picture_buffer_id) !=

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

FWIW, instead of find() != end(), it's shorter to 
DCHECK_EQ(my_map.count(key), 0U);
(or just DCHECK(!my_map.count(key)); if you're feeling hardcore)

[ananta, 2011/12/13 23:29:15]

Done.

+         available_pictures_.end());
+  available_pictures_[picture_buffer_id].available = true;

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

Since you end up needing this several more times in this function, I'd replace
the above three lines with:
OutputBuffers::iterator it = available_pictures_.find(picture_buffer_id);
RETURN_ON_ERROR(it == available_pictures_.end());
it->available = true;

and then later on use *it or it-> instead of repeating the lookup
available_pictures_[picture_buffer_id] (or even store *it in an explicit
DXVAPictureBuffer*).

[ananta, 2011/12/13 23:29:15]

Done.

+
+  PendingOutputSamples::iterator sample_index =

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

This would be clearer as 
if (pending_output_samples_.empty())
  return;
const PendingSampleInfo& sample_info = *pending_output_samples_.front();

But can't this entire stanza be replaced with a call to ProcessPendingSamples? 
As it stands this code is very similar to that code so it'd be nice to avoid the
duplication.

[ananta, 2011/12/13 23:29:15]

Done.

+      pending_output_samples_.begin();
+
+  if (sample_index != pending_output_samples_.end()) {
+    const PendingSampleInfo& sample_info = *sample_index;
+    CopyOutputSampleDataToPictureBuffer(
+        sample_info.surface,
+        available_pictures_[picture_buffer_id].picture_buffer,
+        sample_info.input_buffer_id);
+    available_pictures_[picture_buffer_id].available = false;
+    pending_output_samples_.erase(sample_index);

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

pop_front if you take my suggestion above (and I'd pop it immediately after
taking the front() above).

[ananta, 2011/12/13 23:29:15]

Ignoring this as I took your above suggestion to call ProcessPendingSamples
instead.

+  }
+}
+
+void DXVAVideoDecodeAccelerator::Flush() {
+  DCHECK_EQ(message_loop_, MessageLoop::current()->message_loop_proxy());
+
+  VLOG(1) << "DXVAVideoDecodeAccelerator::Flush";

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

FWIW s/VLOG/DVLOG/g everywhere in this CL (see chromium-dev discussions started
by brettw a few weeks ago).

+  if (state_ == DXVAVideoDecodeAccelerator::kUninitialized) {
+    NOTREACHED() << "ConsumeVideoSample: invalid state";
+    client_->NotifyFlushDone();

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

It is generally not OK to call PPP_* methods from the implementation of PPB_*
methods (b/c ppapi is not generally reentrant).  Here (and in many places below)
you're effectively relying on the fact that the enclosing message is async (so
the PPB_* call has already returned by the time this is executing).  I *think*
this is OK, but wanted to point it out so you can think about it explicitly if
you hadn't already done so.  (audit all client_-> sites in this file).

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

Shouldn't this be a NotifyError(ILLEGAL_STATE)?
In fact the presence of so much error checking with zero calls to NotifyError is
suspicious; audit the failure paths?
(again reco to use macros for failure-handling to reduce boilerplate).

[ananta, 2011/12/13 23:29:15]

Done.

+    return;
+  }
+
+  if (state_ == DXVAVideoDecodeAccelerator::kStopped) {
+    VLOG(1) << "No data available from the decoder";

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

This doesn't seem to be worthy of a VLOG to me.

[ananta, 2011/12/13 23:29:15]

Rewrote this function. PTAL.

+    client_->NotifyFlushDone();
+    return;
+  }
+
+  state_ = DXVAVideoDecodeAccelerator::kEosDrain;

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

qualification unnecessary (here and elsewhere below).

[ananta, 2011/12/13 23:29:15]

Done.

+  if (!SendMFTMessage(MFT_MESSAGE_COMMAND_DRAIN, 0)) {
+    LOG(WARNING) << "Failed to send drain message";

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

Consistency with other MF failures.

[ananta, 2011/12/13 23:29:15]

This function has been rewritten to make it look better. PTAL.

+    state_ = DXVAVideoDecodeAccelerator::kStopped;
+    client_->NotifyFlushDone();
+    return;
+  }
+
+  // As per MSDN docs after the client sends this message, it calls

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

s/this/the above MFT_MESSAGE_COMMAND_DRAIN/ ?

[ananta, 2011/12/13 23:29:15]

Yes.

+  // IMFTransform::ProcessOutput in a loop, until ProcessOutput returns the
+  // error code MF_E_TRANSFORM_NEED_MORE_INPUT. The DoDecode function sets the
+  // state to DXVAVideoDecodeAccelerator::kStopped when the decoder returns
+  // MF_E_TRANSFORM_NEED_MORE_INPUT.
+  while (state_ != DXVAVideoDecodeAccelerator::kStopped) {
+    DoDecode();

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

I guess DoDecode() really is synchronous.  Are you not worried about blocking
the GPU process for all other operation while this is happening? 
If the decoder is fed a bunch of input (without draining the output) and then
fed a flush, and then outputs are drained (pictures are reused), can the page
scroll at all?

[ananta, 2011/12/13 23:29:15]

Worried a little yes. However the MFT decoder uses a thread pool internally.
Based on debugging I found that processoutput just checks the state of an event
which is set when there is decoded output available. So hopefully this should
not be much of an overhead.

+  }
+  client_->NotifyFlushDone();
+}
+
+void DXVAVideoDecodeAccelerator::Reset() {
+  DCHECK_EQ(message_loop_, MessageLoop::current()->message_loop_proxy());
+
+  VLOG(1) << "DXVAVideoDecodeAccelerator::Reset";
+  if (state_ != kNormal && state_ != kStopped) {
+    NOTREACHED() << "Reset: invalid state";
+    client_->NotifyResetDone();
+    return;
+  }
+
+  state_ = DXVAVideoDecodeAccelerator::kResetting;

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

Whenever you have code like this that checks that state is one of a set number
of errors, else set to the next state, it's nice to be able to DCHECK the state
*before* the assignment to document your intent/expectations.
(here and elsewhere in this CL)

+  if (!SendMFTMessage(MFT_MESSAGE_COMMAND_FLUSH, 0)) {
+    LOG(WARNING) << "DXVAVideoDecodeAccelerator::Flush failed to send message";
+    client_->NotifyResetDone();
+    return;
+  }
+  state_ = DXVAVideoDecodeAccelerator::kNormal;
+  client_->NotifyResetDone();

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

This should be the last call in the method.

[ananta, 2011/12/13 23:29:15]

Done.

+  input_buffer_frame_times_.clear();

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

What about the rest of the pending state?

[ananta, 2011/12/13 23:29:15]

Done.

+}
+
+void DXVAVideoDecodeAccelerator::Destroy() {
+  DCHECK_EQ(message_loop_, MessageLoop::current()->message_loop_proxy());
+
+  OutputBuffers::iterator index;
+  for (index = available_pictures_.begin(); index != available_pictures_.end();
+       ++index) {
+    client_->DismissPictureBuffer(index->second.picture_buffer.id());
+  }
+  available_pictures_.clear();
+  pending_output_samples_.clear();
+  input_buffer_frame_times_.clear();
+}

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

Does nothing need to be done to tear down the decoder?  Destroy() should be the
opposite of Initialize().

+
+bool DXVAVideoDecodeAccelerator::CreateD3DDevManager() {
+  base::win::ScopedComPtr<IDirect3D9Ex> d3d9;
+
+  Direct3DCreate9Ex(D3D_SDK_VERSION, d3d9.Receive());

[apatrick_chromium, 2011/12/13 19:51:01]

If the delay loading of d3d9.dll doesn't work for whatever reason (and the gyp
file looks right), the XP bots will explode when you land this on account of not
being able to bind to this import. Good luck! (I gave up and just
GetProcAddressed it).

+  if (d3d9.get() == NULL) {
+    NOTREACHED() << "Failed to create D3D9";
+    return false;
+  }
+
+  D3DPRESENT_PARAMETERS present_params = {0};
+  present_params.BackBufferWidth = 1;
+  present_params.BackBufferHeight = 1;
+  present_params.BackBufferFormat = D3DFMT_UNKNOWN;
+  present_params.BackBufferCount = 1;
+  present_params.SwapEffect = D3DSWAPEFFECT_DISCARD;
+  present_params.hDeviceWindow = GetShellWindow();

[apatrick_chromium, 2011/12/13 19:51:01]

Question for cpu: what will GetShellWindow() do in a world where the GPU process
is not running on the interactive desktop?

+  present_params.Windowed = TRUE;
+  present_params.Flags = D3DPRESENTFLAG_VIDEO;
+  present_params.FullScreen_RefreshRateInHz = 0;
+  present_params.PresentationInterval = 0;
+
+  HRESULT hr = d3d9->CreateDeviceEx(D3DADAPTER_DEFAULT,
+                                    D3DDEVTYPE_HAL,
+                                    GetShellWindow(),
+                                    D3DCREATE_MIXED_VERTEXPROCESSING,

[apatrick_chromium, 2011/12/13 19:51:01]

I think MIXED won't work on cards that do not support hardware vertex shaders
either. Since you aren't using vertex shaders anyway, you could just use
SOFTWARE. Otherwise, the usual pattern is to ask for HARDWARE and if
CreateDeviceEx fails, ask for SOFTWARE.

+                                    &present_params,
+                                    NULL,
+                                    device_.Receive());
+  if (FAILED(hr)) {
+    NOTREACHED() << "Failed to create D3D Device";
+    return false;
+  }
+
+  hr = DXVA2CreateDirect3DDeviceManager9(&dev_manager_reset_token_,
+                                         device_manager_.Receive());
+  if (FAILED(hr)) {
+    NOTREACHED() << "Couldn't create D3D Device manager";
+    return false;
+  }
+
+  hr = device_manager_->ResetDevice(device_.get(),
+                                    dev_manager_reset_token_);
+  if (FAILED(hr)) {
+    NOTREACHED() << "Failed to set device to device manager";
+    return false;
+  }
+  return true;
+}
+
+
+bool DXVAVideoDecodeAccelerator::InitDecoder() {
+  HRESULT hr = CoCreateInstance(__uuidof(CMSH264DecoderMFT),
+                                NULL,
+                                CLSCTX_INPROC_SERVER,
+                                __uuidof(IMFTransform),
+                                reinterpret_cast<void**>(decoder_.Receive()));
+  if (FAILED(hr) || !decoder_.get()) {
+    NOTREACHED() << "CoCreateInstance failed "
+                 << std::hex << std::showbase << hr;
+    return false;
+  }
+
+  if (!CheckDecoderDxvaSupport())
+    return false;
+  hr = decoder_->ProcessMessage(
+            MFT_MESSAGE_SET_D3D_MANAGER,
+            reinterpret_cast<ULONG_PTR>(device_manager_.get()));
+  if (FAILED(hr)) {
+    NOTREACHED() << "Failed to pass D3D9 device to decoder "
+                 << std::hex << hr;
+    return false;
+  }
+  return SetDecoderMediaTypes();
+}
+
+bool DXVAVideoDecodeAccelerator::CheckDecoderDxvaSupport() {
+  base::win::ScopedComPtr<IMFAttributes> attributes;
+  HRESULT hr = decoder_->GetAttributes(attributes.Receive());
+  if (FAILED(hr)) {
+    NOTREACHED() << "Unlock: Failed to get attributes, hr = "
+                 << std::hex << std::showbase << hr;
+    return false;
+  }
+  UINT32 dxva = 0;
+  hr = attributes->GetUINT32(MF_SA_D3D_AWARE, &dxva);
+  if (FAILED(hr) || !dxva) {
+    NOTREACHED() << "Failed to get DXVA attr. Error:"
+                 << std::hex << std::showbase << hr
+                 << " .This might not be the right decoder.";
+    return false;
+  }
+
+  hr = attributes->SetUINT32(CODECAPI_AVDecVideoAcceleration_H264, TRUE);
+  DCHECK(SUCCEEDED(hr));
+  return true;
+}
+
+bool DXVAVideoDecodeAccelerator::SetDecoderMediaTypes() {
+  MFT_REGISTER_TYPE_INFO* input_types = NULL;
+  MFT_REGISTER_TYPE_INFO* output_types = NULL;
+  uint32 num_inputs = 0;
+  uint32 num_outputs = 0;
+  HRESULT hr = MFTGetInfo(__uuidof(CMSH264DecoderMFT), NULL, &input_types,
+                          &num_inputs, &output_types, &num_outputs, NULL);
+  if (!SetDecoderInputMediaType())
+    return false;
+  return SetDecoderOutputMediaType(MFVideoFormat_NV12);
+}
+
+bool DXVAVideoDecodeAccelerator::SetDecoderInputMediaType() {
+  base::win::ScopedComPtr<IMFMediaType> media_type;
+  HRESULT hr = MFCreateMediaType(media_type.Receive());
+  if (FAILED(hr)) {
+    NOTREACHED() << "Failed to create empty media type object";
+    return false;
+  }
+
+  hr = media_type->SetGUID(MF_MT_MAJOR_TYPE, MFMediaType_Video);
+  if (FAILED(hr)) {
+    NOTREACHED() << "SetGUID for major type failed";
+    return false;
+  }
+
+  hr = media_type->SetGUID(MF_MT_SUBTYPE, MFVideoFormat_H264);
+  if (FAILED(hr)) {
+    NOTREACHED() << "SetGUID for subtype failed";
+    return false;
+  }
+
+  hr = decoder_->SetInputType(0, media_type.get(), 0);  // No flags
+  if (FAILED(hr)) {
+    NOTREACHED() << "Failed to set decoder's input type";
+    return false;
+  }
+  return true;
+}
+
+bool DXVAVideoDecodeAccelerator::SetDecoderOutputMediaType(
+    const GUID& subtype) {
+  DWORD i = 0;
+  base::win::ScopedComPtr<IMFMediaType> out_media_type;
+  bool found = false;

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

unused

[ananta, 2011/12/13 23:29:15]

Done.

+  while (SUCCEEDED(decoder_->GetOutputAvailableType(

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

ISTM this would be more natural as a for-loop
for (DWORD i = 0; SUCCEEDED(decoder_->GetOutputAvailableType(0, i,
out_media_type.Receive()); ++i) { ...

[ananta, 2011/12/13 23:29:15]

Done.

+      0, i, out_media_type.Receive()))) {
+    GUID out_subtype = {0};
+    HRESULT hr = out_media_type->GetGUID(MF_MT_SUBTYPE, &out_subtype);
+    if (FAILED(hr)) {
+      NOTREACHED() << "Failed to GetGUID() on GetOutputAvailableType() "
+                   << i;
+      out_media_type.Release();

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

Unnecessary? (here and elsewhere)

[ananta, 2011/12/13 23:29:15]

We have to release the COM interface here inorder to fetch the next guy.

+      continue;
+    }
+    if (out_subtype == subtype) {
+      hr = decoder_->SetOutputType(0, out_media_type, 0);  // No flags

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

No checking for failures?

[ananta, 2011/12/13 23:29:15]

Done.

+      hr = MFGetAttributeSize(out_media_type, MF_MT_FRAME_SIZE,
+              reinterpret_cast<UINT32*>(&surface_width_),
+              reinterpret_cast<UINT32*>(&surface_height_));

[Ami GONE FROM CHROMIUM, 2011/12/13 07:24:56]

surface_{width,height}_ don't seem to be used anywhere.

[ananta, 2011/12/13 23:29:15]

Done. PTAL at this function. Rewritten to abort on errors.

+      if (FAILED(hr)) {
+        NOTREACHED() << "Failed to SetOutputType to |subtype| or obtain "
+                     << "width/height " << std::hex << hr;
+      }
+      out_media_type.Release();
+      return true;
+    }
+    i++;
+    out_media_type.Release();
+  }
+  return false;
+}
+
+bool DXVAVideoDecodeAccelerator::SendMFTMessage(MFT_MESSAGE_TYPE msg,
+                                                int32 param) {
+  HRESULT hr = decoder_->ProcessMessage(msg, param);
+  return SUCCEEDED(hr);
+}
+
+// Gets the minimum buffer sizes for input and output samples.
+// The MFT will not allocate buffer for input nor output, so we have
+// to do it ourselves and make sure they're the correct size.
+// Exception is when dxva is enabled, the decoder will allocate output.

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

What's the alternative?  Can dxva be "disabled" and this class still do useful
work?  Apparently not, given the CHECK 0x107u below.

[ananta, 2011/12/13 23:29:15]

Unfortunate cut paste. Rephrased the comment.

+bool DXVAVideoDecodeAccelerator::GetStreamsInfoAndBufferReqs() {
+  HRESULT hr = decoder_->GetInputStreamInfo(0, &input_stream_info_);
+  if (FAILED(hr)) {
+    LOG(ERROR) << "Failed to get input stream info";
+    return false;
+  }
+  VLOG(1) << "Input stream info: ";

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

Between the VLOGs and the error-handling it's really hard to see that this
33-line function makes only 2 function calls that aren't
error-checking/vlogging.  I hope you can improve that ratio.

(this is a generic comment about this file)

[ananta, 2011/12/13 23:29:15]

Fixed this function to get the input and output streams at the beginning. Will
take a look at the rest of the functions.

+  VLOG(1) << "Max latency: " << input_stream_info_.hnsMaxLatency;
+  // There should be three flags, one for requiring a whole frame be in a
+  // single sample, one for requiring there be one buffer only in a single
+  // sample, and one that specifies a fixed sample size. (as in cbSize)
+  VLOG(1) << "Flags: "
+          << std::hex << std::showbase << input_stream_info_.dwFlags;

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

Unnecessary since the CHECK_EQ in the next line will tell us if it isn't 0x7u
(and tell us what it is).

+  CHECK_EQ(input_stream_info_.dwFlags, 0x7u);
+  VLOG(1) << "Min buffer size: " << input_stream_info_.cbSize;
+  VLOG(1) << "Max lookahead: " << input_stream_info_.cbMaxLookahead;
+  VLOG(1) << "Alignment: " << input_stream_info_.cbAlignment;
+
+  hr = decoder_->GetOutputStreamInfo(0, &output_stream_info_);
+  if (FAILED(hr)) {
+    LOG(ERROR) << "Failed to get output stream info";
+    return false;
+  }
+  VLOG(1) << "Output stream info: ";
+  // The flags here should be the same and mean the same thing, except when
+  // DXVA is enabled, there is an extra 0x100 flag meaning decoder will
+  // allocate its own sample.
+  VLOG(1) << "Flags: "
+          << std::hex << std::showbase << output_stream_info_.dwFlags;
+  CHECK_EQ(output_stream_info_.dwFlags, 0x107u);
+  VLOG(1) << "Min buffer size: " << output_stream_info_.cbSize;
+  VLOG(1) << "Alignment: " << output_stream_info_.cbAlignment;
+  return true;
+}
+
+bool DXVAVideoDecodeAccelerator::DoDecode() {
+  if (state_ != kNormal && state_ != kEosDrain) {
+    NOTREACHED() << "DoDecode: not in normal or drain state";
+    return false;
+  }
+  base::win::ScopedComPtr<IMFSample> output_sample;
+  MFT_OUTPUT_DATA_BUFFER output_data_buffer;
+  DWORD status = 0;
+
+  memset(&output_data_buffer, 0, sizeof(output_data_buffer));

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

This is incorrect in general; prefer the = {0}; style for zero-initialization.

[ananta, 2011/12/13 23:29:15]

Done.

+  output_data_buffer.pSample = output_sample;
+  status = 0;

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

redundant.

[ananta, 2011/12/13 23:29:15]

Done.

+
+  HRESULT hr = decoder_->ProcessOutput(0,  // No flags
+                                       1,  // # of out streams to pull from
+                                       &output_data_buffer,
+                                       &status);
+  IMFCollection* events = output_data_buffer.pEvents;
+  if (events != NULL) {
+    VLOG(1) << "Got events from ProcessOuput, but discarding";
+    events->Release();
+  }
+  if (FAILED(hr)) {
+    if (hr == MF_E_TRANSFORM_STREAM_CHANGE) {
+      hr = SetDecoderOutputMediaType(MFVideoFormat_NV12);

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

I don't understand this - the stream changed nothing about the change is
reflected in this call?

[ananta, 2011/12/13 23:29:15]

This is how MSDN mentions that we should handle the format change :(.

+      if (SUCCEEDED(hr)) {
+        return true;
+      } else {
+        NOTREACHED() << "Failed to set decoder output media type";
+        return false;
+      }
+    } else if (hr == MF_E_TRANSFORM_NEED_MORE_INPUT) {
+      // No more output from the decoder. Notify EOS and stop playback.

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

DCHECK that we were waiting for this?

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

This doesn't actually Notify or stop, it just signals that that should happen to
the caller.

[ananta, 2011/12/13 23:29:15]

Could you clarify this?.

+      state_ = DXVAVideoDecodeAccelerator::kStopped;
+      return false;
+    } else {
+      NOTREACHED() << "Unhandled error in DoDecode()";
+      state_ = DXVAVideoDecodeAccelerator::kStopped;
+      return false;
+    }
+  }
+
+#if !defined(NDEBUG)
+  VLOG(1) << "Number of input packets before successful decode: "
+          << inputs_before_decode_;
+  inputs_before_decode_ = 0;
+  uint32 end_decode = GetTickCount();
+  VLOG(1) << "Total time for decode: "
+          << end_decode - decode_start_time_;
+  decode_start_time_ = 0;
+#endif  // !defined(NDEBUG)
+  if (!ProcessOutputSample(output_data_buffer.pSample)) {
+    state_ = DXVAVideoDecodeAccelerator::kStopped;

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

More like error than stopped.  NotifyError?

[ananta, 2011/12/13 23:29:15]

Done.

+    return false;
+  }
+
+  input_buffer_frame_times_.clear();
+  state_ = DXVAVideoDecodeAccelerator::kNormal;

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

Why overwrite the state if we were stopping?

[ananta, 2011/12/13 23:29:15]

When we arrive here we are not stopping as we have output to process.

+  return true;

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

Return value is ignored at all call-sites.

[ananta, 2011/12/13 23:29:15]

Done.

+}
+
+bool DXVAVideoDecodeAccelerator::ProcessOutputSample(IMFSample* sample) {
+  if (!sample) {
+    NOTREACHED() << "ProcessOutput succeeded, but did not get a sample back";
+    return false;
+  }
+  base::win::ScopedComPtr<IMFSample> output_sample;
+  output_sample.Attach(sample);
+
+  if (!input_buffer_frame_times_.size()) {
+    VLOG(1) << "In input buffers left to process output";

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

what?

[ananta, 2011/12/13 23:29:15]

Replaced with No. :(

+    return false;
+  }
+  int32 input_buffer_id =
+      input_buffer_frame_times_[input_buffer_frame_times_.size() - 1];

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

I extra don't understand input_buffer_frame_times_ considering all the decode is
synchronous and there doesn't seem to be queuing of inputs; why can't you just
remember the "current" input buffer id?

+  input_buffer_frame_times_.clear();
+
+  base::win::ScopedComPtr<IMFMediaBuffer> output_buffer;
+  HRESULT hr = sample->GetBufferByIndex(0, output_buffer.Receive());
+  if (FAILED(hr)) {
+    NOTREACHED() << "Failed to get buffer from sample";
+    return false;
+  }
+  base::win::ScopedComPtr<IDirect3DSurface9> surface;
+  hr = MFGetService(output_buffer, MR_BUFFER_SERVICE,
+                    IID_PPV_ARGS(surface.Receive()));
+  if (FAILED(hr)) {
+    NOTREACHED() << "Failed to get surface from buffer";
+    return false;
+  }
+
+  D3DSURFACE_DESC surface_desc;
+  hr = surface->GetDesc(&surface_desc);
+  if (FAILED(hr)) {
+    NOTREACHED() << "Failed to get surface description";
+    return false;
+  }
+
+#if !defined(NDEBUG)
+  DWORD start_surface_time = GetTickCount();
+#endif  // !defined(NDEBUG)
+
+  // TODO(ananta)

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

TODOs are usually marked w/ an email address.  s/ananta/iyengar/?

[Ami GONE FROM CHROMIUM, 2011/12/13 20:24:38]

oops, I see that's your @chromium address.  Disregard.

+  // The code below may not be necessary once we have an ANGLE extension which
+  // allows us to pass the Direct 3D surface directly for rendering.
+
+  // The decoded bits in the source direct 3d surface are in the YUV
+  // format. Angle does not support that. As a workaround we create an
+  // offscreen surface in the RGB format and copy the source surface
+  // to this surface.
+  base::win::ScopedComPtr<IDirect3DSurface9> dest_surface;
+  hr = device_->CreateOffscreenPlainSurface(surface_desc.Width,
+                                            surface_desc.Height,
+                                            D3DFMT_A8R8G8B8,
+                                            D3DPOOL_DEFAULT,
+                                            dest_surface.Receive(),
+                                            NULL);
+  if (FAILED(hr)) {
+    NOTREACHED() << "Failed to create offscreen surface";
+    return false;
+  }
+
+  hr = D3DXLoadSurfaceFromSurface(dest_surface, NULL, NULL, surface, NULL,
+                                  NULL, 0, 0);
+  if (FAILED(hr)) {
+    NOTREACHED() << "Failed to copy source surface to dest.";
+    return false;
+  }
+
+#if !defined(NDEBUG)
+  DWORD end_surface_time = GetTickCount();
+  VLOG(1) << "Time to create and copy new surface is "
+          << end_surface_time - start_surface_time;
+#endif  // !defined(NDEBUG)
+
+  PendingSampleInfo sample_info;

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

Give PSI a ctor and use it here?
In fact can replace l.721-725 with
pending_output_samples_.push_back(PendingSampleInfo(input_buffer_id,
dest_surface);

[ananta, 2011/12/13 23:29:15]

Done.

+
+  sample_info.input_buffer_id = input_buffer_id;
+  sample_info.surface = dest_surface;
+  pending_output_samples_.push_back(sample_info);
+
+  // If we have available picture buffers to copy the output data then use the
+  // first one and then flag it as not being available for use.
+  if (available_pictures_.size()) {
+    ProcessPendingSamples();
+    return true;
+  }
+
+  if (pictures_requested_) {
+    VLOG(1) << "Waiting for picture slots from the client.";
+    return true;
+  }
+  // Go ahead and request picture buffers.
+  client_->ProvidePictureBuffers(
+      kNumPictureBuffers, gfx::Size(surface_desc.Width, surface_desc.Height));
+  pictures_requested_ = true;

[cpu_(ooo_6.6-7.5), 2011/12/13 19:28:57]

so pictures_requested_ does not become false again?

[ananta, 2011/12/13 23:29:15]

We only do this upfront. After that we just rotate among the set the client
provided us.

+  return true;
+}
+
+bool DXVAVideoDecodeAccelerator::CopyOutputSampleDataToPictureBuffer(
+    IDirect3DSurface9* dest_surface, media::PictureBuffer picture_buffer,
+    int32 input_buffer_id) {
+  DCHECK(dest_surface);
+
+  // Get the currently loaded bitmap from the DC.
+  HDC hdc = NULL;
+

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

drop \n

[ananta, 2011/12/13 23:29:15]

Done.

+  HRESULT hr = dest_surface->GetDC(&hdc);
+  if (FAILED(hr)) {
+    NOTREACHED() << "Failed to get HDC for dest offscreen surface";
+    return false;
+  }
+
+  HBITMAP bitmap =
+      reinterpret_cast<HBITMAP>(GetCurrentObject(hdc, OBJ_BITMAP));
+  if (!bitmap) {
+    NOTREACHED() << "Failed to get bitmap from DC";
+    return false;
+  }
+  // TODO(ananta)

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

ditto

[ananta, 2011/12/13 23:29:15]

Done.

+  // The code below may not be necessary once we have an ANGLE extension which
+  // allows us to pass the Direct 3D surface directly for rendering.
+
+  // The Device dependent bitmap is upside down for OpenGL. We convert the
+  // bitmap to a DIB and render it on the texture instead.
+  BITMAP bitmap_basic_info = {0};
+  GetObject(bitmap, sizeof(BITMAP), &bitmap_basic_info);

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

failure handling

[ananta, 2011/12/13 23:29:15]

Done.

+
+  BITMAPINFO bitmap_info = {0};
+  bitmap_info.bmiHeader.biSize = sizeof(BITMAPINFOHEADER);
+  bitmap_info.bmiHeader.biWidth = bitmap_basic_info.bmWidth;
+  bitmap_info.bmiHeader.biHeight = bitmap_basic_info.bmHeight;
+  bitmap_info.bmiHeader.biPlanes = 1;
+  bitmap_info.bmiHeader.biBitCount = bitmap_basic_info.bmBitsPixel;
+  bitmap_info.bmiHeader.biCompression = BI_RGB;
+  bitmap_info.bmiHeader.biSizeImage = 0;
+  bitmap_info.bmiHeader.biClrUsed = 0;
+
+  int ret = GetDIBits(hdc, bitmap, 0, 0, NULL, &bitmap_info, DIB_RGB_COLORS);

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

failure handling

[ananta, 2011/12/13 23:29:15]

Done.

+  if (bitmap_info.bmiHeader.biSizeImage <= 0) {
+    NOTREACHED() << "Failed to read bitmap size";
+    return false;
+  }
+  scoped_ptr<char> bits(new char[bitmap_info.bmiHeader.biSizeImage]);

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

scoped_array

[ananta, 2011/12/13 23:29:15]

Done.

+  ret = GetDIBits(hdc, bitmap, 0, bitmap_basic_info.bmHeight, bits.get(),
+                  &bitmap_info, DIB_RGB_COLORS);
+  DCHECK_GT(ret, 0);

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

You seem to have given up on error handling from here down ;)

[ananta, 2011/12/13 23:29:15]

Done.

+
+  D3DSURFACE_DESC surface_desc;
+  hr = dest_surface->GetDesc(&surface_desc);
+  DCHECK(SUCCEEDED(hr));
+
+  glBindTexture(GL_TEXTURE_2D, picture_buffer.texture_id());
+  glTexImage2D(GL_TEXTURE_2D, 0, GL_BGRA_EXT, surface_desc.Width,
+               surface_desc.Height, 0, GL_BGRA_EXT, GL_UNSIGNED_BYTE,
+               reinterpret_cast<GLvoid*>(bits.get()));
+  glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
+  glBindTexture(GL_TEXTURE_2D, 0);
+  dest_surface->ReleaseDC(hdc);
+
+  media::Picture output_picture(picture_buffer.id(), input_buffer_id);
+  client_->PictureReady(output_picture);
+  return true;
+}
+
+void DXVAVideoDecodeAccelerator::ProcessPendingSamples() {
+  PendingOutputSamples::iterator sample_index =
+      pending_output_samples_.begin();
+  HRESULT hr = E_FAIL;

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

Unused.

[ananta, 2011/12/13 23:29:15]

Done.

+
+  while (sample_index != pending_output_samples_.end()) {

[Ami GONE FROM CHROMIUM, 2011/12/13 16:44:06]

This while smells like a for, but more than that the inner for is going to
re-iterate the unavailable pictures for each pending output sample, which seems
suboptimal.  It would be much better to iterate over available_pictures_ once,
front()/pop_front()'ing pending_output_samples_ each time you see an available
picture, and returning when out of pending_output_samples_.

[ananta, 2011/12/13 23:29:15]

Done.

+    const PendingSampleInfo& sample_info = *sample_index;
+    OutputBuffers::iterator index;
+    for (index = available_pictures_.begin();
+         index != available_pictures_.end();
+         ++index) {
+      if (index->second.available) {
+        CopyOutputSampleDataToPictureBuffer(sample_info.surface,
+                                            index->second.picture_buffer,
+                                            sample_info.input_buffer_id);
+        index->second.available = false;
+        sample_index = pending_output_samples_.erase(sample_index);
+        break;
+      }
+    }
+    if (index == available_pictures_.end()) {
+      VLOG(1) << "No available picture slots for output";
+      break;
+    }
+  }
+}
+