Implement media::VideoDecodeAccelerator on Mac

content/common/gpu/media/mac_video_decode_accelerator.h

+// 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.
+
+#ifndef CONTENT_COMMON_GPU_MEDIA_MAC_VIDEO_DECODE_ACCELERATOR_H_
+#define CONTENT_COMMON_GPU_MEDIA_MAC_VIDEO_DECODE_ACCELERATOR_H_
+
+#include <CoreVideo/CoreVideo.h>
+#include <map>
+#include <list>
+
+#include "base/mac/scoped_cftyperef.h"
+#include "base/memory/ref_counted.h"
+#include "media/video/video_decode_accelerator.h"
+
+namespace base {
+class RefCountedBytes;
+}
+namespace gfx {
+class GLContext;
+class VideoDecodeAccelerationSupport;
+}
+
+class GpuCommandBufferStub;
+
+class MacVideoDecodeAccelerator : public media::VideoDecodeAccelerator {

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

Impl has a refreshing lack of mention of threads.  If everything in this class
is really running in a single thread (yay!) then can you make this NotThreadSafe
and assert same-threadedness in all the entry points (public methods &
base::Bind()'d methods not posted to ML::current())?
Otherwise please doco (& assert) which functions run on which threads.

[sail, 2012/05/23 23:02:29]

Done.

Ahh, sorry, forgot to comment on your previous comment about this issue.

The class is single threaded. I changed the class to derive from
base::NonThreadSafe.

+ public:
+  // Does not take ownership of |client| which must outlive |*this|.
+  MacVideoDecodeAccelerator(media::VideoDecodeAccelerator::Client* client);
+
+  // Set the OpenGL context to use.
+  void SetGLContext(void* gl_context);
+
+  // Set extra data required to initialize the H.264 video decoder.
+  // TODO(sail): Move this into Initialize.
+  bool SetConfigInfo(uint32_t frame_width,
+                     uint32_t frame_height,
+                     const std::vector<uint8_t>& avc_data);
+
+  // media::VideoDecodeAccelerator implementation.
+  virtual bool Initialize(media::VideoCodecProfile profile) OVERRIDE;
+  virtual void Decode(const media::BitstreamBuffer& bitstream_buffer) OVERRIDE;
+  virtual void AssignPictureBuffers(
+      const std::vector<media::PictureBuffer>& buffers) OVERRIDE;
+  virtual void ReusePictureBuffer(int32 picture_buffer_id) OVERRIDE;
+  virtual void Flush() OVERRIDE;
+  virtual void Reset() OVERRIDE;
+  virtual void Destroy() OVERRIDE;
+
+ private:
+  virtual ~MacVideoDecodeAccelerator();
+
+  // Callback for a completed frame.
+  void OnFrameReady(int32 bitstream_buffer_id,
+                    scoped_refptr<base::RefCountedBytes> bytes,
+                    CVImageBufferRef image,
+                    int status);
+
+  // Sends available decoded frames to the client.
+  void SendImages();
+
+  // Calls the client's flush completed callback.
+  void NotifyFlushDone();
+
+  // Calls the client's reset completed callback.
+  void NotifyResetDone();
+
+  // To expose client callbacks from VideoDecodeAccelerator.
+  Client* client_;
+  // C++ wrapper around the Mac VDA API.
+  scoped_refptr<gfx::VideoDecodeAccelerationSupport> vda_;

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

Does VDAS need to be ref-counted (as opposed to not, and stored in a
scoped_ptr<>)?

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

Ouch; the name "vda_" makes me think of the interface *this* class implements,
not a support class.  Rename to vda_support_?

[sail, 2012/05/23 23:02:29]

Done.

[sail, 2012/05/23 23:02:29]

Yea, this needs to be refcounted. It's referenced by the Mac API so it can
potentially live on a little longer than this class.

+  // Picture buffers that are available for use by the decoder to draw decoded
+  // video frames on.
+  std::list<media::PictureBuffer> available_pictures_;
+
+  // Maps ids to picture buffers that are in use by the client.
+  struct PendingPictureInfo {

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

s/pending/used/g (here and below) to parallel "available"_pictures_?

[sail, 2012/05/23 23:02:29]

Done.

+    PendingPictureInfo(media::PictureBuffer pic);
+    ~PendingPictureInfo();
+    media::PictureBuffer picture_buffer;
+    base::mac::ScopedCFTypeRef<CVImageBufferRef> image;
+  };
+  std::map<int32, PendingPictureInfo> pending_pictures_;
+
+  // List of decoded images waiting to be sent to the client.
+  struct DecodedImageInfo {
+    DecodedImageInfo();

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

unnecessary?

[sail, 2012/05/23 23:02:29]

Because of DecodedImageInfo::image the compiler complains that this is a complex
struct and needs an out-of-line constructor and destructor.

+    ~DecodedImageInfo();
+    base::mac::ScopedCFTypeRef<CVImageBufferRef> image;
+    int32 bitstream_buffer_id;
+  };
+  std::list<DecodedImageInfo> decoded_images_;
+
+  // The context to use to perform OpenGL operations.
+  CGLContextObj cgl_context_;
+
+  // The number of bytes used to store the frame buffer size.
+  size_t nalu_len_field_size_;
+
+  // Width of a video frame.
+  uint32_t frame_width_;

[Ami GONE FROM CHROMIUM, 2012/05/17 21:44:47]

unsigned is the devil.
In this case use gfx::Size for the dimensions and avoid the argument ;)

[sail, 2012/05/23 23:02:29]

Done.

+  // Height of a video frame.
+  uint32_t frame_height_;
+  // Flag to check if pictures have been requested from the client.
+  bool did_request_pictures_;
+};
+
+#endif  // CONTENT_COMMON_GPU_MEDIA_VIDEO_DECODE_ACCELERATOR_MAC_H_