Implement actually decoding frames in VTVideoDecodeAccelerator.

content/common/gpu/media/vt_video_decode_accelerator.h

 // Copyright 2014 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_VT_VIDEO_DECODE_ACCELERATOR_H_
 #define CONTENT_COMMON_GPU_MEDIA_VT_VIDEO_DECODE_ACCELERATOR_H_
 
+#include <map>
+#include <queue>
+
 #include "base/basictypes.h"
 #include "base/mac/scoped_cftyperef.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"

[Pawel Osciak, 2014/07/18 01:22:13]

I don't see any usage of scoped_ptr in this class.
There is however scoped_refptr, for which the correct header I believe is
ref_counted.h

[sandersd (OOO until July 31), 2014/07/18 21:50:15]

Done.

 #include "base/memory/weak_ptr.h"
 #include "base/message_loop/message_loop.h"
+#include "base/synchronization/lock.h"

[Pawel Osciak, 2014/07/18 01:22:12]

Unused?

[sandersd (OOO until July 31), 2014/07/18 21:50:15]

Done.

 #include "base/threading/thread.h"
 #include "content/common/gpu/media/vt.h"
 #include "media/filters/h264_parser.h"
 #include "media/video/video_decode_accelerator.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gl/gl_context_cgl.h"
 
 namespace base {
 class SingleThreadTaskRunner;
 }  // namespace base
 
 namespace content {
 
 // VideoToolbox.framework implementation of the VideoDecodeAccelerator
 // interface for Mac OS X (currently limited to 10.9+).
 class VTVideoDecodeAccelerator
     : public media::VideoDecodeAccelerator,
       public base::NonThreadSafe {
  public:
   explicit VTVideoDecodeAccelerator(CGLContextObj cgl_context);
   virtual ~VTVideoDecodeAccelerator();
 
   // VideoDecodeAccelerator implementation.
   virtual bool Initialize(
       media::VideoCodecProfile profile,
       Client* client) OVERRIDE;
   virtual void Decode(const media::BitstreamBuffer& bitstream) OVERRIDE;
   virtual void AssignPictureBuffers(
       const std::vector<media::PictureBuffer>& pictures) OVERRIDE;
   virtual void ReusePictureBuffer(int32_t picture_id) OVERRIDE;

[Pawel Osciak, 2014/07/18 01:22:13]

Need to include stdint.h for int32_t.

[sandersd (OOO until July 31), 2014/07/18 21:50:14]

Done.

   virtual void Flush() OVERRIDE;
   virtual void Reset() OVERRIDE;
   virtual void Destroy() OVERRIDE;
   virtual bool CanDecodeOnIOThread() OVERRIDE;
 
-  // Called by VideoToolbox when a frame is decoded.
+  // Called by OutputThunk when VideoToolbox finishes decoding a frame.
   void Output(
       int32_t bitstream_id,
       OSStatus status,
-      VTDecodeInfoFlags info_flags,
       CVImageBufferRef image_buffer);
 
  private:
-  // Configure a VideoToolbox decompression session from parameter set NALUs.
+  struct DecodedFrame {
+    DecodedFrame(uint32_t bitstream_id, CVImageBufferRef image_buffer);
+    ~DecodedFrame();
+
+    int32_t bitstream_id;
+    base::ScopedCFTypeRef<CVImageBufferRef> image_buffer;
+  };
+
+  // Methods for interacting with VideoToolbox. Run on |decoded_thread_|.
   void ConfigureDecoder(

[Pawel Osciak, 2014/07/18 01:22:13]

s/ConfigureDecoder/ConfigureDecoderTask/ ?

[sandersd (OOO until July 31), 2014/07/18 21:50:14]

ConfigureDecoder is a utility method that is only called directly.

       const std::vector<const uint8_t*>& nalu_data_ptrs,
       const std::vector<size_t>& nalu_data_sizes);
-
-  // Decode a frame of bitstream.
   void DecodeTask(const media::BitstreamBuffer);
 
+  // Methods for interacting with |client_|. Run on |gpu_task_runner_|.
+  void OutputTask(DecodedFrame frame);
+  void SizeChangedTask(gfx::Size coded_size);
+  void SendPictures();

[Pawel Osciak, 2014/07/18 01:22:12]

Task?

[sandersd (OOO until July 31), 2014/07/18 21:50:14]

This one I'm not sure of. It's usually called as a regular method, but it one
case it's posted as a task to work around an API limitation. It's not
intrinsically a task though.

+
+  //
+  // GPU thread state.
+  //
   CGLContextObj cgl_context_;
   media::VideoDecodeAccelerator::Client* client_;
-  base::Thread decoder_thread_;
+  gfx::Size texture_size_;

[Pawel Osciak, 2014/07/18 01:22:12]

Shouldn't this be the same as coded_size_ ?

[sandersd (OOO until July 31), 2014/07/18 21:50:15]

It's nearly the same, but one is owned by the decoder thread and one is owned by
the GPU thread.

+  // Texture IDs of pictures.
+  std::map<int32_t, int32_t> texture_ids_;

[Pawel Osciak, 2014/07/18 01:22:12]

texture ids are uint32

[sandersd (OOO until July 31), 2014/07/18 21:50:14]

Done.

+  // Pictures ready to be rendered to.
+  std::queue<int32_t> picture_ids_;

[Pawel Osciak, 2014/07/18 01:22:13]

s/picture_ids_/available_picture_ids_/ ?

[sandersd (OOO until July 31), 2014/07/18 21:50:14]

Done.

+  // Decoded frames ready to render.
+  std::queue<DecodedFrame> decoded_frames_;
+  // Image buffers retained while they are bound to textures.
+  std::map<int32_t, base::ScopedCFTypeRef<CVImageBufferRef>> picture_bindings_;

[Pawel Osciak, 2014/07/18 01:22:12]

Is the first parameter a texture id? -> uint32.

Also, how about having texture id, picture id and
ScopedCFTypeRef<CVImageBufferRef> in one struct, instead of two maps? Looks like
both maps are indexed by picture_id. Would make it easier to manage this. You
could also save a lookup in SendPictures.

[sandersd (OOO until July 31), 2014/07/18 21:50:15]

It's a picture ID.

This is a reasonable idea (it's what I did for DecodedFrames), but I'd like to
hold off until the flushing logic is done to be sure it doesn't impact that.
I've added a TODO.

 
-  // Decoder configuration (used only on decoder thread).
+  //
+  // Decoder thread state.
+  //
   VTDecompressionOutputCallbackRecord callback_;
   base::ScopedCFTypeRef<CMFormatDescriptionRef> format_;
   base::ScopedCFTypeRef<VTDecompressionSessionRef> session_;
   media::H264Parser parser_;
   gfx::Size coded_size_;
 
-  // Member variables should appear before the WeakPtrFactory, to ensure
-  // that any WeakPtrs to Controller are invalidated before its members
-  // variable's destructors are executed, rendering them invalid.
+  //
+  // Unprotected shared state (set up and torn down on GPU thread).

[Pawel Osciak, 2014/07/18 01:22:13]

What does "unprotected" mean?

[sandersd (OOO until July 31), 2014/07/18 21:50:14]

It means there is no lock (there will be lock-protected shared state to handle
flushing correctly).

+  //
+  scoped_refptr<base::SingleThreadTaskRunner> gpu_task_runner_;
+
+  // Declared second-to-last to prevent any GPU thread tasks from executing once
+  // state starts being destroyed.
   base::WeakPtrFactory<VTVideoDecodeAccelerator> weak_this_factory_;
 
+  // Declared last to ensure that all decoder thread tasks complete before any
+  // state starts being destroyed.
+  base::Thread decoder_thread_;
+
   DISALLOW_COPY_AND_ASSIGN(VTVideoDecodeAccelerator);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_COMMON_GPU_MEDIA_VT_VIDEO_DECODE_ACCELERATOR_H_