Update VideoDecode PPAPI structs to be consistent with media structures, part 1

media/video/picture.h

Index: media/video/picture.h
diff --git a/media/video/picture.h b/media/video/picture.h
index ec84453fa1f55992d9c665e8832b3be27628f3da..e0c3cfe96a22196408ad16c71e82e0465e223186 100644
--- a/media/video/picture.h
+++ b/media/video/picture.h
@@ -5,59 +5,97 @@
 #ifndef MEDIA_VIDEO_PICTURE_H_
 #define MEDIA_VIDEO_PICTURE_H_
 
-#include <vector>
-
-#include "base/compiler_specific.h"
-#include "media/video/video_decode_accelerator.h"
+#include "base/basictypes.h"
+#include "ui/gfx/size.h"
 
 namespace media {
 
-// TODO(vmr): Evaluate the generalization potential of these interfaces &
-//            classes and refactor as needed with the rest of media stack.
-class PictureBuffer : public VideoDecodeAccelerator::PictureBuffer {
+// Interface expected from PictureBuffers where pictures are stored.

[Ami GONE FROM CHROMIUM, 2011/04/29 18:40:34]

Include a comment pointer to the equivalent PP structs here and below?

[vrk (LEFT CHROMIUM), 2011/05/03 18:19:58]

Done.

+class PictureBuffer {
  public:
-  PictureBuffer(int32 id, gfx::Size pixel_size,
-                std::vector<uint32> color_format, MemoryType memory_type,
-                std::vector<DataPlaneHandle> data_plane_handles);
-  virtual ~PictureBuffer();
-
-  // VideoDecodeAccelerator::PictureBuffer implementation.
-  virtual int32 GetId() OVERRIDE;
-  virtual gfx::Size GetSize() OVERRIDE;
-  virtual const std::vector<uint32>& GetColorFormat() OVERRIDE;
-  virtual MemoryType GetMemoryType() OVERRIDE;
-  virtual std::vector<DataPlaneHandle>& GetPlaneHandles() OVERRIDE;
+  enum MemoryType {
+    PICTUREBUFFER_MEMORYTYPE_NONE = 0,

[Ami GONE FROM CHROMIUM, 2011/04/29 18:40:34]

I wonder about the utility of NONE.  What does that mean?  Send decoded frames
to /dev/null?  

[vrk (LEFT CHROMIUM), 2011/05/03 18:19:58]

I'm not sure either :/ Looks like most of the existing enums have this as well
(see video_decode_accelerator.h). My guess is it's to allow variables holding
enum values to have an uninitialized state? I can remove this if you want.

+    PICTUREBUFFER_MEMORYTYPE_SYSTEM,
+    PICTUREBUFFER_MEMORYTYPE_GL_TEXTURE,
+  };
+
+  union Data {
+    struct {
+      uint32 context_id;  // GLES context id.
+      uint32 texture_id;  // GLES texture id.
+    } gles2_texture;
+    void* sysmem;  // Simply a pointer to system memory.
+    Data();

[Ami GONE FROM CHROMIUM, 2011/04/29 18:40:34]

Unions can have constructors???  OMG.
Per offline convo this is probably unnecessary.

[vrk (LEFT CHROMIUM), 2011/05/03 18:19:58]

Done.

+  };
+
+  PictureBuffer();

[Ami GONE FROM CHROMIUM, 2011/04/29 18:40:34]

ctor goes at the top

[vrk (LEFT CHROMIUM), 2011/05/03 18:19:58]

Done.

+  void Initialize(int32 id, MemoryType memory_type,
+                  gfx::Size dimensions, Data data);
+
+  int32 GetId() const;

[Ami GONE FROM CHROMIUM, 2011/04/29 18:40:34]

const getters can be lower_case()'d (ditto for Picture below).

[vrk (LEFT CHROMIUM), 2011/05/03 18:19:58]

Done.

+  MemoryType GetMemoryType() const;
+  gfx::Size GetDimensions() const;
+  Data GetData() const;
 
  private:
   int32 id_;
-  gfx::Size pixel_size_;
-  std::vector<uint32> color_format_;
   MemoryType memory_type_;
-  std::vector<DataPlaneHandle>& data_plane_handles_;
-
-  DISALLOW_IMPLICIT_CONSTRUCTORS(PictureBuffer);
+  gfx::Size dimensions_;
+  Data data_;
+  bool initialized_;
 };

[scherkus (not reviewing), 2011/04/29 20:33:01]

DISALLOW etc?

[vrk (LEFT CHROMIUM), 2011/05/03 18:19:58]

Since I'm using std::vectors that contain instances of these classes (not
pointers to them), got to allow copy constructors... Let me know if you want me
to change the containers to hold smart pointers instead.

 
-class Picture : public VideoDecodeAccelerator::Picture {
+class Picture {
  public:
-  Picture(PictureBuffer* picture_buffer, gfx::Size decoded_pixel_size,
-          gfx::Size visible_pixel_size, void* user_handle);
-  virtual ~Picture();
+  // Picture size related functions.

[Ami GONE FROM CHROMIUM, 2011/04/29 18:40:34]

ISTM the same commentary belongs also in the PP structs, and this one needs to
point to that one anyway, so just move the comment block?

[vrk (LEFT CHROMIUM), 2011/05/03 18:19:58]

Done.

+  // There are three types of logical picture sizes that applications using
+  // video decoder should be aware of:
+  //   - Visible picture size,
+  //   - Decoded picture size, and
+  //   - Picture buffer size.

[Ami GONE FROM CHROMIUM, 2011/04/29 18:40:34]

You explain visible & decoded sizes, but what about picture buffer size?  Why
would it be different to the others?

[vrk (LEFT CHROMIUM), 2011/05/03 18:19:58]

Ah whoops, didn't read this very carefully when I moved it. Reworked and moved
to pp_video_dev, let me know what you think!

+  //
+  // Visible picture size means the actual picture size that is intended to be
+  // displayed from the decoded output.
+  //
+  // Decoded picture size might vary from the visible size of the picture,
+  // because of the underlying properties of the codec. Vast majority of
+  // modern video compression algorithms are based on (macro)block-based
+  // transforms and therefore process the picture in small windows (usually
+  // of size 16x16 pixels) one by one. However, if the native picture size
+  // does not happen to match the block-size of the algorithm, there may be
+  // redundant data left on the sides of the output picture, which are not
+  // intended for display. For example, this happens to video of size 854x480
+  // and H.264 codec. Since the width (854 pixels) is not multiple of the

[Ami GONE FROM CHROMIUM, 2011/04/29 18:40:34]

s/not/not a/

[vrk (LEFT CHROMIUM), 2011/05/03 18:19:58]

n/a now

+  // block size of the coding format (16 pixels), pixel columns 854-863
+  // contain garbage data which is notintended for display.

[Ami GONE FROM CHROMIUM, 2011/04/29 18:40:34]

s/noti/not i/

[vrk (LEFT CHROMIUM), 2011/05/03 18:19:58]

n/a now

+  //
+  // Plugin is providing the buffers for output decoding and it should know
+  // the picture buffer size it has provided to the decoder. Thus, there is
+  // no function to query the buffer size from this class.
 
-  // VideoDecodeAccelerator::Picture implementation.
-  virtual PictureBuffer* picture_buffer() OVERRIDE;
-  virtual gfx::Size GetDecodedSize() const OVERRIDE;
-  virtual gfx::Size GetVisibleSize() const OVERRIDE;
-  virtual void* GetUserHandle() OVERRIDE;
+  Picture();
+  void Initialize(int32 picture_buffer_id, gfx::Size decoded_pixel_size,
+                  gfx::Size visible_pixel_size, void* user_handle);
+
+  // Returns the id of the picture buffer where this picture is contained.
+  int32 GetId() const;

[Ami GONE FROM CHROMIUM, 2011/04/29 18:40:34]

picture_buffer_id() ?

[vrk (LEFT CHROMIUM), 2011/05/03 18:19:58]

Done.

+
+  // Returns the decoded size of the decoded picture in pixels.
+  gfx::Size GetDecodedSize() const;
+
+  // Returns the visible size of the decoded picture in pixels.
+  gfx::Size GetVisibleSize() const;
+
+  // Returns metadata associated with the picture.
+  void* GetUserHandle() const;
 
  private:
-  // Pointer to the picture buffer which contains this picture.
-  PictureBuffer* picture_buffer_;
+  // ID of the picture buffer which contains this picture.
+  int32 picture_buffer_id_;
   gfx::Size decoded_pixel_size_;
   gfx::Size visible_pixel_size_;
   void* user_handle_;
-
-  DISALLOW_IMPLICIT_CONSTRUCTORS(Picture);
+  bool initialized_;

[Ami GONE FROM CHROMIUM, 2011/04/29 18:40:34]

DISALLOW_COPY_AND_ASSIGN for this and PictureBuffer?

[vrk (LEFT CHROMIUM), 2011/05/03 18:19:58]

As I told Andrew above: since I'm using std::vectors that contain instances of
these classes (not pointers to them), got to allow copy constructors... Let me
know if you want me to change the containers to hold smart pointers instead.

 };
 
 }  // namespace media