Integrating with new rendering for CRD iOS.

remoting/client/ios/display/gl_display_handler.mm

Index: remoting/client/ios/display/gl_display_handler.mm
diff --git a/remoting/client/ios/display/gl_display_handler.mm b/remoting/client/ios/display/gl_display_handler.mm
index d5636e80116bf1f5450d857ac7aa5159ef65c21a..10ef89a8c6abf360e20523f3dddf59144bb66ad3 100644
--- a/remoting/client/ios/display/gl_display_handler.mm
+++ b/remoting/client/ios/display/gl_display_handler.mm
@@ -6,8 +6,11 @@
 #error "This file requires ARC support."
 #endif
 
+#include <array>
+
 #import "remoting/client/ios/display/gl_display_handler.h"
 
+#import "base/mac/bind_objc_block.h"
 #import "remoting/client/display/sys_opengl.h"
 #import "remoting/client/ios/display/gl_demo_screen.h"
 
@@ -15,7 +18,6 @@
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/weak_ptr.h"
-#include "remoting/client/chromoting_client.h"
 #include "remoting/client/chromoting_client_runtime.h"
 #include "remoting/client/cursor_shape_stub_proxy.h"
 #include "remoting/client/display/gl_canvas.h"
@@ -33,6 +35,8 @@ class Core : public protocol::CursorShapeStub, public GlRendererDelegate {
   Core();
   ~Core() override;
 
+  void Initialize();
+
   // CursorShapeStub interface.
   void SetCursorShape(const protocol::CursorShapeInfo& cursor_shape) override;
 
@@ -41,21 +45,24 @@ class Core : public protocol::CursorShapeStub, public GlRendererDelegate {
   void OnFrameRendered() override;
   void OnSizeChanged(int width, int height) override;
 
-  void Created();
+  void OnFrameReceived(std::unique_ptr<webrtc::DesktopFrame> frame,
+                       const base::Closure& done);
   void Stop();
   void SurfaceChanged(int width, int height);
   std::unique_ptr<protocol::FrameConsumer> GrabFrameConsumer();
+  EAGLContext* GetEAGLContext();
   base::WeakPtr<Core> GetWeakPtr();
 
  private:
-  // Will be std::move'd when GrabFrameConsumer() is called.
   remoting::ChromotingClientRuntime* runtime_;
-  std::unique_ptr<DualBufferFrameConsumer> owned_frame_consumer_;
 
+  // Will be std::move'd when GrabFrameConsumer() is called.
+  std::unique_ptr<DualBufferFrameConsumer> owned_frame_consumer_;
   base::WeakPtr<DualBufferFrameConsumer> frame_consumer_;
+
   EAGLContext* eagl_context_;
-  GlRenderer renderer_;
-  GlDemoScreen demo_screen_;
+  std::unique_ptr<GlRenderer> renderer_;
+  //  GlDemoScreen *demo_screen_;

[Yuwei, 2017/04/21 19:02:42]

Looks like the whole GlDemoScreen class is not used any more after you remove
this from the GlDisplayHandler. Are you keeping that class for future OpenGL
debugging?

[nicholss, 2017/04/21 21:00:06]

Yes just for debugging at the moment, not quite ready to give it up yet. But I
will as we get closer to beta.

 
   // Used on display thread.
   base::WeakPtr<Core> weak_ptr_;
@@ -66,22 +73,61 @@ class Core : public protocol::CursorShapeStub, public GlRendererDelegate {
 
 Core::Core() : weak_factory_(this) {
   runtime_ = ChromotingClientRuntime::GetInstance();
+  DCHECK(!runtime_->display_task_runner()->BelongsToCurrentThread());
+
   weak_ptr_ = weak_factory_.GetWeakPtr();
-  renderer_.SetDelegate(weak_ptr_);
+
+  runtime_->display_task_runner()->PostTask(
+      FROM_HERE, base::Bind(&Core::Initialize, base::Unretained(this)));
+
+  // Do not bind GlRenderer::OnFrameReceived. |renderer_| is not ready yet.
   owned_frame_consumer_.reset(new remoting::DualBufferFrameConsumer(
-      base::Bind(&remoting::GlRenderer::OnFrameReceived,
-                 renderer_.GetWeakPtr()),
+      base::Bind(&Core::OnFrameReceived, weak_ptr_),
       runtime_->display_task_runner(),
-      remoting::protocol::FrameConsumer::PixelFormat::FORMAT_RGBA));
+      protocol::FrameConsumer::PixelFormat::FORMAT_RGBA));
   frame_consumer_ = owned_frame_consumer_->GetWeakPtr();
-  renderer_.AddDrawable(demo_screen_.GetWeakPtr());
 }
 
 Core::~Core() {}

[Yuwei, 2017/04/21 19:02:42]

I think we should DCHECK it's being destructed on the display thread since we
don't have a thread checker and weak_ptr_ may not be bound to any thread yet
just after the Core is created...

[nicholss, 2017/04/21 21:00:06]

Seems good to me.

 
+void Core::Initialize() {
+  DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
+
+  eagl_context_ = [EAGLContext currentContext];
+  if (!eagl_context_) {
+    // TODO(nicholss): For prod code, make sure to check for ES3 support and
+    // fall back to ES2 if needed.
+    eagl_context_ =
+        [[EAGLContext alloc] initWithAPI:kEAGLRenderingAPIOpenGLES3];
+    [EAGLContext setCurrentContext:eagl_context_];
+  }
+
+  renderer_ = remoting::GlRenderer::CreateGlRendererWithDesktop();
+
+  //  renderer_.RequestCanvasSize();
+
+  renderer_->OnSurfaceCreated(
+      base::MakeUnique<GlCanvas>(static_cast<int>([eagl_context_ API])));
+
+  SurfaceChanged(1024, 640);  // TODO(nicholss): Where does this data comefrom?
+
+  // TODO(nicholss): This are wrong values but it lets us get something on the
+  // screen.
+  std::array<float, 9> matrix;

[Yuwei, 2017/04/21 18:28:59]

Looks like it may not zero-initialize the matrix:
http://stackoverflow.com/questions/18296274/why-stdarrayint-10-x-is-not-zero-...

Do you see any scaling or translation in the viewport? I could expect the
viewport to stay at the top-left corner of the desktop without scaling if the
matrix is indeed identity.

What about something like this:
std::array<float, 9> matrix =
  {1, 0, 0,
   0, 1, 0,
   0, 0, 1};

This might be clearer to the read.

[Yuwei, 2017/04/21 19:02:42]

*reader.

The test I suggested above probably doesn't work since you hardcoded the surface
size...

[nicholss, 2017/04/21 21:00:06]

I had this before and it would not compile. I get the following:


../../remoting/client/ios/display/gl_display_handler.mm:119:4: error: suggest
braces around initialization of subobject [-Werror,-Wmissing-braces]
  {1, 0, 0,
   ^~~~~~~~
1 error generated.




OK it turns out you need to double { the array for this type.

But git cl format forces it back to {{1, 0, 0, 0, 1, 0, 0, 0, 1}};

+  matrix[0] = 1;
+  matrix[4] = 1;
+  matrix[8] = 1;
+
+  renderer_->OnPixelTransformationChanged(matrix);
+
+  // demo_screen_ = new GlDemoScreen();
+  // renderer_->AddDrawable(demo_screen_->GetWeakPtr());
+  renderer_->SetDelegate(weak_ptr_);
+}
+
 void Core::SetCursorShape(const protocol::CursorShapeInfo& cursor_shape) {
   DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
-  renderer_.OnCursorShapeChanged(cursor_shape);
+  renderer_->OnCursorShapeChanged(cursor_shape);
 }
 
 bool Core::CanRenderFrame() {
@@ -94,6 +140,12 @@ std::unique_ptr<protocol::FrameConsumer> Core::GrabFrameConsumer() {
   return std::move(owned_frame_consumer_);
 }
 
+void Core::OnFrameReceived(std::unique_ptr<webrtc::DesktopFrame> frame,
+                           const base::Closure& done) {
+  DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
+  renderer_->OnFrameReceived(std::move(frame), done);
+}
+
 void Core::OnFrameRendered() {
   // Nothing to do.
 }
@@ -102,33 +154,20 @@ void Core::OnSizeChanged(int width, int height) {
   // Nothing to do.
 }
 
-void Core::Created() {
-  DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
-  DCHECK(!eagl_context_);
-
-  eagl_context_ = [EAGLContext currentContext];
-  if (!eagl_context_) {
-    eagl_context_ =
-        [[EAGLContext alloc] initWithAPI:kEAGLRenderingAPIOpenGLES3];
-    [EAGLContext setCurrentContext:eagl_context_];
-  }
-  renderer_.RequestCanvasSize();
-
-  renderer_.OnSurfaceCreated(base::MakeUnique<GlCanvas>(
-      static_cast<int>([eagl_context_ API])));
-}
-
 void Core::Stop() {
   DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
 
   eagl_context_ = nil;
-  // demo_screen_
-  // renderer_ = nil;
+  // demo_screen_ = nil;
 }
 
 void Core::SurfaceChanged(int width, int height) {
   DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread());
-  renderer_.OnSurfaceChanged(width, height);
+  renderer_->OnSurfaceChanged(width, height);
+}
+
+EAGLContext* Core::GetEAGLContext() {
+  return eagl_context_;
 }
 
 base::WeakPtr<remoting::GlDisplayHandler::Core> Core::GetWeakPtr() {
@@ -138,49 +177,53 @@ base::WeakPtr<remoting::GlDisplayHandler::Core> Core::GetWeakPtr() {
 }  // namespace GlDisplayHandler
 }  // namespace remoting
 
-@interface GlDisplayHandler ()
-@property(nonatomic) remoting::GlDisplayHandler::Core* core_;
-@property(nonatomic) remoting::ChromotingClientRuntime* runtime_;
+@interface GlDisplayHandler () {
+  remoting::GlDisplayHandler::Core* _core;
+  remoting::ChromotingClientRuntime* _runtime;
+}
 @end
 
 @implementation GlDisplayHandler
 
-@synthesize core_ = _core_;
-@synthesize runtime_ = _runtime_;
-
-- (id)initWithRuntime:(remoting::ChromotingClientRuntime*)runtime {
-  self.runtime_ = runtime;
+- (id)init {
+  self = [super init];
+  if (self) {
+    _runtime = remoting::ChromotingClientRuntime::GetInstance();
+    _core = new remoting::GlDisplayHandler::Core();
+  }
   return self;
 }
 
-- (void)created {
-  _core_ = new remoting::GlDisplayHandler::Core();
-
-  self.runtime_->display_task_runner()->PostTask(
-      FROM_HERE, base::Bind(&remoting::GlDisplayHandler::Core::Created,
-                            self.core_->GetWeakPtr()));
-}
+#pragma mark - Public
 
 - (void)stop {
-  self.runtime_->display_task_runner()->PostTask(
-      FROM_HERE, base::Bind(&remoting::GlDisplayHandler::Core::Stop,
-                            self.core_->GetWeakPtr()));
+  _runtime->display_task_runner()->PostTask(
+      FROM_HERE,
+      base::Bind(&remoting::GlDisplayHandler::Core::Stop, _core->GetWeakPtr()));
 }
 
 - (std::unique_ptr<remoting::protocol::VideoRenderer>)CreateVideoRenderer {
   return base::MakeUnique<remoting::SoftwareVideoRenderer>(
-      _core_->GrabFrameConsumer());
+      _core->GrabFrameConsumer());
 }
 
 - (std::unique_ptr<remoting::protocol::CursorShapeStub>)CreateCursorShapeStub {
   return base::MakeUnique<remoting::CursorShapeStubProxy>(
-      _core_->GetWeakPtr(), self.runtime_->display_task_runner());
+      _core->GetWeakPtr(), _runtime->display_task_runner());
+}
+
+- (EAGLContext*)GetEAGLContext {
+  return _core->GetEAGLContext();
 }
 
 // In general, avoid expensive work in this function to maximize frame rate.
 - (void)glkView:(GLKView*)view drawInRect:(CGRect)rect {
-  if (_core_) {
-    _core_->SurfaceChanged(rect.size.width, rect.size.height);
+  if (_core) {
+    _runtime->display_task_runner()->PostTask(
+        FROM_HERE,
+        base::Bind(&remoting::GlDisplayHandler::Core::SurfaceChanged,
+                   _core->GetWeakPtr(), rect.size.width, rect.size.height));
+    // should be RequestRender

[Yuwei, 2017/04/21 19:02:42]

The Android client is configured to draw (swap buffer) on demand rather than
continuously swapping to save power and avoid flickering/tearing. We should be
able to do the same thing with EAGL context?

In this case I don't think the caller needs to RequestRender anything...

[nicholss, 2017/04/21 21:00:06]

This method is not used in the current implementation, I added a TODO to
refactor this out.

   }
 }