Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1305633002:
Refactor drawing desktop. (Closed)
|
Created:
5 years, 4 months ago by shichengfeng Modified:
5 years, 4 months ago CC:
chromium-reviews, chromoting-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAndroid Chromoting: Refactor desktop drawing.
BUG=516871
Committed: https://crrev.com/ac968614ea0f0e988334aab74dd651b4c07e91c3
Cr-Commit-Position: refs/heads/master@{#344652}
Patch Set 1 : Refactor desktop drawing. #
Total comments: 12
Patch Set 2 : Move desktop related variables into CardboardActivityDesktop. #
Total comments: 8
Patch Set 3 : Store desktop bitmap reference in CardboardActivityDesktop. #
Total comments: 6
Patch Set 4 : Rename imageFrame with videoFrame. #
Total comments: 2
Patch Set 5 : Create a lock for video frame bitmap in CardboardActivityDesktop. #
Total comments: 8
Patch Set 6 : Add lock for mHalfWidth in CardboardActivityDestkop. #
Total comments: 3
Patch Set 7 : Add final for locks. #
Messages
Total messages: 26 (9 generated)
Patchset #1 (id:1) has been deleted
shichengfeng@google.com changed reviewers: + lambroslambrou@chromium.org, sergeyu@chromium.org
Patchset #1 (id:20001) has been deleted
Refactor desktop drawing.
https://codereview.chromium.org/1305633002/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java (right): https://codereview.chromium.org/1305633002/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java:49: // Size of the vertexes to draw desktop. English grammar: You can say "number of items in a list" or you can say "size of a list". But not "size of items" because that's talking about the size of each item individually. I would simply say "Number of vertices passed to glDrawArrays()." or "Size of vertex list passed to glDrawArrays()." without talking about "desktop" or "eye position" or anything else. You can then copy-paste the same comment in every similar refactoring. https://codereview.chromium.org/1305633002/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java:50: private static final int VERTEXES_NUMBER = 6; s/VERTEXES/VERTICES/ Code-search has only 4 hits for "vertexes" and 803 hits for "vertices" :) https://codereview.chromium.org/1305633002/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java:83: public void setPosition(FloatBuffer position) { You won't need this if you move the FloatBuffer into this class. https://codereview.chromium.org/1305633002/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java (right): https://codereview.chromium.org/1305633002/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java:190: private FloatBuffer mDesktopCoordinates; I think this should be moved into the CardboardActivityDesktop class. https://codereview.chromium.org/1305633002/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java:342: if (mDesktopCoordinates == null) { mDesktopCoordinates is no longer used in this method, so this seems like the wrong thing to test. The test should be based on whether a desktop frame has been received and loaded into OpenGL. Probably makes sense to move the test inside the mDesktop object, like: mDesktop.hasDesktopImage() or something like that? https://codereview.chromium.org/1305633002/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java:583: private void updateDesktopCoordinatesBuffer(Bitmap bitmap) { Move this inside CardboardActivityDesktop as well.
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Move desktop related variables into CardboardActivityDesktop. https://codereview.chromium.org/1305633002/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java (right): https://codereview.chromium.org/1305633002/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java:49: // Size of the vertexes to draw desktop. On 2015/08/19 23:17:28, Lambros wrote: > English grammar: You can say "number of items in a list" or you can say "size of > a list". But not "size of items" because that's talking about the size of each > item individually. > > I would simply say "Number of vertices passed to glDrawArrays()." > or "Size of vertex list passed to glDrawArrays()." > > without talking about "desktop" or "eye position" or anything else. You can then > copy-paste the same comment in every similar refactoring. Done. https://codereview.chromium.org/1305633002/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java:50: private static final int VERTEXES_NUMBER = 6; On 2015/08/19 23:17:28, Lambros wrote: > s/VERTEXES/VERTICES/ > > Code-search has only 4 hits for "vertexes" and 803 hits for "vertices" :) Done. https://codereview.chromium.org/1305633002/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java:83: public void setPosition(FloatBuffer position) { On 2015/08/19 23:17:28, Lambros wrote: > You won't need this if you move the FloatBuffer into this class. Done. https://codereview.chromium.org/1305633002/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java (right): https://codereview.chromium.org/1305633002/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java:190: private FloatBuffer mDesktopCoordinates; On 2015/08/19 23:17:28, Lambros wrote: > I think this should be moved into the CardboardActivityDesktop class. Done. https://codereview.chromium.org/1305633002/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java:342: if (mDesktopCoordinates == null) { On 2015/08/19 23:17:28, Lambros wrote: > mDesktopCoordinates is no longer used in this method, so this seems like the > wrong thing to test. > > The test should be based on whether a desktop frame has been received and loaded > into OpenGL. Probably makes sense to move the test inside the mDesktop object, > like: > mDesktop.hasDesktopImage() > or something like that? Done. https://codereview.chromium.org/1305633002/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java:583: private void updateDesktopCoordinatesBuffer(Bitmap bitmap) { On 2015/08/19 23:17:28, Lambros wrote: > Move this inside CardboardActivityDesktop as well. Done.
https://codereview.chromium.org/1305633002/diff/100001/remoting/android/java/... File remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java (right): https://codereview.chromium.org/1305633002/diff/100001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java:7: import static org.chromium.chromoting.CardboardDesktopRenderer.makeFloatBuffer; You could move makeFloatBuffer to a separate CardboardUtils class. (can create a separate CL for this if you prefer). https://codereview.chromium.org/1305633002/diff/100001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java:66: private int mHeightPixels; What is this for? mHeightPixels and mWidthPixels are not used anywhere in this class, except for public getter/setter. Are these supposed to match the size of the Bitmap? Why not just keep a reference to the Bitmap in that case? https://codereview.chromium.org/1305633002/diff/100001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java:141: public void setTexture(Bitmap texture) { Why is setTexture() a separate method from updateFrameData()? Could you combine these? https://codereview.chromium.org/1305633002/diff/100001/remoting/android/java/... File remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java (right): https://codereview.chromium.org/1305633002/diff/100001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java:170: // This protects access to mEyePositionVector as well as mDesktop{Height/Width}Pixels. ... as well as mDesktop.
Patchset #3 (id:120001) has been deleted
Store desktop bitmap reference in CardboardActivityDesktop. https://codereview.chromium.org/1305633002/diff/100001/remoting/android/java/... File remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java (right): https://codereview.chromium.org/1305633002/diff/100001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java:7: import static org.chromium.chromoting.CardboardDesktopRenderer.makeFloatBuffer; On 2015/08/20 19:07:27, Lambros wrote: > You could move makeFloatBuffer to a separate CardboardUtils class. > (can create a separate CL for this if you prefer). Will do that in separate CL. https://codereview.chromium.org/1305633002/diff/100001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java:66: private int mHeightPixels; On 2015/08/20 19:07:27, Lambros wrote: > What is this for? mHeightPixels and mWidthPixels are not used anywhere in this > class, except for public getter/setter. > > Are these supposed to match the size of the Bitmap? Why not just keep a > reference to the Bitmap in that case? Done. https://codereview.chromium.org/1305633002/diff/100001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java:141: public void setTexture(Bitmap texture) { On 2015/08/20 19:07:27, Lambros wrote: > Why is setTexture() a separate method from updateFrameData()? Could you combine > these? Done. https://codereview.chromium.org/1305633002/diff/100001/remoting/android/java/... File remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java (right): https://codereview.chromium.org/1305633002/diff/100001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java:170: // This protects access to mEyePositionVector as well as mDesktop{Height/Width}Pixels. On 2015/08/20 19:07:27, Lambros wrote: > ... as well as mDesktop. Done.
https://codereview.chromium.org/1305633002/diff/140001/remoting/android/java/... File remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java (right): https://codereview.chromium.org/1305633002/diff/140001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java:161: public void setImageFrame(Bitmap imageFrame) { Combine this with updateFrameData() - no need for these to be separate methods. Also, in JniInterface, we call it "Video Frame" and "Frame Bitmap". So maybe call the method updateVideoFrame() or updateFrameBitmap(), and pass in the Bitmap as a parameter. https://codereview.chromium.org/1305633002/diff/140001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java:166: return mImageFrame.getHeight(); mImageFrame == null ? 0 : mImageFrame.getHeight() Or: Document that this should only be called when hasImageFrame() returns true, and fix the calling code to check this. https://codereview.chromium.org/1305633002/diff/140001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java:170: return mImageFrame.getWidth(); same here
Rename imageFrame with videoFrame. https://codereview.chromium.org/1305633002/diff/140001/remoting/android/java/... File remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java (right): https://codereview.chromium.org/1305633002/diff/140001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java:161: public void setImageFrame(Bitmap imageFrame) { On 2015/08/20 20:40:56, Lambros wrote: > Combine this with updateFrameData() - no need for these to be separate methods. > Also, in JniInterface, we call it "Video Frame" and "Frame Bitmap". > So maybe call the method updateVideoFrame() or updateFrameBitmap(), and pass in > the Bitmap as a parameter. Done. https://codereview.chromium.org/1305633002/diff/140001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java:166: return mImageFrame.getHeight(); On 2015/08/20 20:40:56, Lambros wrote: > mImageFrame == null ? 0 : mImageFrame.getHeight() > > Or: > Document that this should only be called when hasImageFrame() returns true, and > fix the calling code to check this. Done. https://codereview.chromium.org/1305633002/diff/140001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java:170: return mImageFrame.getWidth(); On 2015/08/20 20:40:56, Lambros wrote: > same here Done.
https://codereview.chromium.org/1305633002/diff/160001/remoting/android/java/... File remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java (right): https://codereview.chromium.org/1305633002/diff/160001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java:170: // This protects access to mEyePositionVector as well as mVideoFrame in mDesktop. We shouldn't "protect access" to some private part of an object. You are synchronizing some CardboardActivityDesktop methods but not others, on the basis of whether the implementation touches the Bitmap or not. This violates encapsulation. Instead, we should handle synchronization internally in the CardboardActivityDesktop class, and document which methods can be called from any thread.
Patchset #5 (id:180001) has been deleted
Patchset #5 (id:200001) has been deleted
Create a lock for video frame bitmap in CardboardActivityDesktop. https://codereview.chromium.org/1305633002/diff/160001/remoting/android/java/... File remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java (right): https://codereview.chromium.org/1305633002/diff/160001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/CardboardDesktopRenderer.java:170: // This protects access to mEyePositionVector as well as mVideoFrame in mDesktop. On 2015/08/20 22:42:14, Lambros wrote: > We shouldn't "protect access" to some private part of an object. You are > synchronizing some CardboardActivityDesktop methods but not others, on the basis > of whether the implementation touches the Bitmap or not. This violates > encapsulation. > > Instead, we should handle synchronization internally in the > CardboardActivityDesktop class, and document which methods can be called from > any thread. Done.
https://codereview.chromium.org/1305633002/diff/220001/remoting/android/java/... File remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java (right): https://codereview.chromium.org/1305633002/diff/220001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java:119: public void updateVideoFrameData(Bitmap videoFrame) { You chose hasVideoFrame() instead of hasVideoFrameData(), so maybe shorten this to updateVideoFrame() ? https://codereview.chromium.org/1305633002/diff/220001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java:168: return mHalfWidth; Is this called on the UI thread? If so, you need to synchronize on mVideoFrameLock, because mHalfWidth is set on the OpenGL thread in updateVideoFrameData(). https://codereview.chromium.org/1305633002/diff/220001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java:172: * Get desktop height and width pixels. s/pixels/in pixels/ https://codereview.chromium.org/1305633002/diff/220001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java:174: public Point getShapePixels() { "shape" is unclear, I think. Maybe getFrameSizePixels() or getVideoFrameSize() ?
Add lock for mHalfWidth in CardboardActivityDestkop. https://codereview.chromium.org/1305633002/diff/220001/remoting/android/java/... File remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java (right): https://codereview.chromium.org/1305633002/diff/220001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java:119: public void updateVideoFrameData(Bitmap videoFrame) { On 2015/08/21 00:08:39, Lambros wrote: > You chose hasVideoFrame() instead of hasVideoFrameData(), so maybe shorten this > to updateVideoFrame() ? Done. https://codereview.chromium.org/1305633002/diff/220001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java:168: return mHalfWidth; On 2015/08/21 00:08:39, Lambros wrote: > Is this called on the UI thread? If so, you need to synchronize on > mVideoFrameLock, because mHalfWidth is set on the OpenGL thread in > updateVideoFrameData(). Done. https://codereview.chromium.org/1305633002/diff/220001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java:172: * Get desktop height and width pixels. On 2015/08/21 00:08:39, Lambros wrote: > s/pixels/in pixels/ Done. https://codereview.chromium.org/1305633002/diff/220001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java:174: public Point getShapePixels() { On 2015/08/21 00:08:39, Lambros wrote: > "shape" is unclear, I think. > Maybe getFrameSizePixels() or getVideoFrameSize() ? Done.
Add final for locks.
https://codereview.chromium.org/1305633002/diff/240001/remoting/android/java/... File remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java (right): https://codereview.chromium.org/1305633002/diff/240001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java:69: private Object mHalfWidthLock = new Object(); final https://codereview.chromium.org/1305633002/diff/240001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java:74: private Object mVideoFrameLock = new Object(); final https://codereview.chromium.org/1305633002/diff/240001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/CardboardActivityDesktop.java:131: if (Math.abs(mHalfWidth - newHalfDesktopWidth) > 0.0001) { mHalfWidth needs to be inside the synchronized block.
lgtm
The CQ bit was checked by shichengfeng@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1305633002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1305633002/260001
Message was sent while issue was closed.
Committed patchset #7 (id:260001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/ac968614ea0f0e988334aab74dd651b4c07e91c3 Cr-Commit-Position: refs/heads/master@{#344652} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
