[Remoting iOS] Fix screen tearing and screen dimensions

[Remoting iOS] Fix screen tearing and screen dimensions

This CL:
1. Fixes the screen tearing issue by getting rid of the GLKViewController,
   which runs its rendering loops and swaps buffers in unexpected moment. The
   GL view will now do on-demand buffer swapping, similar to how the Android
   client works.
2. Passes GL view dimensions to the renderer to fix the screen aspect issues.

BUG=716699
Review-Url: https://codereview.chromium.org/2848143002
Cr-Commit-Position: refs/heads/master@{#468418}
Committed: https://chromium.googlesource.com/chromium/src/+/807d153f2330e11c8f6f6590ec0eab737c39dffe

[Yuwei, 2017-04-30 01:18:05]

yuweih@chromium.org changed reviewers:
+ jamiewalch@chromium.org, nicholss@chromium.org

[Yuwei, 2017-04-30 01:18:06]

PTAL. Thanks!

https://codereview.chromium.org/2848143002/diff/40001/remoting/client/ios/dis...
File remoting/client/ios/display/gl_display_handler.mm (right):

https://codereview.chromium.org/2848143002/diff/40001/remoting/client/ios/dis...
remoting/client/ios/display/gl_display_handler.mm:172: std::array<float, 9>
matrix = {{1, 0, 0,  // Row 1
BTW this is a trick to stop git cl format by adding comments after the line :P

[Yuwei, 2017-04-30 01:24:25]

Description was changed from

==========
[Remoting iOS] Fix screen tearing and screen dimensions

BUG=
==========

to

==========
[Remoting iOS] Fix screen tearing and screen dimensions

This CL:
1. Fixes the screen tearing issue by getting rid of the GLKViewController,
   which runs its rendering loops and swaps buffers in unexpected moment. The
   GL view will now do on-demand buffer swapping, similar to how the Android
   client works.
2. Passes GL view dimensions to the renderer to fix the screen aspect issues.

BUG=716699
==========

[nicholss, 2017-05-01 16:20:05]

On 2017/04/30 01:18:06, Yuwei wrote:
> PTAL. Thanks!
> 
>
https://codereview.chromium.org/2848143002/diff/40001/remoting/client/ios/dis...
> File remoting/client/ios/display/gl_display_handler.mm (right):
> 
>
https://codereview.chromium.org/2848143002/diff/40001/remoting/client/ios/dis...
> remoting/client/ios/display/gl_display_handler.mm:172: std::array<float, 9>
> matrix = {{1, 0, 0,  // Row 1
> BTW this is a trick to stop git cl format by adding comments after the line :P

LGTM, thanks for the work. Some minor obj-c style issues and conventions noted,
and please explain why you don't want to subclass GLKViewController?

[nicholss, 2017-05-01 16:20:14]

https://codereview.chromium.org/2848143002/diff/40001/remoting/client/ios/app...
File remoting/client/ios/app/host_view_controller.mm (right):

https://codereview.chromium.org/2848143002/diff/40001/remoting/client/ios/app...
remoting/client/ios/app/host_view_controller.mm:20: GLKView* _gl_view;
Why do you need the gl view to be subview and not the class? You can do
everything you are doing in this cl with self.view vs using gl_view.

Also, iOS coding style calls this _glView.

https://codereview.chromium.org/2848143002/diff/40001/remoting/client/ios/app...
remoting/client/ios/app/host_view_controller.mm:54: _gl_view = [[GLKView alloc]
initWithFrame:self.view.bounds];
Views in init should be created with CGRectZero and then sized in
layoutSubviews.

https://codereview.chromium.org/2848143002/diff/40001/remoting/client/ios/app...
remoting/client/ios/app/host_view_controller.mm:55:
_gl_view.enableSetNeedsDisplay = true;
This should move out of the init, you do not know if the controller is in view
or not and this makes extra work that might not be needed. Prob should be in
viewWillAppear:

https://codereview.chromium.org/2848143002/diff/40001/remoting/client/ios/dis...
File remoting/client/ios/display/gl_display_handler.mm (right):

https://codereview.chromium.org/2848143002/diff/40001/remoting/client/ios/dis...
remoting/client/ios/display/gl_display_handler.mm:64: GLKView* gl_view_;
Note, gl_view_ here is correct because this is Obj-C++. It is just the Obj-C
style that you need to worry about.

https://codereview.chromium.org/2848143002/diff/40001/remoting/client/ios/dis...
remoting/client/ios/display/gl_display_handler.mm:172: std::array<float, 9>
matrix = {{1, 0, 0,  // Row 1
On 2017/04/30 01:18:06, Yuwei wrote:
> BTW this is a trick to stop git cl format by adding comments after the line :P

nice!

[nicholss, 2017-05-01 16:53:18]

https://codereview.chromium.org/2848143002/diff/40001/remoting/client/ios/dis...
File remoting/client/ios/display/gl_display_handler.h (right):

https://codereview.chromium.org/2848143002/diff/40001/remoting/client/ios/dis...
remoting/client/ios/display/gl_display_handler.h:31: -
(void)onSurfaceCreated:(GLKView*)view;
I know the rest of the file is not doing this yet, but can you please add a
comment explaining what the new methods do? 

In time every method defined in all interfaces should be commented...

[Yuwei, 2017-05-01 20:58:08]

PTAL Thanks!

I just made it call onSurfaceChanged in viewDidLayoutSubviews so rotation should
(mostly works).

For some reason the first key frame is missing after rotation. I'll resolve it
in another CL.

https://codereview.chromium.org/2848143002/diff/40001/remoting/client/ios/app...
File remoting/client/ios/app/host_view_controller.mm (right):

https://codereview.chromium.org/2848143002/diff/40001/remoting/client/ios/app...
remoting/client/ios/app/host_view_controller.mm:20: GLKView* _gl_view;
On 2017/05/01 16:20:14, nicholss wrote:
> Why do you need the gl view to be subview and not the class? You can do
> everything you are doing in this cl with self.view vs using gl_view.
> 
> Also, iOS coding style calls this _glView.

I just thought it is easier to reuse the UIViewController's default view. I
don't have a strong opinion though.

Just overrode self.view instead.

https://codereview.chromium.org/2848143002/diff/40001/remoting/client/ios/app...
remoting/client/ios/app/host_view_controller.mm:54: _gl_view = [[GLKView alloc]
initWithFrame:self.view.bounds];
On 2017/05/01 16:20:14, nicholss wrote:
> Views in init should be created with CGRectZero and then sized in
> layoutSubviews.

Done. I also needed to fix floatingButton by doing so.

https://codereview.chromium.org/2848143002/diff/40001/remoting/client/ios/app...
remoting/client/ios/app/host_view_controller.mm:55:
_gl_view.enableSetNeedsDisplay = true;
On 2017/05/01 16:20:14, nicholss wrote:
> This should move out of the init, you do not know if the controller is in view
> or not and this makes extra work that might not be needed. Prob should be in
> viewWillAppear:

Done.

https://codereview.chromium.org/2848143002/diff/40001/remoting/client/ios/dis...
File remoting/client/ios/display/gl_display_handler.h (right):

https://codereview.chromium.org/2848143002/diff/40001/remoting/client/ios/dis...
remoting/client/ios/display/gl_display_handler.h:31: -
(void)onSurfaceCreated:(GLKView*)view;
On 2017/05/01 16:53:18, nicholss wrote:
> I know the rest of the file is not doing this yet, but can you please add a
> comment explaining what the new methods do? 
> 
> In time every method defined in all interfaces should be commented...

Done.

https://codereview.chromium.org/2848143002/diff/40001/remoting/client/ios/dis...
File remoting/client/ios/display/gl_display_handler.mm (right):

https://codereview.chromium.org/2848143002/diff/40001/remoting/client/ios/dis...
remoting/client/ios/display/gl_display_handler.mm:64: GLKView* gl_view_;
On 2017/05/01 16:20:14, nicholss wrote:
> Note, gl_view_ here is correct because this is Obj-C++. It is just the Obj-C
> style that you need to worry about.

Acknowledged.

I've also noticed that ARC will still work here even though it looks like a raw
pointer :P

[nicholss, 2017-05-01 21:16:34]

LGTM

Thanks for the help!

[Yuwei, 2017-05-01 21:20:25]

On 2017/05/01 21:16:34, nicholss wrote:
> LGTM
> 
> Thanks for the help!

Thanks! Checking in...

[Yuwei, 2017-05-01 21:20:28]

The CQ bit was checked by yuweih@chromium.org

[commit-bot: I haz the power, 2017-05-01 21:21:12]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yuwei, 2017-05-01 21:23:59]

Sorry... Just forgot the OWNER thing right after you talked about it...

Jamie could you PTAL for OWNER's approval?

[Jamie, 2017-05-01 21:28:37]

lgtm

[commit-bot: I haz the power, 2017-05-01 21:35:36]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493673628044410,
"parent_rev": "879b2fcdbf4efc613abf3ab094ca11d53e74498c", "commit_rev":
"807d153f2330e11c8f6f6590ec0eab737c39dffe"}

[commit-bot: I haz the power, 2017-05-01 21:35:48]

Description was changed from

==========
[Remoting iOS] Fix screen tearing and screen dimensions

This CL:
1. Fixes the screen tearing issue by getting rid of the GLKViewController,
   which runs its rendering loops and swaps buffers in unexpected moment. The
   GL view will now do on-demand buffer swapping, similar to how the Android
   client works.
2. Passes GL view dimensions to the renderer to fix the screen aspect issues.

BUG=716699
==========

to

==========
[Remoting iOS] Fix screen tearing and screen dimensions

This CL:
1. Fixes the screen tearing issue by getting rid of the GLKViewController,
   which runs its rendering loops and swaps buffers in unexpected moment. The
   GL view will now do on-demand buffer swapping, similar to how the Android
   client works.
2. Passes GL view dimensions to the renderer to fix the screen aspect issues.

BUG=716699

Review-Url: https://codereview.chromium.org/2848143002
Cr-Commit-Position: refs/heads/master@{#468418}
Committed:
https://chromium.googlesource.com/chromium/src/+/807d153f2330e11c8f6f6590ec0e...
==========

[commit-bot: I haz the power, 2017-05-01 21:35:49]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/807d153f2330e11c8f6f6590ec0e...