Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2272253004:
[Remoting Android] Move mRenderData into TouchInputHandler (Closed)
Description[Remoting Android] Move mRenderData into TouchInputHandler
As the old bitmap DesktopView is removed, there is no need to allow the
DesktopView to directly access the render data. This CL moves mRenderData into
TouchInputHandler to simplify the logic of the DesktopView a little bit.
BUG=641123
Committed: https://crrev.com/e48c522694fb6849c113c2442e2cd4fa3eea779d
Cr-Commit-Position: refs/heads/master@{#414772}
Patch Set 1 #
Total comments: 10
Messages
Total messages: 14 (3 generated)
yuweih@chromium.org changed reviewers: + joedow@chromium.org, zijiehe@chromium.org
I'm planning to decouple render controlling code like cursorMoved() from DesktopView and this is the first refactoring CL. PTAL https://codereview.chromium.org/2272253004/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java (left): https://codereview.chromium.org/2272253004/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java:52: if (mRenderData.imageHeight == 0 || mRenderData.imageWidth == 0 Just removed this check since it doesn't look useful. The combined transformation in GPU is transformation * position / view_size, and nothing will be drawn if either the transformation or the view size is not set.
https://codereview.chromium.org/2272253004/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java (right): https://codereview.chromium.org/2272253004/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:211: mViewer.transformationChanged(mRenderData.transform); The safety of this logic depends on mRenderData will be accessed only in UI thread. Would you mind to add a ThreadChecker to ensure this behavior? An example is @ https://cs.chromium.org/chromium/src/third_party/webrtc/base/java/src/org/web.... But another suggestion to resolve this issue in a better pattern is to add thread checking logic in the Event. i.e. A parameter to control whether an Event can only be raised in a certain thread. Then it would be safe to assume all the listeners will be executed in one thread. (transformationChangedEvent.Raise(new Parameter(mRenderData.transform) instead of mViewer.transformationChanged(mRenderData.transform)). Same issue impacts cursor position and visibility. https://codereview.chromium.org/2272253004/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java (right): https://codereview.chromium.org/2272253004/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java:86: mOnHostSizeChangedListenerKey = mDisplay Since GlDisplay and GlDesktopView are the only components, I suggest to remove onHostSizeChanged() from DesktopView, and add it into Display class. So you do not need to forward this event.
https://codereview.chromium.org/2272253004/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java (right): https://codereview.chromium.org/2272253004/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java:86: mOnHostSizeChangedListenerKey = mDisplay On 2016/08/25 21:55:14, Hzj_jie wrote: > Since GlDisplay and GlDesktopView are the only components, I suggest to remove > onHostSizeChanged() from DesktopView, and add it into Display class. So you do > not need to forward this event. Yep. That's what I am planning to do. I'm planning to add an interface called RenderCallback to GlDisplay and make it handle on*SizeChanged and onCanvasRendered events.
https://codereview.chromium.org/2272253004/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java (right): https://codereview.chromium.org/2272253004/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:211: mViewer.transformationChanged(mRenderData.transform); On 2016/08/25 21:55:14, Hzj_jie wrote: > The safety of this logic depends on mRenderData will be accessed only in UI > thread. Would you mind to add a ThreadChecker to ensure this behavior? > An example is @ > https://cs.chromium.org/chromium/src/third_party/webrtc/base/java/src/org/web.... But this is for WebRTC... Maybe have a simplified copy in our code base? > But another suggestion to resolve this issue in a better pattern is to add > thread checking logic in the Event. i.e. A parameter to control whether an Event > can only be raised in a certain thread. Then it would be safe to assume all the > listeners will be executed in one thread. (transformationChangedEvent.Raise(new > Parameter(mRenderData.transform) instead of > mViewer.transformationChanged(mRenderData.transform)). > > Same issue impacts cursor position and visibility. I think now it is basically a precondition that all public functions in Java can only be used on the UI thread and RenderData is explicitly defined to be single threaded so I'm less worried about the thread safety of mRenderData (or any other classes that are not explicitly defined to be multi-threaded). We can have a CL later to ensure thread safety if we think that's really useful.
lgtm https://codereview.chromium.org/2272253004/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java (right): https://codereview.chromium.org/2272253004/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:211: mViewer.transformationChanged(mRenderData.transform); On 2016/08/25 22:44:16, Yuwei wrote: > On 2016/08/25 21:55:14, Hzj_jie wrote: > > The safety of this logic depends on mRenderData will be accessed only in UI > > thread. Would you mind to add a ThreadChecker to ensure this behavior? > > An example is @ > > > https://cs.chromium.org/chromium/src/third_party/webrtc/base/java/src/org/web.... > > But this is for WebRTC... Maybe have a simplified copy in our code base? > > > But another suggestion to resolve this issue in a better pattern is to add > > thread checking logic in the Event. i.e. A parameter to control whether an > Event > > can only be raised in a certain thread. Then it would be safe to assume all > the > > listeners will be executed in one thread. > (transformationChangedEvent.Raise(new > > Parameter(mRenderData.transform) instead of > > mViewer.transformationChanged(mRenderData.transform)). > > > > Same issue impacts cursor position and visibility. > > I think now it is basically a precondition that all public functions in Java can > only be used on the UI thread and RenderData is explicitly defined to be single > threaded so I'm less worried about the thread safety of mRenderData (or any > other classes that are not explicitly defined to be multi-threaded). We can have > a CL later to ensure thread safety if we think that's really useful. We no longer need any synchronization on the Java side since all of the render operations occur on the same thread. On the native side, the updates are posted to another thread. I don't think we need any thread checking mechanism now (though that would change if we added another thread in the future). https://codereview.chromium.org/2272253004/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java (left): https://codereview.chromium.org/2272253004/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java:52: if (mRenderData.imageHeight == 0 || mRenderData.imageWidth == 0 On 2016/08/25 21:32:28, Yuwei wrote: > Just removed this check since it doesn't look useful. > > The combined transformation in GPU is transformation * position / view_size, and > nothing will be drawn if either the transformation or the view size is not set. The reason this existed was that there used to be a codepath which could call this method before the screen or image size were set and that would result in a divide by zero. I'm ok removing the check, but you might want to wait until after the branchpoint to check this in just to be safe.
https://codereview.chromium.org/2272253004/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java (right): https://codereview.chromium.org/2272253004/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:211: mViewer.transformationChanged(mRenderData.transform); On 2016/08/25 22:57:47, joedow wrote: > On 2016/08/25 22:44:16, Yuwei wrote: > > On 2016/08/25 21:55:14, Hzj_jie wrote: > > > The safety of this logic depends on mRenderData will be accessed only in UI > > > thread. Would you mind to add a ThreadChecker to ensure this behavior? > > > An example is @ > > > > > > https://cs.chromium.org/chromium/src/third_party/webrtc/base/java/src/org/web.... > > > > But this is for WebRTC... Maybe have a simplified copy in our code base? > > > > > But another suggestion to resolve this issue in a better pattern is to add > > > thread checking logic in the Event. i.e. A parameter to control whether an > > Event > > > can only be raised in a certain thread. Then it would be safe to assume all > > the > > > listeners will be executed in one thread. > > (transformationChangedEvent.Raise(new > > > Parameter(mRenderData.transform) instead of > > > mViewer.transformationChanged(mRenderData.transform)). > > > > > > Same issue impacts cursor position and visibility. > > > > I think now it is basically a precondition that all public functions in Java > can > > only be used on the UI thread and RenderData is explicitly defined to be > single > > threaded so I'm less worried about the thread safety of mRenderData (or any > > other classes that are not explicitly defined to be multi-threaded). We can > have > > a CL later to ensure thread safety if we think that's really useful. > > We no longer need any synchronization on the Java side since all of the render > operations occur on the same thread. On the native side, the updates are posted > to another thread. I don't think we need any thread checking mechanism now > (though that would change if we added another thread in the future). Looks like we do create anonymous thread for fetching OAuth token or refreshing host list, but they don't expose the thread publicly and data will always be consumed back in the UI thread. A thread checker may be useful in that case to ensure thread safety. We can add the thread checker later if we check in code like these. https://codereview.chromium.org/2272253004/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java (left): https://codereview.chromium.org/2272253004/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/GlDesktopView.java:52: if (mRenderData.imageHeight == 0 || mRenderData.imageWidth == 0 On 2016/08/25 22:57:47, joedow wrote: > On 2016/08/25 21:32:28, Yuwei wrote: > > Just removed this check since it doesn't look useful. > > > > The combined transformation in GPU is transformation * position / view_size, > and > > nothing will be drawn if either the transformation or the view size is not > set. > > The reason this existed was that there used to be a codepath which could call > this method before the screen or image size were set and that would result in a > divide by zero. I'm ok removing the check, but you might want to wait until > after the branchpoint to check this in just to be safe. Acknowledged.
The CQ bit was checked by yuweih@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2272253004/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java (right): https://codereview.chromium.org/2272253004/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:211: mViewer.transformationChanged(mRenderData.transform); On 2016/08/25 23:14:35, Yuwei wrote: > On 2016/08/25 22:57:47, joedow wrote: > > On 2016/08/25 22:44:16, Yuwei wrote: > > > On 2016/08/25 21:55:14, Hzj_jie wrote: > > > > The safety of this logic depends on mRenderData will be accessed only in > UI > > > > thread. Would you mind to add a ThreadChecker to ensure this behavior? > > > > An example is @ > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/webrtc/base/java/src/org/web.... > > > > > > But this is for WebRTC... Maybe have a simplified copy in our code base? > > > > > > > But another suggestion to resolve this issue in a better pattern is to add > > > > thread checking logic in the Event. i.e. A parameter to control whether an > > > Event > > > > can only be raised in a certain thread. Then it would be safe to assume > all > > > the > > > > listeners will be executed in one thread. > > > (transformationChangedEvent.Raise(new > > > > Parameter(mRenderData.transform) instead of > > > > mViewer.transformationChanged(mRenderData.transform)). > > > > > > > > Same issue impacts cursor position and visibility. > > > > > > I think now it is basically a precondition that all public functions in Java > > can > > > only be used on the UI thread and RenderData is explicitly defined to be > > single > > > threaded so I'm less worried about the thread safety of mRenderData (or any > > > other classes that are not explicitly defined to be multi-threaded). We can > > have > > > a CL later to ensure thread safety if we think that's really useful. > > > > We no longer need any synchronization on the Java side since all of the render > > operations occur on the same thread. On the native side, the updates are > posted > > to another thread. I don't think we need any thread checking mechanism now > > (though that would change if we added another thread in the future). > > Looks like we do create anonymous thread for fetching OAuth token or refreshing > host list, but they don't expose the thread publicly and data will always be > consumed back in the UI thread. > > A thread checker may be useful in that case to ensure thread safety. We can add > the thread checker later if we check in code like these. Sorry, I have not followed up with Java code for a while, and I have not realized this important change.
On 2016/08/26 18:34:58, Hzj_jie wrote: > lgtm > > https://codereview.chromium.org/2272253004/diff/1/remoting/android/java/src/o... > File remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java > (right): > > https://codereview.chromium.org/2272253004/diff/1/remoting/android/java/src/o... > remoting/android/java/src/org/chromium/chromoting/DesktopCanvas.java:211: > mViewer.transformationChanged(mRenderData.transform); > On 2016/08/25 23:14:35, Yuwei wrote: > > On 2016/08/25 22:57:47, joedow wrote: > > > On 2016/08/25 22:44:16, Yuwei wrote: > > > > On 2016/08/25 21:55:14, Hzj_jie wrote: > > > > > The safety of this logic depends on mRenderData will be accessed only in > > UI > > > > > thread. Would you mind to add a ThreadChecker to ensure this behavior? > > > > > An example is @ > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/webrtc/base/java/src/org/web.... > > > > > > > > But this is for WebRTC... Maybe have a simplified copy in our code base? > > > > > > > > > But another suggestion to resolve this issue in a better pattern is to > add > > > > > thread checking logic in the Event. i.e. A parameter to control whether > an > > > > Event > > > > > can only be raised in a certain thread. Then it would be safe to assume > > all > > > > the > > > > > listeners will be executed in one thread. > > > > (transformationChangedEvent.Raise(new > > > > > Parameter(mRenderData.transform) instead of > > > > > mViewer.transformationChanged(mRenderData.transform)). > > > > > > > > > > Same issue impacts cursor position and visibility. > > > > > > > > I think now it is basically a precondition that all public functions in > Java > > > can > > > > only be used on the UI thread and RenderData is explicitly defined to be > > > single > > > > threaded so I'm less worried about the thread safety of mRenderData (or > any > > > > other classes that are not explicitly defined to be multi-threaded). We > can > > > have > > > > a CL later to ensure thread safety if we think that's really useful. > > > > > > We no longer need any synchronization on the Java side since all of the > render > > > operations occur on the same thread. On the native side, the updates are > > posted > > > to another thread. I don't think we need any thread checking mechanism now > > > (though that would change if we added another thread in the future). > > > > Looks like we do create anonymous thread for fetching OAuth token or > refreshing > > host list, but they don't expose the thread publicly and data will always be > > consumed back in the UI thread. > > > > A thread checker may be useful in that case to ensure thread safety. We can > add > > the thread checker later if we check in code like these. > > Sorry, I have not followed up with Java code for a while, and I have not > realized this important change. Thanks!
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== [Remoting Android] Move mRenderData into TouchInputHandler As the old bitmap DesktopView is removed, there is no need to allow the DesktopView to directly access the render data. This CL moves mRenderData into TouchInputHandler to simplify the logic of the DesktopView a little bit. BUG=641123 ========== to ========== [Remoting Android] Move mRenderData into TouchInputHandler As the old bitmap DesktopView is removed, there is no need to allow the DesktopView to directly access the render data. This CL moves mRenderData into TouchInputHandler to simplify the logic of the DesktopView a little bit. BUG=641123 Committed: https://crrev.com/e48c522694fb6849c113c2442e2cd4fa3eea779d Cr-Commit-Position: refs/heads/master@{#414772} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/e48c522694fb6849c113c2442e2cd4fa3eea779d Cr-Commit-Position: refs/heads/master@{#414772} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
