rendering_helper - Refactoring - remove the Client interface.

content/common/gpu/media/rendering_helper.h

Index: content/common/gpu/media/rendering_helper.h
diff --git a/content/common/gpu/media/rendering_helper.h b/content/common/gpu/media/rendering_helper.h
index a2dfb1bbf492690104ee2f183aa076ccfc60a3a3..826de05ac454b2d14fb54a04d07f46538f23f7a5 100644
--- a/content/common/gpu/media/rendering_helper.h
+++ b/content/common/gpu/media/rendering_helper.h
@@ -5,6 +5,7 @@
 #ifndef CONTENT_COMMON_GPU_MEDIA_RENDERING_HELPER_H_
 #define CONTENT_COMMON_GPU_MEDIA_RENDERING_HELPER_H_
 
+#include <deque>
 #include <map>
 #include <vector>
 
@@ -24,27 +25,49 @@ class WaitableEvent;
 
 namespace content {
 
-struct RenderingHelperParams;
+class VideoFrame {

[Pawel Osciak, 2014/08/14 04:48:23]

Why not use media::VideoFrame?

[Owen Lin, 2014/08/14 09:20:41]

The main reason is I don't know how to use it in our case. It uses mailbox not
texture_id.

Besides, we don't need many things there like coded size and visible size.

[Pawel Osciak, 2014/08/15 05:45:15]

Ok, texture_id is an issue (the other is not really ;)).
Let's just please rename it to VideoFrameTexture or something similar so that
nobody confuses it with media::VideoFrame.

+ public:
+  uint32 texture_id() const { return texture_id_; }
+  uint32 texture_target() const { return texture_target_; }
+
+  VideoFrame(uint32 texture_target,
+             uint32 texture_id,
+             const base::Closure& no_longer_needed_cb);
+  ~VideoFrame();
+
+ private:
+  uint32 texture_target_;
+  uint32 texture_id_;
+  base::Closure no_longer_needed_cb_;
+};
+
+struct RenderingHelperParams {
+  RenderingHelperParams();
+  ~RenderingHelperParams();
+
+  // The rendering FPS.
+  int rendering_fps;
+
+  // The designed size of each window.

[Pawel Osciak, 2014/08/14 04:48:23]

s/designed//

Please explain this is used for the regular playback mode with each stream
playing in its own window on the screen, and the members below that they are
used for the thumbnail mode where all frames are rendered in sequence onto one
FBO for comparison/verification purposes in thumbnail test.

[Owen Lin, 2014/08/14 09:20:41]

Sorry, it should be "desired". We may resize the window in order to show all
streams on the screen.

The window_sizes is also used for the thumbnail mode. So, I rephrase the comment
a little bit.


On 2014/08/14 04:48:23, Pawel Osciak wrote:

+  std::vector<gfx::Size> window_sizes;
+
+  // Whether the frames are rendered as scaled thumbnails within a
+  // larger FBO that is in turn rendered to the window.
+  bool render_as_thumbnails;
+  // The size of the FBO containing all visible thumbnails.
+  gfx::Size thumbnails_page_size;
+  // The size of each thumbnail within the FBO.
+  gfx::Size thumbnail_size;
+};
 
 // Creates and draws textures used by the video decoder.
 // This class is not thread safe and thus all the methods of this class
 // (except for ctor/dtor) ensure they're being run on a single thread.
 class RenderingHelper {
  public:
-  // Interface for the content provider of the RenderingHelper.
-  class Client {
-   public:
-    // Callback to tell client to render the content.
-    virtual void RenderContent(RenderingHelper* helper) = 0;
-
-    // Callback to get the desired window size of the client.
-    virtual const gfx::Size& GetWindowSize() = 0;
-
-   protected:
-    virtual ~Client() {}
-  };
 
   RenderingHelper();
+

[Pawel Osciak, 2014/08/14 04:48:23]

Not needed.

[Owen Lin, 2014/08/14 09:20:41]

Done.

   ~RenderingHelper();
 
   static bool InitializeOneOff();
@@ -67,9 +90,10 @@ class RenderingHelper {
   // |texture_target|.
   void RenderThumbnail(uint32 texture_target, uint32 texture_id);
 
-  // Render |texture_id| to the current view port of the screen using target
-  // |texture_target|.
-  void RenderTexture(uint32 texture_target, uint32 texture_id);
+  // Queues the VideoFrame for rendering.
+  void QueueVideoFrame(size_t window_id, scoped_ptr<VideoFrame> video_frame);
+
+  void ClearVideoFrames(size_t window_id);

[Pawel Osciak, 2014/08/14 04:48:23]

Please document and rename to DropPendingFrames().

[Owen Lin, 2014/08/14 09:20:41]

Done.

 
   // Delete |texture_id|.
   void DeleteTexture(uint32 texture_id);
@@ -87,12 +111,25 @@ class RenderingHelper {
                           base::WaitableEvent* done);
 
  private:
+  struct RenderingClient {

[Pawel Osciak, 2014/08/14 04:48:23]

I think we should not call it a Client, because this is not a Client, just a
struct with data for clients. Otherwise it would imply that we owned the clients
by having a vector of them below.

How about RenderedVideo ?

[Owen Lin, 2014/08/14 09:20:41]

Done.

On 2014/08/14 04:48:23, Pawel Osciak wrote:

+    gfx::Rect render_area;

[Pawel Osciak, 2014/08/14 04:48:23]

Please documentation for all members.

[Owen Lin, 2014/08/14 09:20:41]

Done.

+    bool last_frame_rendered;

[Pawel Osciak, 2014/08/14 04:48:23]

Isn't this equivalent to pending_frames.empty() ?

[Owen Lin, 2014/08/14 09:20:41]

Why? do you mean pending_frames.size() == 1? We will keep the last frame even it
has been rendered.

[Pawel Osciak, 2014/08/15 05:45:15]

Sorry, this was something that I forgot to remove before posting my comments for
previous PS. Please ignore.

+    base::Closure wait_rendering_cb;
+    std::deque<VideoFrame*> pending_frames;

[Pawel Osciak, 2014/08/14 04:48:23]

Please keep scoped_refptrs here.

[Owen Lin, 2014/08/14 09:20:41]

Done.

+
+    inline RenderingClient() : last_frame_rendered(false) {}

[Pawel Osciak, 2014/08/14 04:48:23]

s/inline//

[Owen Lin, 2014/08/14 09:20:41]

Done.

+  };
+
   void Clear();
 
   void RenderContent();
 
-  void LayoutRenderingAreas();
+  void LayoutRenderingAreas(const std::vector<gfx::Size>& window_sizes);
+
+  // Render |texture_id| to the current view port of the screen using target
+  // |texture_target|.
 

[Pawel Osciak, 2014/08/14 04:48:23]

Remove empty line please.

[Owen Lin, 2014/08/14 09:20:41]

Done.

+  void RenderTexture(uint32 texture_target, uint32 texture_id);
   // Timer to trigger the RenderContent() repeatly.
   scoped_ptr<base::RepeatingTimer<RenderingHelper> > render_timer_;
   base::MessageLoop* message_loop_;
@@ -104,10 +141,7 @@ class RenderingHelper {
 
   gfx::Size screen_size_;
 
-  // The rendering area of each window on the screen.
-  std::vector<gfx::Rect> render_areas_;
-
-  std::vector<base::WeakPtr<Client> > clients_;
+  std::vector<RenderingClient> clients_;
 
   bool render_as_thumbnails_;
   int frame_count_;
@@ -121,24 +155,6 @@ class RenderingHelper {
   DISALLOW_COPY_AND_ASSIGN(RenderingHelper);
 };
 
-struct RenderingHelperParams {
-  RenderingHelperParams();
-  ~RenderingHelperParams();
-
-  // The rendering FPS.
-  int rendering_fps;
-
-  // The clients who provide the content for rendering.
-  std::vector<base::WeakPtr<RenderingHelper::Client> > clients;
-
-  // Whether the frames are rendered as scaled thumbnails within a
-  // larger FBO that is in turn rendered to the window.
-  bool render_as_thumbnails;
-  // The size of the FBO containing all visible thumbnails.
-  gfx::Size thumbnails_page_size;
-  // The size of each thumbnail within the FBO.
-  gfx::Size thumbnail_size;
-};
 }  // namespace content
 
 #endif  // CONTENT_COMMON_GPU_MEDIA_RENDERING_HELPER_H_