[Chromoting] Implement GlRenderer

remoting/client/gl_renderer.cc

Index: remoting/client/gl_renderer.cc
diff --git a/remoting/client/gl_renderer.cc b/remoting/client/gl_renderer.cc
new file mode 100644
index 0000000000000000000000000000000000000000..b5c061c16a2401a9f5ff1d281c10327df24c3349
--- /dev/null
+++ b/remoting/client/gl_renderer.cc
@@ -0,0 +1,158 @@
+// Copyright 2016 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.
+
+#include "remoting/client/gl_renderer.h"
+
+#include "base/bind.h"
+#include "base/logging.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "remoting/client/gl_canvas.h"
+#include "remoting/client/gl_math.h"
+#include "remoting/client/sys_opengl.h"
+#include "third_party/webrtc/modules/desktop_capture/desktop_frame.h"
+
+namespace remoting {
+
+GlRenderer::GlRenderer() :
+    weak_factory_(this) {
+  weak_ptr_ = weak_factory_.GetWeakPtr();
+  thread_checker_.DetachFromThread();
+}
+
+GlRenderer::~GlRenderer() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+}
+
+void GlRenderer::SetRenderCallback(const RenderCallback& callback) {
+  DCHECK(!render_callback_);
+  render_callback_ = callback;
+}
+
+void GlRenderer::SetCanvasSizeChangedCallback(const SizeCallback& callback) {
+  DCHECK(!canvas_size_changed_callback_);
+  canvas_size_changed_callback_ = callback;
+}
+
+void GlRenderer::RequestCanvasSize() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  canvas_size_changed_callback_.Run(canvas_width_, canvas_height_);
+}
+
+void GlRenderer::OnPixelTransformationChanged(
+    std::unique_ptr<std::array<float, 9>> mat) {

[Sergey Ulanov, 2016/07/25 19:34:13]

nit: Pass the argument as a const reference to the array. This function doesn't
really need to take ownership

[Yuwei, 2016/07/25 19:52:18]

Currently NormalizeTransformationMatrix is done directly on the array. Passing
owned array may avoid copying it for normalization. Copying just an array of 9
probably won't have noticable performance impact though...

[Sergey Ulanov, 2016/07/25 21:22:41]

call it |matrix| instead of |mat|.

[Sergey Ulanov, 2016/07/25 21:22:41]

The way this function is implemented it doesn't really need to take ownership of
the matrix. Problem is that if the caller doesn't have the array on heap or
doesn't have ownership then it would have to copy it itself.

[Yuwei, 2016/07/25 22:13:55]

But base::Bind will still copy it, right? For current usage in the WIP
JniGlDisplayHandler, it creates an array and fills it up then move it to the
closure. By having const std::array& we will need to do the first copy to pass
the array to the closure and a second copy to get a non-const array.

Anyway, I will just use const reference so that it can be more friendly to the
caller.

+  DCHECK(thread_checker_.CalledOnValidThread());
+  NormalizeTransformationMatrix(view_width_, view_height_, canvas_width_,
+                                canvas_height_, mat.get());
+  canvas_->SetNormalizedTransformation(*mat.get());
+  RequestRender();
+}
+
+void GlRenderer::OnCursorMoved(int x, int y) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  cursor_.SetCursorPosition(x, y);
+  RequestRender();
+}
+
+void GlRenderer::OnCursorInputFeedback(int x, int y, float diameter) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  cursor_feedback_.StartAnimation(((float) x) / canvas_width_,
+                                  ((float) y) / canvas_height_,
+                                  diameter / canvas_width_,
+                                  diameter / canvas_height_);
+  RequestRender();
+}
+
+void GlRenderer::OnCursorVisibilityChanged(bool visible) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  cursor_.SetCursorVisible(visible);
+  RequestRender();
+}
+
+void GlRenderer::OnFrameReceived(std::unique_ptr<webrtc::DesktopFrame> frame,
+                                 const base::Closure& done) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(frame->size().width() > 0 && frame->size().height() > 0);
+  if (canvas_width_ != frame->size().width() ||
+      canvas_height_ != frame->size().height()) {
+    if (canvas_size_changed_callback_) {
+      canvas_size_changed_callback_.Run(frame->size().width(),
+                                        frame->size().height());
+    }
+    canvas_width_ = frame->size().width();
+    canvas_height_ = frame->size().height();
+    cursor_.SetCanvasSize(canvas_width_, canvas_height_);
+  }
+
+  desktop_.SetVideoFrame(std::move(frame));
+  pending_done_callbacks_.push(done);
+  RequestRender();
+}
+
+void GlRenderer::OnCursorShapeChanged(const protocol::CursorShapeInfo& shape) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  cursor_.SetCursorShape(shape);
+  RequestRender();
+}
+
+void GlRenderer::OnSurfaceCreated(int gl_version) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  // Set the background clear color to black.
+  glClearColor(0.0f, 0.0f, 0.0f, 1.0f);
+  canvas_.reset(new GlCanvas(gl_version));
+  desktop_.SetCanvas(canvas_.get());
+  cursor_.SetCanvas(canvas_.get());
+  cursor_feedback_.SetCanvas(canvas_.get());
+}
+
+void GlRenderer::OnSurfaceChanged(int view_width, int view_height) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(view_width > 0 && view_height > 0);
+  glViewport(0, 0, view_width, view_height);
+  view_width_ = view_width;
+  view_height_ = view_height;
+  RequestRender();
+}
+
+void GlRenderer::OnSurfaceDestroyed() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  cursor_feedback_.SetCanvas(nullptr);
+  cursor_.SetCanvas(nullptr);
+  desktop_.SetCanvas(nullptr);
+  canvas_.reset();
+}
+
+base::WeakPtr<GlRenderer> GlRenderer::GetWeakPtr() {
+  return weak_ptr_;
+}
+
+void GlRenderer::RequestRender() {
+  if (render_scheduled_) {
+    return;
+  }
+  base::ThreadTaskRunnerHandle::Get()->PostTask(

[Yuwei, 2016/07/23 01:02:02]

The cursor will occasionally shake when moving around if the task runs without
delay. Still trying to figure out what is going on...

[Yuwei, 2016/07/23 04:41:14]

Just did some printf debugging and turns out the problem is that OnCursorMoved
will be occasionally blocked by eglSwapBuffers then we will get one frame with
only OnTransformationChanged and the next frame with two OnCursorMoved calls.
This makes the cursor move back and forth on the screen.

If we post the task with some delay, then we will consistently get the sequence
of move viewport -- move cursor -- swap buffer and the cursor will not shake.

Interestingly even just delaying the render task for 1ms the cursor won't
shake...

[Yuwei, 2016/07/25 19:52:18]

For this issue, should I just add back the minimum rendering interval logic?

[Sergey Ulanov, 2016/07/25 21:22:41]

As far as I can tell here is what happens:
 1. Display thread: The renderer renders a frame.
 2. Display thread: JniGlDisplayHandler::RenderOnDisplayThread() posts a
NotifyRenderDoneOnUiThread() task to notify that rendering has been done.
 3. UI thread: FlingAnimationJob moves the cursor in response to the
OnRenderDone event.
 4. UI thread: JniGlDisplayHandler::OnCursorPixelPositionChanged() post a task
to display thread to move the cursor, which may or may not be executed before
the next frame is rendered because the desktop thread may be already rendering
another frame.

Adding a delay before each render may appear to fix the problem, but it's not
going to be reliable (e.g. if (3) takes more than 1ms due to garbage
collection). I think the renderer should wait for notification that all
animations have been processed and render another frame only after that. I.e. it
would looks something like this:
  1. Render frame.
  2. Request the UI layer (java) to process animations, e.g. move the cursor,
scroll, etc. This is effectively what "RenderDone" is doing right now.
  3. Wait for notification that (2) has been done.
  4. Post task for (1) to render next frame if necessary.

Step 3 is missing right now, and that's what causes jerkiness right now.

[Yuwei, 2016/07/25 21:34:50]

That's a fair enough solution. I think I just need to block the render request
until both animations are processed.

[Yuwei, 2016/07/26 05:01:43]

Looks like this only solves the specific problem of animation. User interaction
that moves both the cursor and viewport will still make the cursor jump a little
bit... This will be hard to resolve unless you totally bundle cursor movement
with the viewport movement.

Delaying just 1ms will not be reliable, but making it render not earlier than
15ms will statistically have fewer problem (events out of sync, frame skipping,
etc.) and require less hacks.

[Yuwei, 2016/07/26 22:26:55]

Hmm... Looks like stuttering can still happen at the beginning of the movement.
It will only go away if we unconditionally delay the rendering task, which will
hurt the rendering latency of the desktop frame...

Alternatively we can bundle cursor movement with the viewport movement or have
different delay for different reasons of requesting render.

I think I will just hold this issue for the JniGlDisplayHandler CL and have the
simplest implementation in this CL...

+      FROM_HERE, base::Bind(&GlRenderer::OnRender, weak_ptr_));
+  render_scheduled_ = true;
+}
+
+void GlRenderer::OnRender() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  render_scheduled_ = false;
+  render_callback_.Run(base::Bind(&GlRenderer::OnDrawFrame, weak_ptr_));
+}
+
+void GlRenderer::OnDrawFrame() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  glClear(GL_COLOR_BUFFER_BIT);
+  desktop_.Draw();
+  cursor_.Draw();
+  if (cursor_feedback_.Draw()) {
+    RequestRender();
+  }
+  while (!pending_done_callbacks_.empty()) {
+    pending_done_callbacks_.front().Run();
+    pending_done_callbacks_.pop();
+  }
+}
+
+}  // namespace remoting