Pepper: PPB_VideoEncoder implementation

ppapi/proxy/video_encoder_resource.h

Index: ppapi/proxy/video_encoder_resource.h
diff --git a/ppapi/proxy/video_encoder_resource.h b/ppapi/proxy/video_encoder_resource.h
index 585d5c14576585f15e90ac012a461e7aca62d7e0..d3643bc4ecc52ca096bf44049953f281de1c4c74 100644
--- a/ppapi/proxy/video_encoder_resource.h
+++ b/ppapi/proxy/video_encoder_resource.h
@@ -5,18 +5,30 @@
 #ifndef PPAPI_PROXY_VIDEO_ENCODER_RESOURCE_H_
 #define PPAPI_PROXY_VIDEO_ENCODER_RESOURCE_H_
 
+#include <deque>
+
 #include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/scoped_vector.h"
 #include "ppapi/proxy/connection.h"
 #include "ppapi/proxy/plugin_resource.h"
+#include "ppapi/shared_impl/media_stream_buffer_manager.h"
 #include "ppapi/shared_impl/resource.h"
 #include "ppapi/thunk/ppb_video_encoder_api.h"
 
+namespace base {
+class SharedMemory;
+}
+
 namespace ppapi {
 
 class TrackedCallback;
 
 namespace proxy {
 
+class SerializedHandle;
+class VideoFrameResource;
+
 class PPAPI_PROXY_EXPORT VideoEncoderResource
     : public PluginResource,
       public thunk::PPB_VideoEncoder_API {
@@ -27,6 +39,25 @@ class PPAPI_PROXY_EXPORT VideoEncoderResource
   thunk::PPB_VideoEncoder_API* AsPPB_VideoEncoder_API() override;
 
  private:
+  struct ShmBuffer {
+    ShmBuffer(base::SharedMemoryHandle handle, uint32_t id, uint32_t size);
+    ~ShmBuffer();
+
+    scoped_ptr<base::SharedMemory> shm;
+    uint32_t id;

[bbudge, 2015/02/10 01:31:08]

There should be a comment somewhere that this is the index of the ShmBuffer in
the ScopedVector.
See:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/proxy/video_...

[llandwerlin-old, 2015/02/10 14:28:13]

Done.

+    uint32_t size;
+  };
+
+  struct BitstreamBuffer {
+    BitstreamBuffer(uint32_t id, uint32_t size, bool key_frame);
+    BitstreamBuffer(const BitstreamBuffer& other);
+    ~BitstreamBuffer();
+
+    uint32_t id;

[bbudge, 2015/02/10 01:31:08]

I think this id has the same meaning as above, right?

[llandwerlin-old, 2015/02/10 14:28:13]

Adding a comment too.

+    uint32_t size;
+    bool key_frame;
+  };
+
   // PPB_VideoEncoder_API implementation.
   int32_t GetSupportedProfiles(
       const PP_ArrayOutput& output,
@@ -53,6 +84,73 @@ class PPAPI_PROXY_EXPORT VideoEncoderResource
                                        uint32_t framerate) override;
   void Close() override;
 
+  // PluginResource implementation.
+  void OnReplyReceived(const ResourceMessageReplyParams& params,
+                       const IPC::Message& msg) override;
+
+  // Reply message handlers for operations that are done in the host.
+  void OnPluginMsgGetSupportedProfilesReply(
+      const PP_ArrayOutput& output,
+      const scoped_refptr<TrackedCallback>& callback,
+      const ResourceMessageReplyParams& params,
+      const std::vector<PP_VideoProfileDescription>& profiles);
+  void OnPluginMsgInitializeReply(const ResourceMessageReplyParams& params,
+                                  uint32_t buffer_count,
+                                  uint32_t buffer_length,
+                                  uint32_t input_frame_count,
+                                  const PP_Size& input_coded_size);
+  void OnPluginMsgGetVideoFramesReply(const ResourceMessageReplyParams& params,

[bbudge, 2015/02/10 01:31:08]

This name is a little confusing to me, since it sounds like it's getting video
frames. How about 'GetVideoFrameInfo'?

[llandwerlin-old, 2015/02/10 14:28:13]

We're actually getting the shm buffer too.
It feels easier to understand when the name match the ipc message name.
Or maybe I could rename the message?

+                                      uint32_t frame_count,
+                                      uint32_t frame_length,
+                                      const PP_Size& frame_size);
+  void OnPluginMsgEncodeReply(const scoped_refptr<TrackedCallback>& callback,
+                              const ResourceMessageReplyParams& params,
+                              uint32_t frame_id);
+
+  // Unsolicited reply message handlers.
+  void OnPluginMsgBitstreamBufferReady(const ResourceMessageReplyParams& params,
+                                       uint32_t buffer_id,
+                                       uint32_t buffer_size,
+                                       bool key_frame);
+  void OnPluginMsgNotifyError(const ResourceMessageReplyParams& params,
+                              int32_t error);
+
+  // Internal utility functions.
+  void NotifyError(int32_t error);
+  void NotifyGetVideoFrameCallbacks();
+  void NotifyGetVideoFrameCallbacksError(int32_t error);
+  void WriteBitstreamerBuffer(PP_BitstreamBuffer* bitstream_buffer,

[bbudge, 2015/02/10 01:31:08]

s/WriteBitstreamerBuffer/WriteBitstreamBuffer

[llandwerlin-old, 2015/02/10 14:28:13]

Oh dear... Done.

+                              const BitstreamBuffer& buffer);
+  void ReleaseFrames();
+
+  bool initialized_;
+  int32_t encoder_last_error_;
+
+  int32_t encoder_frame_count_;
+  PP_Size encoder_coded_size_;
+
+  bool waiting_for_video_frames_;
+
+  scoped_ptr<MediaStreamBufferManager::Delegate> buffer_manager_delegate_;
+  scoped_ptr<MediaStreamBufferManager> buffer_manager_;

[bbudge, 2015/02/10 01:31:08]

Same comments as on PepperVideoEncoderHost to eliminate a separate Delegate, and
use of scoped_ptrs here.

[llandwerlin-old, 2015/02/10 14:28:13]

Done.

+
+  typedef std::map<PP_Resource, scoped_refptr<VideoFrameResource> >

[bbudge, 2015/02/10 01:31:08]

Since we only use C++11 toolchains when we build the proxy, I think we can use
>> without parsing problems.

[llandwerlin-old, 2015/02/10 14:28:14]

I might be wrong, but I think the GCC version used on ChromeOS doesn't support
this.
Last time I triggered a try job with the API containing a ">>" the job failed.

+      VideoFrameMap;
+  VideoFrameMap video_frames_;
+
+  std::deque<std::pair<PP_Resource*, scoped_refptr<TrackedCallback> > >
+      get_video_frame_cbs_;

[bbudge, 2015/02/10 01:31:08]

get_video_frame_cbs_ sounds like a function or method name.
How about 'pending_get_video_frame_calls_' or something similar?

[llandwerlin-old, 2015/02/10 14:28:13]

Done.

+
+  ScopedVector<ShmBuffer> bitstream_buffers_;

[bbudge, 2015/02/10 01:31:08]

Could we name this shm_buffers_?

[llandwerlin-old, 2015/02/10 14:28:13]

Sure.

+
+  std::deque<BitstreamBuffer> available_bitstream_buffers_;
+  typedef std::map<void*, uint32_t> BitstreamBufferMap;
+  BitstreamBufferMap bitstream_buffers_map_;
+
+  scoped_refptr<TrackedCallback> initialize_callback_;
+  scoped_refptr<TrackedCallback> get_bitstreamer_buffer_callback_;

[bbudge, 2015/02/10 01:31:08]

s/get_bitstreamer_buffer_callback_/get_bitstream_buffer_callback_

[llandwerlin-old, 2015/02/10 14:28:13]

Done.

+  PP_BitstreamBuffer* get_bitstreamer_buffer_data_;

[bbudge, 2015/02/10 01:31:08]

s/get_bitstreamer_buffer_data_/get_bitstream_buffer_data_

[llandwerlin-old, 2015/02/10 14:28:13]

Done.

+
   DISALLOW_COPY_AND_ASSIGN(VideoEncoderResource);
 };