Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2825403004:
Integrating with new rendering for CRD iOS. (Closed)
DescriptionIntegrating with new rendering for CRD iOS.
BUG=671692
Patch Set 1 #Patch Set 2 : Merge branch 'master' into update_display_ms1 #
Total comments: 10
Patch Set 3 : Adding remoting service changes to this CL, they are dependent. #
Total comments: 11
Patch Set 4 : Updated based on feedback. #Patch Set 5 : Removing service. #
Messages
Total messages: 37 (27 generated)
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
nicholss@chromium.org changed reviewers: + jamiewalch@chromium.org
PTAL, updates to the gl interface to make the app draw it's first remote desktop.
nicholss@chromium.org changed reviewers: + yuweih@chromium.org
https://codereview.chromium.org/2825403004/diff/20001/remoting/client/ios/dis... File remoting/client/ios/display/gl_display_handler.mm (right): https://codereview.chromium.org/2825403004/diff/20001/remoting/client/ios/dis... remoting/client/ios/display/gl_display_handler.mm:65: // GlDemoScreen *demo_screen_; We don't tend to comment-out unused code. Is there a reason not to simply delete it? https://codereview.chromium.org/2825403004/diff/20001/remoting/client/ios/dis... remoting/client/ios/display/gl_display_handler.mm:81: FROM_HERE, base::Bind(&Core::Initialize, base::Unretained(this))); Optional: In general, you should do as little work as possible in the ctor (among other things, it tends to lead to more testable code, although since this is a private class you probably don't want to test it in isolation anyway). Consider refactoring this so that Init() can be called on any thread and bounces itself to the correct thread if necessary, and making it mandatory for the caller to call Init() after construction. https://codereview.chromium.org/2825403004/diff/20001/remoting/client/ios/dis... remoting/client/ios/display/gl_display_handler.mm:99: // fall back to ES2 if needed. Does this need a tracking bug? What devices only support ES2? https://codereview.chromium.org/2825403004/diff/20001/remoting/client/ios/dis... remoting/client/ios/display/gl_display_handler.mm:112: SurfaceChanged(1024, 640); // TODO(nicholss): Where does this data comefrom? s/comefrom/come from/ https://codereview.chromium.org/2825403004/diff/20001/remoting/client/ios/dis... remoting/client/ios/display/gl_display_handler.mm:226: // should be RequestRender Should this be a TODO?
On 2017/04/21 17:16:28, nicholss wrote: This CL is dependent on changes in RemotingService that is in another CL, reworking.
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2825403004/diff/20001/remoting/client/ios/dis... File remoting/client/ios/display/gl_display_handler.mm (right): https://codereview.chromium.org/2825403004/diff/20001/remoting/client/ios/dis... remoting/client/ios/display/gl_display_handler.mm:65: // GlDemoScreen *demo_screen_; On 2017/04/21 17:24:18, Jamie wrote: > We don't tend to comment-out unused code. Is there a reason not to simply delete > it? I was not sure if I was done with it yet was all, I am still integrating with the rest of the system. https://codereview.chromium.org/2825403004/diff/20001/remoting/client/ios/dis... remoting/client/ios/display/gl_display_handler.mm:81: FROM_HERE, base::Bind(&Core::Initialize, base::Unretained(this))); On 2017/04/21 17:24:18, Jamie wrote: > Optional: In general, you should do as little work as possible in the ctor > (among other things, it tends to lead to more testable code, although since this > is a private class you probably don't want to test it in isolation anyway). > Consider refactoring this so that Init() can be called on any thread and bounces > itself to the correct thread if necessary, and making it mandatory for the > caller to call Init() after construction. Acknowledged. For this case I think having the call to init makes sense because it would be in the ctor of the display handler otherwise. Nothing else will use this core class, and i would not tend to do something like this in a normal public class, so I take the point. https://codereview.chromium.org/2825403004/diff/20001/remoting/client/ios/dis... remoting/client/ios/display/gl_display_handler.mm:99: // fall back to ES2 if needed. On 2017/04/21 17:24:18, Jamie wrote: > Does this need a tracking bug? What devices only support ES2? iphone < 5 I think and some iPads. I think a tracking bug will be lost, I have a lot of TODOs in the code at the moment, I plan to sweep them at some point and I am not sure I need the overhead of creating a bunch of bugs.
LGTM once my remaining comments are addressed. https://codereview.chromium.org/2825403004/diff/20001/remoting/client/ios/dis... File remoting/client/ios/display/gl_display_handler.mm (right): https://codereview.chromium.org/2825403004/diff/20001/remoting/client/ios/dis... remoting/client/ios/display/gl_display_handler.mm:65: // GlDemoScreen *demo_screen_; On 2017/04/21 17:35:15, nicholss wrote: > On 2017/04/21 17:24:18, Jamie wrote: > > We don't tend to comment-out unused code. Is there a reason not to simply > delete > > it? > > I was not sure if I was done with it yet was all, I am still integrating with > the rest of the system. Acknowledged. https://codereview.chromium.org/2825403004/diff/20001/remoting/client/ios/dis... remoting/client/ios/display/gl_display_handler.mm:99: // fall back to ES2 if needed. On 2017/04/21 17:35:15, nicholss wrote: > On 2017/04/21 17:24:18, Jamie wrote: > > Does this need a tracking bug? What devices only support ES2? > > iphone < 5 I think and some iPads. I think a tracking bug will be lost, I have a > lot of TODOs in the code at the moment, I plan to sweep them at some point and I > am not sure I need the overhead of creating a bunch of bugs. SGTM. Please create a bug assigned to yourself to make sure we don't forget to do that in case you win the lottery :) https://codereview.chromium.org/2825403004/diff/40001/remoting/client/ios/fac... File remoting/client/ios/facade/remoting_service.mm (right): https://codereview.chromium.org/2825403004/diff/40001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.mm:104: // service. Put this on the line before so it only takes up one line.
https://codereview.chromium.org/2825403004/diff/40001/remoting/client/ios/dis... File remoting/client/ios/display/gl_display_handler.mm (right): https://codereview.chromium.org/2825403004/diff/40001/remoting/client/ios/dis... remoting/client/ios/display/gl_display_handler.mm:116: std::array<float, 9> matrix; 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.
https://codereview.chromium.org/2825403004/diff/40001/remoting/client/ios/dis... File remoting/client/ios/display/gl_display_handler.mm (right): https://codereview.chromium.org/2825403004/diff/40001/remoting/client/ios/dis... remoting/client/ios/display/gl_display_handler.mm:65: // GlDemoScreen *demo_screen_; 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? https://codereview.chromium.org/2825403004/diff/40001/remoting/client/ios/dis... remoting/client/ios/display/gl_display_handler.mm:91: Core::~Core() {} 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... https://codereview.chromium.org/2825403004/diff/40001/remoting/client/ios/dis... remoting/client/ios/display/gl_display_handler.mm:116: std::array<float, 9> matrix; On 2017/04/21 18:28:59, Yuwei wrote: > 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. *reader. The test I suggested above probably doesn't work since you hardcoded the surface size... https://codereview.chromium.org/2825403004/diff/40001/remoting/client/ios/dis... remoting/client/ios/display/gl_display_handler.mm:226: // should be RequestRender 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...
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
This cl is dependent on changes in https://codereview.chromium.org/2828113002 https://codereview.chromium.org/2825403004/diff/40001/remoting/client/ios/dis... File remoting/client/ios/display/gl_display_handler.mm (right): https://codereview.chromium.org/2825403004/diff/40001/remoting/client/ios/dis... remoting/client/ios/display/gl_display_handler.mm:65: // GlDemoScreen *demo_screen_; On 2017/04/21 19:02:42, Yuwei wrote: > 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? Yes just for debugging at the moment, not quite ready to give it up yet. But I will as we get closer to beta. https://codereview.chromium.org/2825403004/diff/40001/remoting/client/ios/dis... remoting/client/ios/display/gl_display_handler.mm:91: Core::~Core() {} On 2017/04/21 19:02:42, Yuwei wrote: > 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... Seems good to me. https://codereview.chromium.org/2825403004/diff/40001/remoting/client/ios/dis... remoting/client/ios/display/gl_display_handler.mm:116: std::array<float, 9> matrix; On 2017/04/21 18:28:59, Yuwei wrote: > 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. 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}}; https://codereview.chromium.org/2825403004/diff/40001/remoting/client/ios/dis... remoting/client/ios/display/gl_display_handler.mm:226: // should be RequestRender On 2017/04/21 19:02:42, Yuwei wrote: > 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... This method is not used in the current implementation, I added a TODO to refactor this out. https://codereview.chromium.org/2825403004/diff/40001/remoting/client/ios/fac... File remoting/client/ios/facade/remoting_service.mm (right): https://codereview.chromium.org/2825403004/diff/40001/remoting/client/ios/fac... remoting/client/ios/facade/remoting_service.mm:104: // service. On 2017/04/21 17:43:01, Jamie wrote: > Put this on the line before so it only takes up one line. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
LGTM
The CQ bit was checked by nicholss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Description was changed from ========== Integrating with new rendering for CRD iOS. BUG=671692 ========== to ========== Integrating with new rendering for CRD iOS. BUG=671692 ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
