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.h

Index: content/common/gpu/media/dxva_video_decode_accelerator.h
===================================================================
--- content/common/gpu/media/dxva_video_decode_accelerator.h	(revision 0)
+++ content/common/gpu/media/dxva_video_decode_accelerator.h	(revision 0)
@@ -0,0 +1,133 @@
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.

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

FYI the CL description describes omx_v_d_a_test as being a browsertest, but it's
not - it's a standalone gtest target.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CONTENT_COMMON_GPU_MEDIA_DVX_VIDEO_DECODE_ACCELERATOR_H_

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

s/DVX/DXVA/
(here and in the other 2 related lines)

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

Done.

+#define CONTENT_COMMON_GPU_MEDIA_DVX_VIDEO_DECODE_ACCELERATOR_H_
+
+#include <d3d9.h>
+#include <dxva2api.h>
+#include <map>
+#include <mfidl.h>
+#include <vector>
+
+#include "base/message_loop_proxy.h"
+#include "base/win/scoped_comptr.h"
+#include "media/video/video_decode_accelerator.h"
+#include "third_party/angle/include/EGL/egl.h"
+#include "third_party/angle/include/EGL/eglext.h"
+
+interface IMFSample;
+interface IDirect3DSurface9;
+
+// Class to provide a DXVA 2.0 based accelerator behind VideoDecodeAccelerator

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

Is this really DXVA2.0 or is it MF?  I know *nothing* about either of these APIs
but ISTM that most/all of your platform calls are MF.

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

Fixed the comment. Thanks for pointing this out.

[Ami GONE FROM CHROMIUM, 2011/12/16 07:38:54]

So you're saying this really is DXVA2.0-specific, not just MF?

+// interface.
+// This class lives on a single thread and DCHECKs that it is never accessed
+// from any other.
+class DXVAVideoDecodeAccelerator : public media::VideoDecodeAccelerator {
+ public:
+  typedef enum {

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

FWIW this is C++; you can say
enum State {
 ...
};
which seems much more common in chromium code.

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

Done.

+    kUninitialized,   // un-initialized.
+    kNormal,          // normal playing state.
+    kResetting,       // upon received Reset(), before ResetDone()
+    kEosDrain,        // upon input EOS received.
+    kStopped,         // upon output EOS received.
+  } State;
+
+  // Does not take ownership of |client| which must outlive |*this|.
+  explicit DXVAVideoDecodeAccelerator(

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

explicit unneeded (unless you drop renderer_process as suggested in another
comment).

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

Done.

+      media::VideoDecodeAccelerator::Client* client,
+      base::ProcessHandle renderer_process);
+  virtual ~DXVAVideoDecodeAccelerator();
+
+  // media::VideoDecodeAccelerator implementation.
+  bool Initialize(Profile profile) OVERRIDE;

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

OVERRIDE requires 'virtual' (or else the clang bots explode)

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

Done.

+  void Decode(const media::BitstreamBuffer& bitstream_buffer) OVERRIDE;
+  virtual void AssignPictureBuffers(
+      const std::vector<media::PictureBuffer>& buffers) OVERRIDE;
+  void ReusePictureBuffer(int32 picture_buffer_id) OVERRIDE;
+  void Flush() OVERRIDE;
+  void Reset() OVERRIDE;
+  void Destroy() OVERRIDE;
+
+ private:
+  bool CreateD3DDevManager();

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

There's a real dearth of commentary here.
I think most/all of these have a single call-site and you extracted them b/c
they look big with all the error-handling.
Hopefully my suggestion about macrofying error handling (see .cc file) will make
these inline-able at their (unique) call-sites.  Otherwise please provide some
commentary for what these are about.

+  bool InitDecoder();
+  bool CheckDecoderDxvaSupport();
+  bool SetDecoderMediaTypes();
+  bool SetDecoderInputMediaType();
+  bool SetDecoderOutputMediaType(const GUID& subtype);
+  bool SendMFTMessage(MFT_MESSAGE_TYPE msg, int32 param);
+  bool GetStreamsInfoAndBufferReqs();
+  bool DoDecode();
+  bool ProcessOutputSample(IMFSample* sample);
+  bool CopyOutputSampleDataToPictureBuffer(IDirect3DSurface9* dest_surface,
+                                           media::PictureBuffer buffer,
+                                           int32 input_buffer_id);
+  void ProcessPendingSamples();
+
+ private:

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

You already said private: at l.53 above.

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

I added this because this section has member variables. Removed now.

+   base::MessageLoopProxy* message_loop_;

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

I think this is only used to assert being on the correct thread, in which case
base::NonThreadSafe might be more idiomatic.

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

Done.

+  // To expose client callbacks from VideoDecodeAccelerator.
+  // NOTE: all calls to this object *MUST* be executed in message_loop_.

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

Can drop comment this if the class becomes NonThreadSafe.

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

Done.

+  media::VideoDecodeAccelerator::Client* client_;
+
+  base::win::ScopedComPtr<IDirect3DDevice9Ex> device_;
+  base::win::ScopedComPtr<IDirect3DDeviceManager9> device_manager_;
+  base::win::ScopedComPtr<IMFTransform> decoder_;
+
+  UINT32 surface_width_;
+  UINT32 surface_height_;
+
+  // Current state of the decoder.
+  State state_;
+
+  MFT_INPUT_STREAM_INFO input_stream_info_;
+  MFT_OUTPUT_STREAM_INFO output_stream_info_;
+
+  // Contains information about a decoded sample.
+  struct PendingSampleInfo {
+    base::win::ScopedComPtr<IDirect3DSurface9> surface;
+    int32 input_buffer_id;
+  };
+
+  typedef std::vector<PendingSampleInfo>

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

Single line

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

Done.

+      PendingOutputSamples;
+
+  // List of output samples for rendering.
+  PendingOutputSamples pending_output_samples_;
+
+  // Maintains information about a DXVA picture buffer, i.e. whether it is
+  // available for rendering, the texture information, etc.
+  struct DXVAPictureBuffer {
+    DXVAPictureBuffer()
+        : picture_buffer(0, gfx::Size(), 0) {

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

ctor definition belongs out-of-line in the .cc file.

+      available = false;

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

Why not initializer-list this one too?

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

Done.

+    }
+    bool available;
+    media::PictureBuffer picture_buffer;
+  };
+
+  typedef std::map<int32, DXVAPictureBuffer> OutputBuffers;

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

doco the key.

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

Done.

+  OutputBuffers available_pictures_;

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

This is a poor name considering some of its elements are not in fact available.

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

Changed to output_picture_buffers_

+
+  // Set to true if we requested picture slots from the plugin.

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

s/plugin/client/

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

s/slots/buffers/

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

Done.

+  bool pictures_requested_;
+
+  // Contains the list of input buffer ids which map to frame time from the

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

I don't understand this comment (or how the vector is used in the .cc file).
I *think* this wants to be:
// The i'th entry in this vector is the input buffer's ID currently occupying
input slot i of the decoder.
But I'm having a hard time wrapping my head around how the code in the .cc file
is a correct implementation of this.  Perhaps you're doing something completely
different?

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

Vector not needed as you mentioned in the cc file. We just maintain the last
input buffer id.

+  // plugin.
+  std::vector<int32> input_buffer_frame_times_;
+
+  // Handle to the renderer process.
+  base::ProcessHandle renderer_process_;
+
+  uint32 dev_manager_reset_token_;

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

Can this not be a local variable in the (only) function it's used?

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

Please see Al Patrick's comment in the initial snapshot. It looks the decoder
holds onto the pointer and tries to update it when the device info changes :(
thus corrupting the stack.

+
+#if !defined(NDEBUG)
+  uint32 decode_start_time_;

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

This smells like something that must have a generic, efficient, implementation,
possible in a macro involving the word TRACE:
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/base/debug/trace_even...
Does none of that work for you?

(using trace_event.h buys you all kinds of snazzy visualization and analysis for
free, plus avoids this sort of home-brew)

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

Will take a look at this.

[ananta, 2011/12/14 23:56:52]

Fixed. We now use the ETW trace macros for Windows. TRACE_EVENT**

+  int inputs_before_decode_;
+#endif  // !defined(NDEBUG)
+};

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

DISALLOW_?

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

Done.

+
+#endif  // CONTENT_COMMON_GPU_MEDIA_DVX_VIDEO_DECODE_ACCELERATOR_H_
+