rendering_helper - Refactoring - remove the Client interface.

content/common/gpu/media/rendering_helper.cc

Index: content/common/gpu/media/rendering_helper.cc
diff --git a/content/common/gpu/media/rendering_helper.cc b/content/common/gpu/media/rendering_helper.cc
index 7ee940148d2d2f9e741fd38f0b3c1d798fad37df..ebb58f5f3e126112e4020ba6978a0b643a02253d 100644
--- a/content/common/gpu/media/rendering_helper.cc
+++ b/content/common/gpu/media/rendering_helper.cc
@@ -9,9 +9,11 @@
 #include <vector>
 
 #include "base/bind.h"
+#include "base/callback_helpers.h"
 #include "base/command_line.h"
 #include "base/mac/scoped_nsautorelease_pool.h"
 #include "base/message_loop/message_loop.h"
+#include "base/stl_util.h"
 #include "base/strings/stringize_macros.h"
 #include "base/synchronization/waitable_event.h"
 #include "ui/gl/gl_context.h"
@@ -60,6 +62,19 @@ RenderingHelperParams::RenderingHelperParams() {}
 
 RenderingHelperParams::~RenderingHelperParams() {}
 
+VideoFrame::VideoFrame(uint32 texture_target,
+                       uint32 texture_id,
+                       const base::Closure& no_longer_needed_cb)
+    : texture_target_(texture_target),
+      texture_id_(texture_id),
+      no_longer_needed_cb_(no_longer_needed_cb) {
+  DCHECK(!no_longer_needed_cb_.is_null());
+}
+
+VideoFrame::~VideoFrame() {
+  base::ResetAndReturn(&no_longer_needed_cb_).Run();
+}
+
 // static
 bool RenderingHelper::InitializeOneOff() {
   base::CommandLine* cmd_line = base::CommandLine::ForCurrentProcess();
@@ -78,15 +93,15 @@ RenderingHelper::RenderingHelper() {
 }
 
 RenderingHelper::~RenderingHelper() {
-  CHECK_EQ(clients_.size(), 0U) << "Must call UnInitialize before dtor.";
+  CHECK_EQ(videos_.size(), 0U) << "Must call UnInitialize before dtor.";
   Clear();
 }
 
 void RenderingHelper::Initialize(const RenderingHelperParams& params,
                                  base::WaitableEvent* done) {
-  // Use cients_.size() != 0 as a proxy for the class having already been
+  // Use videos_.size() != 0 as a proxy for the class having already been
   // Initialize()'d, and UnInitialize() before continuing.
-  if (clients_.size()) {
+  if (videos_.size()) {
     base::WaitableEvent done(false, false);
     UnInitialize(&done);
     done.Wait();
@@ -153,12 +168,12 @@ void RenderingHelper::Initialize(const RenderingHelperParams& params,
       NULL, gl_surface_, gfx::PreferIntegratedGpu);
   gl_context_->MakeCurrent(gl_surface_);
 
-  clients_ = params.clients;
-  CHECK_GT(clients_.size(), 0U);
-  LayoutRenderingAreas();
+  CHECK_GT(params.window_sizes.size(), 0U);
+  videos_.resize(params.window_sizes.size());
+  LayoutRenderingAreas(params.window_sizes);
 
   if (render_as_thumbnails_) {
-    CHECK_EQ(clients_.size(), 1U);
+    CHECK_EQ(videos_.size(), 1U);
 
     GLint max_texture_size;
     glGetIntegerv(GL_MAX_TEXTURE_SIZE, &max_texture_size);
@@ -370,6 +385,26 @@ void RenderingHelper::RenderThumbnail(uint32 texture_target,
   ++frame_count_;
 }
 
+void RenderingHelper::QueueVideoFrame(size_t window_id,
+                                      scoped_refptr<VideoFrame> video_frame) {
+  RenderedVideo* video = &videos_[window_id];
+
+  // Pop the front if it has been rendered.
+  if (video->last_frame_rendered) {
+    DCHECK(!video->pending_frames.empty());

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

Please comment why, this is not obvious.

[Owen Lin, 2014/08/18 09:03:08]

Done.

+    video->pending_frames.pop_front();
+    video->last_frame_rendered = false;

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

By the way, for the future, we should have some thread checking in this class to
ensure we have no races.

[Owen Lin, 2014/08/18 09:03:08]

Acknowledged.

+  }
+
+  video->pending_frames.push_back(video_frame);
+}
+
+void RenderingHelper::DropPendingFrames(size_t window_id) {
+  RenderedVideo* video = &videos_[window_id];
+  video->pending_frames.clear();
+  video->last_frame_rendered = false;
+}
+
 void RenderingHelper::RenderTexture(uint32 texture_target, uint32 texture_id) {
   // The ExternalOES sampler is bound to GL_TEXTURE1 and the Texture2D sampler
   // is bound to GL_TEXTURE0.
@@ -398,7 +433,7 @@ void* RenderingHelper::GetGLDisplay() {
 }
 
 void RenderingHelper::Clear() {
-  clients_.clear();
+  videos_.clear();
   message_loop_ = NULL;
   gl_context_ = NULL;
   gl_surface_ = NULL;
@@ -461,16 +496,28 @@ void RenderingHelper::RenderContent() {
   CHECK_EQ(base::MessageLoop::current(), message_loop_);
   glUniform1i(glGetUniformLocation(program_, "tex_flip"), 1);
 
+  // To keep the frame before SwapBuffers()

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

This is a bit too vague. Maybe: "Frames that will no longer be needed/will be
returned after rendering. We don't return a rendered frame if it's the last
pending frame on the queue and we have to retain it in case client falls behind
and we need to render the same frame again on the next timer tick."

[Owen Lin, 2014/08/18 09:03:08]

The same comment was added to the member "last_frame_rendered".
I think we need "frames_to_be_returned" is to keep the frame so that it won't be
reused before SwapBuffer. I have rephrased the sentence to explain more.

+  std::vector<scoped_refptr<VideoFrame> > frames_to_be_returned;
+
   if (render_as_thumbnails_) {
     // In render_as_thumbnails_ mode, we render the FBO content on the
     // screen instead of the decoded textures.
-    GLSetViewPort(render_areas_[0]);
+    GLSetViewPort(videos_[0].render_area);
     RenderTexture(GL_TEXTURE_2D, thumbnails_texture_id_);
   } else {
-    for (size_t i = 0; i < clients_.size(); ++i) {
-      if (clients_[i]) {
-        GLSetViewPort(render_areas_[i]);
-        clients_[i]->RenderContent(this);
+    for (size_t i = 0; i < videos_.size(); ++i) {
+      RenderedVideo* video = &videos_[i];
+      if (video->pending_frames.empty())
+        continue;
+      scoped_refptr<VideoFrame> frame = video->pending_frames.front();
+      GLSetViewPort(video->render_area);
+      RenderTexture(frame->texture_target(), frame->texture_id());
+
+      if (video->pending_frames.size() > 1) {
+        frames_to_be_returned.push_back(video->pending_frames.front());

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

s/video->pending_frames.front()/frame/

[Owen Lin, 2014/08/18 09:03:08]

Done.

+        video->pending_frames.pop_front();
+      } else {
+        video->last_frame_rendered = true;

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

Isn't this a problem in QueueVideoFrame? We may return a frame that hasn't been
rendered yet if size > 1 and client is queuing faster than we can render?

[Owen Lin, 2014/08/18 09:03:08]

No, it won't. We set video_last_frame_rendered = true only when
pending_frame.size() == 1 and the frame was just being rendered.

[Pawel Osciak, 2014/08/20 10:26:26]

Ok now I know why I misunderstood this. You are using last_frame_rendered to
mean both that there is only one frame in the pending_frames queue, as well as
the fact that it was rendered. This is confusing that it is used to indicate
there is only one frame left in the queue. I would prefer if we could please use
pending_frames.count() to detect that we only have one frame left at l.394
instead of the DCHECK. Otherwise it's hard to understand that this happens. I
know it's redundant, but I think in this case it helps understanding.

[Owen Lin, 2014/08/21 03:45:18]

We will keep at most one frame for delayed decoding (render the last frame
again). So, last_frame_rendered don't have to be true if there are more than one
frames (In that case, we will drop the rendered frame and set
last_frame_rendered to false). Did I convince you it should be a DCHECK ? :)

On 2014/08/20 10:26:26, Pawel Osciak wrote:

       }
     }
   }
@@ -492,11 +539,12 @@ static void ScaleAndCalculateOffsets(std::vector<int>* lengths,
   }
 }
 
-void RenderingHelper::LayoutRenderingAreas() {
+void RenderingHelper::LayoutRenderingAreas(
+    const std::vector<gfx::Size>& window_sizes) {
   // Find the number of colums and rows.
-  // The smallest n * n or n * (n + 1) > number of clients.
-  size_t cols = sqrt(clients_.size() - 1) + 1;
-  size_t rows = (clients_.size() + cols - 1) / cols;
+  // The smallest n * n or n * (n + 1) > number of windows.
+  size_t cols = sqrt(videos_.size() - 1) + 1;
+  size_t rows = (videos_.size() + cols - 1) / cols;
 
   // Find the widths and heights of the grid.
   std::vector<int> widths(cols);
@@ -504,31 +552,30 @@ void RenderingHelper::LayoutRenderingAreas() {
   std::vector<int> offset_x(cols);
   std::vector<int> offset_y(rows);
 
-  for (size_t i = 0; i < clients_.size(); ++i) {
-    const gfx::Size& window_size = clients_[i]->GetWindowSize();
-    widths[i % cols] = std::max(widths[i % cols], window_size.width());
-    heights[i / cols] = std::max(heights[i / cols], window_size.height());
+  for (size_t i = 0; i < window_sizes.size(); ++i) {
+    const gfx::Size& size = window_sizes[i];
+    widths[i % cols] = std::max(widths[i % cols], size.width());
+    heights[i / cols] = std::max(heights[i / cols], size.height());
   }
 
   ScaleAndCalculateOffsets(&widths, &offset_x, screen_size_.width());
   ScaleAndCalculateOffsets(&heights, &offset_y, screen_size_.height());
 
   // Put each render_area_ in the center of each cell.
-  render_areas_.clear();
-  for (size_t i = 0; i < clients_.size(); ++i) {
-    const gfx::Size& window_size = clients_[i]->GetWindowSize();
+  for (size_t i = 0; i < window_sizes.size(); ++i) {
+    const gfx::Size& size = window_sizes[i];
     float scale =
-        std::min(static_cast<float>(widths[i % cols]) / window_size.width(),
-                 static_cast<float>(heights[i / cols]) / window_size.height());
+        std::min(static_cast<float>(widths[i % cols]) / size.width(),
+                 static_cast<float>(heights[i / cols]) / size.height());
 
     // Don't scale up the texture.
     scale = std::min(1.0f, scale);
 
-    size_t w = scale * window_size.width();
-    size_t h = scale * window_size.height();
+    size_t w = scale * size.width();
+    size_t h = scale * size.height();
     size_t x = offset_x[i % cols] + (widths[i % cols] - w) / 2;
     size_t y = offset_y[i / cols] + (heights[i / cols] - h) / 2;
-    render_areas_.push_back(gfx::Rect(x, y, w, h));
+    videos_[i].render_area = gfx::Rect(x, y, w, h);
   }
 }
 }  // namespace content