Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2148743005:
[Remoting Android] Cursor & Cursor Feedback for OpenGL Renderer (Closed)
|
Created:
4 years, 5 months ago by Yuwei Modified:
4 years, 5 months ago Reviewers:
Sergey Ulanov 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. |
Description[Remoting Android] Cursor & Cursor Feedback for OpenGL Renderer
This CL implements classes for drawing cursor and cursor feedback on the
OpenGL canvas. This is part of the project of using OpenGL to render
remoting host on Android.
BUG=385924
Committed: https://crrev.com/2495ff068c075749e36f20117db5d1c368ffa42e
Cr-Commit-Position: refs/heads/master@{#406991}
Patch Set 1 #Patch Set 2 : Use GYP source list in GN #Patch Set 3 : Fix wrapped interpolation #Patch Set 4 : Swap BGRA to RGBA #
Total comments: 41
Patch Set 5 : Reviewer's Feedback #Patch Set 6 : Use SkColorSetARGB() to create color #
Total comments: 3
Patch Set 7 : Manually draw the texture #
Total comments: 1
Patch Set 8 : Move parameters around #
Total comments: 19
Patch Set 9 : Reviewer's Feedback #Patch Set 10 : Drop Skia dependencies #
Total comments: 26
Patch Set 11 : Reviewer's Feedback #
Total comments: 3
Patch Set 12 : Reviewer's Feedback #
Messages
Total messages: 32 (10 generated)
Patchset #1 (id:1) has been deleted
yuweih@chromium.org changed reviewers: + sergeyu@chromium.org
ptal
https://codereview.chromium.org/2148743005/diff/80001/remoting/base/util.cc File remoting/base/util.cc (right): https://codereview.chromium.org/2148743005/diff/80001/remoting/base/util.cc#n... remoting/base/util.cc:75: void CopyAndSwapRB(const uint8_t* source, uint8_t* dest, int length) { Please use RGBAToARGB() from libyuv. It's optimized with NEON, so it should be significantly faster. It doesn't matter for cursor, but someone will want to use this function for something bigger than mouse cursor. https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... File remoting/client/gl_cursor.h (right): https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor.h:17: } // namespace protocol https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor.h:48: bool visible_ = true; add empty line here please https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... File remoting/client/gl_cursor_feedback.cc (right): https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:14: #include "third_party/webrtc/modules/desktop_capture/desktop_frame.h" I don't think we want to pull this dependency here just for webrtc::DesktopFrame::kBytesPerPixel. webrtc::DesktopFrame::kBytesPerPixel makes sense when working with DesktopFrame, but this class shouldn't depend on DesktopFrame https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:20: const int kFeedbackTexturePixelDiameter = 256; Problem with the way animation is currently implemented is that it ignores screen DPI. It's rather small on modern devices. E.g. Galaxy S7 has screen with DPI of 577, so 256 pixels is about 11mm on that screen, i.e. it's super tiny and hard to notice. Can you please fix that with the new renderer? At very least add a TODO here. https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:39: } git cl format please https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:70: .InMilliseconds() / kAnimationDurationMs; git cl format please https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:86: for (int i = kColorRingsCount - 1; i > -1; i--) { i >= 0 instead of i > -1 https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:87: if (radius >= kFeedbackRadiusStops[i]) { It would be better to break here and then move the rendering logic out of the for loop, but please see my next comment first. https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:118: for (int x = 0; x < kFeedbackTexturePixelDiameter; x++) { This code looks like it would be rather slow. It might be acceptable given that it's called only once, but there are some simple optimizations you can do. E.g. draw one quarter and then copy it to 4 other quarters mirroring as necessary. Also avoid converting between FP and int types point when possible. If we were to use it more than once that I think it would be better to use skia to draw gradients here (see https://skia.org/user/api/skpaint#ShShader). https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:133: std::unique_ptr<uint8_t[]> GlCursorFeedback::texture_; only POD types are allowed for static variables. https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math.cc File remoting/client/gl_math.cc (right): https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math... remoting/client/gl_math.cc:20: } // namespace https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math... remoting/client/gl_math.cc:45: positions[1] = top; I think it would be more readable if you filled in the array in order from 0 to 7 https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math... remoting/client/gl_math.cc:56: outstream << mat[i*num_cols + j] << ", "; spaces around * please. (git cl format?) https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math.h File remoting/client/gl_math.h (right): https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math... remoting/client/gl_math.h:25: void NormalizeTransformationMatrix(std::array<float, 9>& mat, matrix https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math... remoting/client/gl_math.h:25: void NormalizeTransformationMatrix(std::array<float, 9>& mat, use pointer instead of a reference here and move this parameter to the end. https://google.github.io/styleguide/cppguide.html#Reference_Arguments Same for the FillRectangleVertexPositions() https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math... remoting/client/gl_math.h:45: void LinearInterpolate(NumberType* dest, Do you need this as a template? It's used in only one place only fro uint8_t https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math... remoting/client/gl_math.h:51: dest[i] = (1.f - percent) * vec1[i] + percent * vec2[i]; you convert values from int to float and then back to int, which is slow. It's better to do whole computation in int.
https://codereview.chromium.org/2148743005/diff/80001/remoting/base/util.cc File remoting/base/util.cc (right): https://codereview.chromium.org/2148743005/diff/80001/remoting/base/util.cc#n... remoting/base/util.cc:75: void CopyAndSwapRB(const uint8_t* source, uint8_t* dest, int length) { On 2016/07/19 00:42:48, Sergey Ulanov wrote: > Please use RGBAToARGB() from libyuv. It's optimized with NEON, so it should be > significantly faster. It doesn't matter for cursor, but someone will want to use > this function for something bigger than mouse cursor. Wouldn't it be BGRAToRGBA()? Looks like there is no such function but ABGRToARGB() seems to be doing the right thing... I guess that's in little endian? We are doing row-by-row memcpy in GlDesktop when we need to manually pack the image. Is there any similar optimization for that? We will need to list libyuv as the dependency of the renderer. Not sure whether that's a good idea... https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... File remoting/client/gl_cursor_feedback.cc (right): https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:20: const int kFeedbackTexturePixelDiameter = 256; On 2016/07/19 00:42:48, Sergey Ulanov wrote: > Problem with the way animation is currently implemented is that it ignores > screen DPI. It's rather small on modern devices. E.g. Galaxy S7 has screen with > DPI of 577, so 256 pixels is about 11mm on that screen, i.e. it's super tiny and > hard to notice. Can you please fix that with the new renderer? At very least add > a TODO here. I think the actual problem of the current implementation is that in Java it hard codes the radius of the feedback circle in pixels so the small feedback circle is just 80px wide. To fix this we can instead define the radius in dp and convert it back to px when using it. The 256 pixels here is actually just the dimension of the texture. We are still able to draw the low-res texture in larger size with some interpolations, although it won't look very smooth in that case. I can simply change this value to 512 to improve its quality in higher DPI devices... https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:133: std::unique_ptr<uint8_t[]> GlCursorFeedback::texture_; On 2016/07/19 00:42:48, Sergey Ulanov wrote: > only POD types are allowed for static variables. Yeah I don't think it make sense to have a static unique_ptr... Should I just new a uint8_t[] and just not delete it? https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math.h File remoting/client/gl_math.h (right): https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math... remoting/client/gl_math.h:51: dest[i] = (1.f - percent) * vec1[i] + percent * vec2[i]; On 2016/07/19 00:42:48, Sergey Ulanov wrote: > you convert values from int to float and then back to int, which is slow. It's > better to do whole computation in int. Maybe something like: ((100 - percent) * vec1[i] + percent * vec2[i]) / 100 ? Anyway... This may get obsolete if I use Skia to draw the gradient
The CQ bit was checked by yuweih@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...
ptal I guess I will need to list junov@chromium.org as reviewer for adding skia to DEPS? https://codereview.chromium.org/2148743005/diff/80001/remoting/base/util.cc File remoting/base/util.cc (right): https://codereview.chromium.org/2148743005/diff/80001/remoting/base/util.cc#n... remoting/base/util.cc:75: void CopyAndSwapRB(const uint8_t* source, uint8_t* dest, int length) { On 2016/07/19 04:11:52, Yuwei wrote: > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > Please use RGBAToARGB() from libyuv. It's optimized with NEON, so it should be > > significantly faster. It doesn't matter for cursor, but someone will want to > use > > this function for something bigger than mouse cursor. > > Wouldn't it be BGRAToRGBA()? Looks like there is no such function but > ABGRToARGB() seems to be doing the right thing... I guess that's in little > endian? > > We are doing row-by-row memcpy in GlDesktop when we need to manually pack the > image. Is there any similar optimization for that? > > We will need to list libyuv as the dependency of the renderer. Not sure whether > that's a good idea... Done. Using libyuv::ABGRToARGB https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... File remoting/client/gl_cursor.h (right): https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor.h:17: } On 2016/07/19 00:42:48, Sergey Ulanov wrote: > // namespace protocol Done. https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor.h:48: bool visible_ = true; On 2016/07/19 00:42:48, Sergey Ulanov wrote: > add empty line here please Done. https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... File remoting/client/gl_cursor_feedback.cc (right): https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:14: #include "third_party/webrtc/modules/desktop_capture/desktop_frame.h" On 2016/07/19 00:42:48, Sergey Ulanov wrote: > I don't think we want to pull this dependency here just for > webrtc::DesktopFrame::kBytesPerPixel. webrtc::DesktopFrame::kBytesPerPixel makes > sense when working with DesktopFrame, but this class shouldn't depend on > DesktopFrame Done. Reuse the one in remoting/base/util https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:20: const int kFeedbackTexturePixelDiameter = 256; On 2016/07/19 00:42:48, Sergey Ulanov wrote: > Problem with the way animation is currently implemented is that it ignores > screen DPI. It's rather small on modern devices. E.g. Galaxy S7 has screen with > DPI of 577, so 256 pixels is about 11mm on that screen, i.e. it's super tiny and > hard to notice. Can you please fix that with the new renderer? At very least add > a TODO here. Leaved comment in Java GlDesktopView. https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:39: } On 2016/07/19 00:42:48, Sergey Ulanov wrote: > git cl format please Done. https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:70: .InMilliseconds() / kAnimationDurationMs; On 2016/07/19 00:42:48, Sergey Ulanov wrote: > git cl format please Done. https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:86: for (int i = kColorRingsCount - 1; i > -1; i--) { On 2016/07/19 00:42:48, Sergey Ulanov wrote: > i >= 0 instead of i > -1 Obsolete. https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:87: if (radius >= kFeedbackRadiusStops[i]) { On 2016/07/19 00:42:48, Sergey Ulanov wrote: > It would be better to break here and then move the rendering logic out of the > for loop, but please see my next comment first. Obsolete. https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:118: for (int x = 0; x < kFeedbackTexturePixelDiameter; x++) { On 2016/07/19 00:42:48, Sergey Ulanov wrote: > This code looks like it would be rather slow. It might be acceptable given that > it's called only once, but there are some simple optimizations you can do. E.g. > draw one quarter and then copy it to 4 other quarters mirroring as necessary. > Also avoid converting between FP and int types point when possible. > > If we were to use it more than once that I think it would be better to use skia > to draw gradients here (see https://skia.org/user/api/skpaint#ShShader). Done. Using Skia. https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:133: std::unique_ptr<uint8_t[]> GlCursorFeedback::texture_; On 2016/07/19 00:42:48, Sergey Ulanov wrote: > only POD types are allowed for static variables. Done. https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math.cc File remoting/client/gl_math.cc (right): https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math... remoting/client/gl_math.cc:20: } On 2016/07/19 00:42:48, Sergey Ulanov wrote: > // namespace Done. https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math... remoting/client/gl_math.cc:45: positions[1] = top; On 2016/07/19 00:42:48, Sergey Ulanov wrote: > I think it would be more readable if you filled in the array in order from 0 to > 7 Done. https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math... remoting/client/gl_math.cc:56: outstream << mat[i*num_cols + j] << ", "; On 2016/07/19 00:42:48, Sergey Ulanov wrote: > spaces around * please. (git cl format?) Done. https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math.h File remoting/client/gl_math.h (right): https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math... remoting/client/gl_math.h:25: void NormalizeTransformationMatrix(std::array<float, 9>& mat, On 2016/07/19 00:42:48, Sergey Ulanov wrote: > use pointer instead of a reference here and move this parameter to the end. > https://google.github.io/styleguide/cppguide.html#Reference_Arguments > > Same for the FillRectangleVertexPositions() Done. https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math... remoting/client/gl_math.h:45: void LinearInterpolate(NumberType* dest, On 2016/07/19 00:42:48, Sergey Ulanov wrote: > Do you need this as a template? It's used in only one place only fro uint8_t Obsolete. Removed. https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math... remoting/client/gl_math.h:51: dest[i] = (1.f - percent) * vec1[i] + percent * vec2[i]; On 2016/07/19 04:11:52, Yuwei wrote: > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > you convert values from int to float and then back to int, which is slow. It's > > better to do whole computation in int. > > Maybe something like: > > ((100 - percent) * vec1[i] + percent * vec2[i]) / 100 > > ? > > Anyway... This may get obsolete if I use Skia to draw the gradient Removed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/07/19 20:34:24, Yuwei wrote: > ptal > > I guess I will need to list mailto:junov@chromium.org as reviewer for adding skia to > DEPS? > > https://codereview.chromium.org/2148743005/diff/80001/remoting/base/util.cc > File remoting/base/util.cc (right): > > https://codereview.chromium.org/2148743005/diff/80001/remoting/base/util.cc#n... > remoting/base/util.cc:75: void CopyAndSwapRB(const uint8_t* source, uint8_t* > dest, int length) { > On 2016/07/19 04:11:52, Yuwei wrote: > > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > > Please use RGBAToARGB() from libyuv. It's optimized with NEON, so it should > be > > > significantly faster. It doesn't matter for cursor, but someone will want to > > use > > > this function for something bigger than mouse cursor. > > > > Wouldn't it be BGRAToRGBA()? Looks like there is no such function but > > ABGRToARGB() seems to be doing the right thing... I guess that's in little > > endian? > > > > We are doing row-by-row memcpy in GlDesktop when we need to manually pack the > > image. Is there any similar optimization for that? > > > > We will need to list libyuv as the dependency of the renderer. Not sure > whether > > that's a good idea... > > Done. Using libyuv::ABGRToARGB > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... > File remoting/client/gl_cursor.h (right): > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... > remoting/client/gl_cursor.h:17: } > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > // namespace protocol > > Done. > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... > remoting/client/gl_cursor.h:48: bool visible_ = true; > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > add empty line here please > > Done. > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... > File remoting/client/gl_cursor_feedback.cc (right): > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... > remoting/client/gl_cursor_feedback.cc:14: #include > "third_party/webrtc/modules/desktop_capture/desktop_frame.h" > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > I don't think we want to pull this dependency here just for > > webrtc::DesktopFrame::kBytesPerPixel. webrtc::DesktopFrame::kBytesPerPixel > makes > > sense when working with DesktopFrame, but this class shouldn't depend on > > DesktopFrame > > Done. Reuse the one in remoting/base/util > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... > remoting/client/gl_cursor_feedback.cc:20: const int > kFeedbackTexturePixelDiameter = 256; > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > Problem with the way animation is currently implemented is that it ignores > > screen DPI. It's rather small on modern devices. E.g. Galaxy S7 has screen > with > > DPI of 577, so 256 pixels is about 11mm on that screen, i.e. it's super tiny > and > > hard to notice. Can you please fix that with the new renderer? At very least > add > > a TODO here. > > Leaved comment in Java GlDesktopView. > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... > remoting/client/gl_cursor_feedback.cc:39: } > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > git cl format please > > Done. > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... > remoting/client/gl_cursor_feedback.cc:70: .InMilliseconds() / > kAnimationDurationMs; > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > git cl format please > > Done. > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... > remoting/client/gl_cursor_feedback.cc:86: for (int i = kColorRingsCount - 1; i > > -1; i--) { > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > i >= 0 instead of i > -1 > > Obsolete. > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... > remoting/client/gl_cursor_feedback.cc:87: if (radius >= kFeedbackRadiusStops[i]) > { > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > It would be better to break here and then move the rendering logic out of the > > for loop, but please see my next comment first. > > Obsolete. > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... > remoting/client/gl_cursor_feedback.cc:118: for (int x = 0; x < > kFeedbackTexturePixelDiameter; x++) { > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > This code looks like it would be rather slow. It might be acceptable given > that > > it's called only once, but there are some simple optimizations you can do. > E.g. > > draw one quarter and then copy it to 4 other quarters mirroring as necessary. > > Also avoid converting between FP and int types point when possible. > > > > If we were to use it more than once that I think it would be better to use > skia > > to draw gradients here (see https://skia.org/user/api/skpaint#ShShader). > > Done. Using Skia. > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... > remoting/client/gl_cursor_feedback.cc:133: std::unique_ptr<uint8_t[]> > GlCursorFeedback::texture_; > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > only POD types are allowed for static variables. > > Done. > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math.cc > File remoting/client/gl_math.cc (right): > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math... > remoting/client/gl_math.cc:20: } > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > // namespace > > Done. > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math... > remoting/client/gl_math.cc:45: positions[1] = top; > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > I think it would be more readable if you filled in the array in order from 0 > to > > 7 > > Done. > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math... > remoting/client/gl_math.cc:56: outstream << mat[i*num_cols + j] << ", "; > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > spaces around * please. (git cl format?) > > Done. > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math.h > File remoting/client/gl_math.h (right): > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math... > remoting/client/gl_math.h:25: void > NormalizeTransformationMatrix(std::array<float, 9>& mat, > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > use pointer instead of a reference here and move this parameter to the end. > > https://google.github.io/styleguide/cppguide.html#Reference_Arguments > > > > Same for the FillRectangleVertexPositions() > > Done. > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math... > remoting/client/gl_math.h:45: void LinearInterpolate(NumberType* dest, > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > Do you need this as a template? It's used in only one place only fro uint8_t > > Obsolete. Removed. > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math... > remoting/client/gl_math.h:51: dest[i] = (1.f - percent) * vec1[i] + percent * > vec2[i]; > On 2016/07/19 04:11:52, Yuwei wrote: > > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > > you convert values from int to float and then back to int, which is slow. > It's > > > better to do whole computation in int. > > > > Maybe something like: > > > > ((100 - percent) * vec1[i] + percent * vec2[i]) / 100 > > > > ? > > > > Anyway... This may get obsolete if I use Skia to draw the gradient > > Removed. As discussed offline, using Skia for just creating the feedback texture is an overkill and may increase the size of the binary. I have timed the previous nested for-loop implementation and it takes about 30 ms to draw the 512*512 RGBA texture on Nexus 6P (maybe the compiler does some SIMD optimization?). Given that it is called once in the app's lifetime and it is in the order of magnitude of 10 ms, I guess it is okay to keep it like that... Mirroring symmetric parts may lead to complex code. For storing and loading an image for texture, I guess it may end up with a longer decoding time...
On 2016/07/20 04:19:57, Yuwei wrote: > On 2016/07/19 20:34:24, Yuwei wrote: > > ptal > > > > I guess I will need to list mailto:junov@chromium.org as reviewer for adding > skia to > > DEPS? > > > > https://codereview.chromium.org/2148743005/diff/80001/remoting/base/util.cc > > File remoting/base/util.cc (right): > > > > > https://codereview.chromium.org/2148743005/diff/80001/remoting/base/util.cc#n... > > remoting/base/util.cc:75: void CopyAndSwapRB(const uint8_t* source, uint8_t* > > dest, int length) { > > On 2016/07/19 04:11:52, Yuwei wrote: > > > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > > > Please use RGBAToARGB() from libyuv. It's optimized with NEON, so it > should > > be > > > > significantly faster. It doesn't matter for cursor, but someone will want > to > > > use > > > > this function for something bigger than mouse cursor. > > > > > > Wouldn't it be BGRAToRGBA()? Looks like there is no such function but > > > ABGRToARGB() seems to be doing the right thing... I guess that's in little > > > endian? > > > > > > We are doing row-by-row memcpy in GlDesktop when we need to manually pack > the > > > image. Is there any similar optimization for that? > > > > > > We will need to list libyuv as the dependency of the renderer. Not sure > > whether > > > that's a good idea... > > > > Done. Using libyuv::ABGRToARGB > > > > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... > > File remoting/client/gl_cursor.h (right): > > > > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... > > remoting/client/gl_cursor.h:17: } > > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > > // namespace protocol > > > > Done. > > > > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... > > remoting/client/gl_cursor.h:48: bool visible_ = true; > > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > > add empty line here please > > > > Done. > > > > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... > > File remoting/client/gl_cursor_feedback.cc (right): > > > > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... > > remoting/client/gl_cursor_feedback.cc:14: #include > > "third_party/webrtc/modules/desktop_capture/desktop_frame.h" > > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > > I don't think we want to pull this dependency here just for > > > webrtc::DesktopFrame::kBytesPerPixel. webrtc::DesktopFrame::kBytesPerPixel > > makes > > > sense when working with DesktopFrame, but this class shouldn't depend on > > > DesktopFrame > > > > Done. Reuse the one in remoting/base/util > > > > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... > > remoting/client/gl_cursor_feedback.cc:20: const int > > kFeedbackTexturePixelDiameter = 256; > > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > > Problem with the way animation is currently implemented is that it ignores > > > screen DPI. It's rather small on modern devices. E.g. Galaxy S7 has screen > > with > > > DPI of 577, so 256 pixels is about 11mm on that screen, i.e. it's super tiny > > and > > > hard to notice. Can you please fix that with the new renderer? At very least > > add > > > a TODO here. > > > > Leaved comment in Java GlDesktopView. > > > > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... > > remoting/client/gl_cursor_feedback.cc:39: } > > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > > git cl format please > > > > Done. > > > > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... > > remoting/client/gl_cursor_feedback.cc:70: .InMilliseconds() / > > kAnimationDurationMs; > > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > > git cl format please > > > > Done. > > > > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... > > remoting/client/gl_cursor_feedback.cc:86: for (int i = kColorRingsCount - 1; i > > > > -1; i--) { > > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > > i >= 0 instead of i > -1 > > > > Obsolete. > > > > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... > > remoting/client/gl_cursor_feedback.cc:87: if (radius >= > kFeedbackRadiusStops[i]) > > { > > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > > It would be better to break here and then move the rendering logic out of > the > > > for loop, but please see my next comment first. > > > > Obsolete. > > > > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... > > remoting/client/gl_cursor_feedback.cc:118: for (int x = 0; x < > > kFeedbackTexturePixelDiameter; x++) { > > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > > This code looks like it would be rather slow. It might be acceptable given > > that > > > it's called only once, but there are some simple optimizations you can do. > > E.g. > > > draw one quarter and then copy it to 4 other quarters mirroring as > necessary. > > > Also avoid converting between FP and int types point when possible. > > > > > > If we were to use it more than once that I think it would be better to use > > skia > > > to draw gradients here (see https://skia.org/user/api/skpaint#ShShader). > > > > Done. Using Skia. > > > > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... > > remoting/client/gl_cursor_feedback.cc:133: std::unique_ptr<uint8_t[]> > > GlCursorFeedback::texture_; > > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > > only POD types are allowed for static variables. > > > > Done. > > > > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math.cc > > File remoting/client/gl_math.cc (right): > > > > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math... > > remoting/client/gl_math.cc:20: } > > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > > // namespace > > > > Done. > > > > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math... > > remoting/client/gl_math.cc:45: positions[1] = top; > > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > > I think it would be more readable if you filled in the array in order from 0 > > to > > > 7 > > > > Done. > > > > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math... > > remoting/client/gl_math.cc:56: outstream << mat[i*num_cols + j] << ", "; > > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > > spaces around * please. (git cl format?) > > > > Done. > > > > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math.h > > File remoting/client/gl_math.h (right): > > > > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math... > > remoting/client/gl_math.h:25: void > > NormalizeTransformationMatrix(std::array<float, 9>& mat, > > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > > use pointer instead of a reference here and move this parameter to the end. > > > https://google.github.io/styleguide/cppguide.html#Reference_Arguments > > > > > > Same for the FillRectangleVertexPositions() > > > > Done. > > > > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math... > > remoting/client/gl_math.h:45: void LinearInterpolate(NumberType* dest, > > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > > Do you need this as a template? It's used in only one place only fro uint8_t > > > > Obsolete. Removed. > > > > > https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_math... > > remoting/client/gl_math.h:51: dest[i] = (1.f - percent) * vec1[i] + percent * > > vec2[i]; > > On 2016/07/19 04:11:52, Yuwei wrote: > > > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > > > you convert values from int to float and then back to int, which is slow. > > It's > > > > better to do whole computation in int. > > > > > > Maybe something like: > > > > > > ((100 - percent) * vec1[i] + percent * vec2[i]) / 100 > > > > > > ? > > > > > > Anyway... This may get obsolete if I use Skia to draw the gradient > > > > Removed. > > As discussed offline, using Skia for just creating the feedback texture is an > overkill and may increase the size of the binary. Since you had it implemeneted, do you know what the actual impact on binary size is? It would be useful to know, in case we consider using Skia in the future. > > I have timed the previous nested for-loop implementation and it takes about 30 > ms to draw the 512*512 RGBA texture on Nexus 6P (maybe the compiler does some > SIMD optimization?). Given that it is called once in the app's lifetime and it > is in the order of magnitude of 10 ms, I guess it is okay to keep it like > that... Mirroring symmetric parts may lead to complex code. For storing and > loading an image for texture, I guess it may end up with a longer decoding > time... 30ms is a lot, but it would be much higher on slower devices, so I think it's better to fix it, especially given that the optimization I suggested is fairly simple and wouldn't code complexity that much.
https://codereview.chromium.org/2148743005/diff/80001/remoting/base/util.cc File remoting/base/util.cc (right): https://codereview.chromium.org/2148743005/diff/80001/remoting/base/util.cc#n... remoting/base/util.cc:75: void CopyAndSwapRB(const uint8_t* source, uint8_t* dest, int length) { On 2016/07/19 04:11:52, Yuwei wrote: > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > Please use RGBAToARGB() from libyuv. It's optimized with NEON, so it should be > > significantly faster. It doesn't matter for cursor, but someone will want to > use > > this function for something bigger than mouse cursor. > > Wouldn't it be BGRAToRGBA()? Looks like there is no such function but > ABGRToARGB() seems to be doing the right thing... I guess that's in little > endian? > > We are doing row-by-row memcpy in GlDesktop when we need to manually pack the > image. Is there any similar optimization for that? memcpy() itself should be already optimized. > > We will need to list libyuv as the dependency of the renderer. Not sure whether > that's a good idea... Client already depends on libyuv, so this is not a problem https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... File remoting/client/gl_cursor_feedback.cc (right): https://codereview.chromium.org/2148743005/diff/80001/remoting/client/gl_curs... remoting/client/gl_cursor_feedback.cc:20: const int kFeedbackTexturePixelDiameter = 256; On 2016/07/19 04:11:52, Yuwei wrote: > On 2016/07/19 00:42:48, Sergey Ulanov wrote: > > Problem with the way animation is currently implemented is that it ignores > > screen DPI. It's rather small on modern devices. E.g. Galaxy S7 has screen > with > > DPI of 577, so 256 pixels is about 11mm on that screen, i.e. it's super tiny > and > > hard to notice. Can you please fix that with the new renderer? At very least > add > > a TODO here. > > I think the actual problem of the current implementation is that in Java it hard > codes the radius of the feedback circle in pixels so the small feedback circle > is just 80px wide. To fix this we can instead define the radius in dp and > convert it back to px when using it. > > The 256 pixels here is actually just the dimension of the texture. We are still > able to draw the low-res texture in larger size with some interpolations, > although it won't look very smooth in that case. I can simply change this value > to 512 to improve its quality in higher DPI devices... SGTM https://codereview.chromium.org/2148743005/diff/120001/remoting/client/gl_cur... File remoting/client/gl_cursor_feedback.cc (right): https://codereview.chromium.org/2148743005/diff/120001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback.cc:120: bool GlCursorFeedback::texture_initialized_ = false; this flag doesn't have to be a class member. It can be a static variable defined in the .cc file. https://codereview.chromium.org/2148743005/diff/120001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback.cc:120: bool GlCursorFeedback::texture_initialized_ = false; Please use base::Singleton<> to make initialization of this singleton simpler.
> Since you had it implemeneted, do you know what the actual impact on binary size > is? It would be useful to know, in case we consider using Skia in the future. It increases the official build by about 1 MB. 6.4 MB->7.4 MB
ptal https://codereview.chromium.org/2148743005/diff/120001/remoting/client/gl_cur... File remoting/client/gl_cursor_feedback.cc (right): https://codereview.chromium.org/2148743005/diff/120001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback.cc:120: bool GlCursorFeedback::texture_initialized_ = false; On 2016/07/20 18:06:11, Sergey Ulanov wrote: > Please use base::Singleton<> to make initialization of this singleton simpler. Done. https://codereview.chromium.org/2148743005/diff/140001/remoting/client/gl_cur... File remoting/client/gl_cursor_feedback_texture.cc (right): https://codereview.chromium.org/2148743005/diff/140001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.cc:84: new uint32_t[kFeedbackTexturePixelRadius * kFeedbackTexturePixelRadius]); After the change, it takes about 3ms to generate the mapping and 12ms to use it to fill up the texture, which is a worse than linear improvement... Not sure whether there is better implementation for that part..
https://codereview.chromium.org/2148743005/diff/160001/remoting/base/util.h File remoting/base/util.h (right): https://codereview.chromium.org/2148743005/diff/160001/remoting/base/util.h#n... remoting/base/util.h:16: enum { kBytesPerPixelRGB32 = 4 }; I'd like to avoid using util.h in new code. Maybe add GlRenderLayer::kBytesPerPixel? I think it makes sense there because GlRenderLayer deals directly image buffers to upload textures. https://codereview.chromium.org/2148743005/diff/160001/remoting/client/gl_cur... File remoting/client/gl_cursor_feedback_texture.cc (right): https://codereview.chromium.org/2148743005/diff/160001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.cc:30: void LinearInterpolate(const uint8_t* in1, nit: move to anonymous namespace above. https://codereview.chromium.org/2148743005/diff/160001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.cc:33: int percentage, nit: if you use [0..256] range instead of [0..100] range then this compiler will be able to optimize division with bit shift. https://codereview.chromium.org/2148743005/diff/160001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.cc:39: void FillColorByRadius(float radius, uint32_t* out) { Maybe replace this with uint32_t GetColorByRadius(radius)? https://codereview.chromium.org/2148743005/diff/160001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.cc:40: int i; ring_index? https://codereview.chromium.org/2148743005/diff/160001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.cc:42: for (i = kColorRingsCount - 1; radius < kFeedbackRadiusStops[i] && i >= 0; Use while loop here or move the "radius < kFeedbackRadiusStops[i]" condition inside and break the loop. Empty for loop like this is confusing. int ring_index = kColorRingsCount - 1; while (radius < kFeedbackRadiusStops[ring_index] && ring_index >= 0) ring_index++; https://codereview.chromium.org/2148743005/diff/160001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.cc:65: LinearInterpolate(first_color, second_color, byte_out, percentage, 0); for loop? Also I don't think you actually need LinearInterpolate() function. This code would be perfectly readable if you just inline that code here. https://codereview.chromium.org/2148743005/diff/160001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.cc:76: FillColorByRadius(radius, map + x + y * kFeedbackTexturePixelRadius); This code would be simpler if you just set color in 8 pixels at a time: uint32_t color = GetColorByRadius(radius); int x2 = kFeedbackTexturePixelDiameter - 1 - x; int y2 = kFeedbackTexturePixelDiameter - 1 - y; map[x + y * kFeedbackTexturePixelDiameter] = color; map[x2 + y * kFeedbackTexturePixelDiameter] = color; map[x + y2 * kFeedbackTexturePixelDiameter] = color; map[x2 + y2 * kFeedbackTexturePixelDiameter] = color; map[y + x * kFeedbackTexturePixelDiameter] = color; map[y2 + x * kFeedbackTexturePixelDiameter] = color; map[y + x2 * kFeedbackTexturePixelDiameter] = color; map[y2 + x2 * kFeedbackTexturePixelDiameter] = color; https://codereview.chromium.org/2148743005/diff/160001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.cc:83: std::unique_ptr<uint32_t[]> coord_color_map( See my comment above. You can iterate over one 8th of the circle, but fill it in whole by setting 8 pixels on every iteration. That would make this code simpler. https://codereview.chromium.org/2148743005/diff/160001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.cc:116: texture_(new uint32_t[kFeedbackTexturePixelDiameter * git cl format please
ptal https://codereview.chromium.org/2148743005/diff/160001/remoting/client/gl_cur... File remoting/client/gl_cursor_feedback_texture.cc (right): https://codereview.chromium.org/2148743005/diff/160001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.cc:30: void LinearInterpolate(const uint8_t* in1, On 2016/07/20 23:51:40, Sergey Ulanov wrote: > nit: move to anonymous namespace above. Obsolete. Inlined now. https://codereview.chromium.org/2148743005/diff/160001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.cc:33: int percentage, On 2016/07/20 23:51:40, Sergey Ulanov wrote: > nit: if you use [0..256] range instead of [0..100] range then this compiler will > be able to optimize division with bit shift. Done. https://codereview.chromium.org/2148743005/diff/160001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.cc:39: void FillColorByRadius(float radius, uint32_t* out) { On 2016/07/20 23:51:40, Sergey Ulanov wrote: > Maybe replace this with uint32_t GetColorByRadius(radius)? Done. https://codereview.chromium.org/2148743005/diff/160001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.cc:40: int i; On 2016/07/20 23:51:40, Sergey Ulanov wrote: > ring_index? Done. https://codereview.chromium.org/2148743005/diff/160001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.cc:42: for (i = kColorRingsCount - 1; radius < kFeedbackRadiusStops[i] && i >= 0; On 2016/07/20 23:51:40, Sergey Ulanov wrote: > Use while loop here or move the "radius < kFeedbackRadiusStops[i]" condition > inside and break the loop. > Empty for loop like this is confusing. > > int ring_index = kColorRingsCount - 1; > while (radius < kFeedbackRadiusStops[ring_index] && ring_index >= 0) > ring_index++; Done. https://codereview.chromium.org/2148743005/diff/160001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.cc:65: LinearInterpolate(first_color, second_color, byte_out, percentage, 0); On 2016/07/20 23:51:40, Sergey Ulanov wrote: > for loop? > Also I don't think you actually need LinearInterpolate() function. This code > would be perfectly readable if you just inline that code here. Done. https://codereview.chromium.org/2148743005/diff/160001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.cc:76: FillColorByRadius(radius, map + x + y * kFeedbackTexturePixelRadius); On 2016/07/20 23:51:40, Sergey Ulanov wrote: > This code would be simpler if you just set color in 8 pixels at a time: > > uint32_t color = GetColorByRadius(radius); > int x2 = kFeedbackTexturePixelDiameter - 1 - x; > int y2 = kFeedbackTexturePixelDiameter - 1 - y; > map[x + y * kFeedbackTexturePixelDiameter] = color; > map[x2 + y * kFeedbackTexturePixelDiameter] = color; > map[x + y2 * kFeedbackTexturePixelDiameter] = color; > map[x2 + y2 * kFeedbackTexturePixelDiameter] = color; > map[y + x * kFeedbackTexturePixelDiameter] = color; > map[y2 + x * kFeedbackTexturePixelDiameter] = color; > map[y + x2 * kFeedbackTexturePixelDiameter] = color; > map[y2 + x2 * kFeedbackTexturePixelDiameter] = color; > > > Done. BTW x and y are related to the center of the texture so your code may draw something like a curved "x" :P https://codereview.chromium.org/2148743005/diff/160001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.cc:83: std::unique_ptr<uint32_t[]> coord_color_map( On 2016/07/20 23:51:40, Sergey Ulanov wrote: > See my comment above. You can iterate over one 8th of the circle, but fill it in > whole by setting 8 pixels on every iteration. That would make this code simpler. Done. Now it take about 8ms to generate the texture. Much faster :) https://codereview.chromium.org/2148743005/diff/160001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.cc:116: texture_(new uint32_t[kFeedbackTexturePixelDiameter * On 2016/07/20 23:51:40, Sergey Ulanov wrote: > git cl format please Done.
Mostly just some style nits https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_cur... File remoting/client/gl_cursor_feedback.cc (right): https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback.cc:18: const int kTextureId = 2; maybe not for this CL: would it be useful to have a shared header define all texture ids to avoid conflicts? https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_cur... File remoting/client/gl_cursor_feedback_texture.cc (right): https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.cc:31: uint32_t GetColorByRadius(float radius) { move to anonymous namespace. https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_cur... File remoting/client/gl_cursor_feedback_texture.h (right): https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.h:15: class GlCursorFeedbackTexture { add a comment to explain what this class is used for. https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.h:18: const uint8_t* GetTexture() const; empty line above this one https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.h:19: int GetTextureDiameter() const; nit: potentially these can be static functions as well, just to make this class easier to use. https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.h:27: std::unique_ptr<uint32_t[]> texture_; Maybe change this to std::vector<uint8_t> Then you can also return vector reference from GetTexture, which means the consumer will also know the size of the array. https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_ren... File remoting/client/gl_render_layer.cc (right): https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_ren... remoting/client/gl_render_layer.cc:11: namespace { nit: if you put this inside the remoting namespace below then you wouldn't need remoting:: to reference GlRendererLayer. https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_ren... remoting/client/gl_render_layer.cc:59: void PackDirtyRegion(uint8_t* dest, Move this to the anonymous namespace. https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_ren... File remoting/client/gl_render_layer.h (right): https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_ren... remoting/client/gl_render_layer.h:19: enum { kBytesPerPixelRGB32 = 4 }; Don't use enum for define constants. Call it kBytesPerPixel please
https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_cur... File remoting/client/gl_cursor_feedback_texture.cc (right): https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.cc:31: uint32_t GetColorByRadius(float radius) { On 2016/07/21 19:28:48, Sergey Ulanov wrote: > move to anonymous namespace. Also for FillFeedbackTexture? https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_cur... File remoting/client/gl_cursor_feedback_texture.h (right): https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.h:19: int GetTextureDiameter() const; On 2016/07/21 19:28:48, Sergey Ulanov wrote: > nit: potentially these can be static functions as well, just to make this class > easier to use. Maybe just a static const? https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_ren... File remoting/client/gl_render_layer.cc (right): https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_ren... remoting/client/gl_render_layer.cc:11: namespace { On 2016/07/21 19:28:48, Sergey Ulanov wrote: > nit: if you put this inside the remoting namespace below then you wouldn't need > remoting:: to reference GlRendererLayer. Then they are out of the anonymous namespace? I'm not quite understand in general why we need to use anonymous namespace though...
https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_cur... File remoting/client/gl_cursor_feedback_texture.h (right): https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.h:19: int GetTextureDiameter() const; On 2016/07/21 19:53:04, Yuwei wrote: > On 2016/07/21 19:28:48, Sergey Ulanov wrote: > > nit: potentially these can be static functions as well, just to make this > class > > easier to use. > > Maybe just a static const? Yes. But GetTexture() cannot be static const obviously.
https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_cur... File remoting/client/gl_cursor_feedback_texture.cc (right): https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.cc:31: uint32_t GetColorByRadius(float radius) { On 2016/07/21 19:53:04, Yuwei wrote: > On 2016/07/21 19:28:48, Sergey Ulanov wrote: > > move to anonymous namespace. > > Also for FillFeedbackTexture? yes https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_ren... File remoting/client/gl_render_layer.cc (right): https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_ren... remoting/client/gl_render_layer.cc:11: namespace { On 2016/07/21 19:53:04, Yuwei wrote: > On 2016/07/21 19:28:48, Sergey Ulanov wrote: > > nit: if you put this inside the remoting namespace below then you wouldn't > need > > remoting:: to reference GlRendererLayer. > > Then they are out of the anonymous namespace? I'm not quite understand in > general why we need to use anonymous namespace though... namespace remoting { namespace { void foo() { // Can access remoting namespace directly here. } } // namespace } // namespace remoting
https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_ren... File remoting/client/gl_render_layer.cc (right): https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_ren... remoting/client/gl_render_layer.cc:11: namespace { On 2016/07/21 20:14:15, Sergey Ulanov wrote: > On 2016/07/21 19:53:04, Yuwei wrote: > > On 2016/07/21 19:28:48, Sergey Ulanov wrote: > > > nit: if you put this inside the remoting namespace below then you wouldn't > > need > > > remoting:: to reference GlRendererLayer. > > > > Then they are out of the anonymous namespace? I'm not quite understand in > > general why we need to use anonymous namespace though... See https://google.github.io/styleguide/cppguide.html#Namespaces . It allows to avoid conflicts when you have a static functions in different files in global namespace. It's also to make it clear that these functions/constants are not used outside of this file.
Patchset #11 (id:220001) has been deleted
ptal https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_cur... File remoting/client/gl_cursor_feedback.cc (right): https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback.cc:18: const int kTextureId = 2; On 2016/07/21 19:28:48, Sergey Ulanov wrote: > maybe not for this CL: would it be useful to have a shared header define all > texture ids to avoid conflicts? Acknowledged. Will send another CL for this. https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_cur... File remoting/client/gl_cursor_feedback_texture.cc (right): https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.cc:31: uint32_t GetColorByRadius(float radius) { On 2016/07/21 20:14:15, Sergey Ulanov wrote: > On 2016/07/21 19:53:04, Yuwei wrote: > > On 2016/07/21 19:28:48, Sergey Ulanov wrote: > > > move to anonymous namespace. > > > > Also for FillFeedbackTexture? > > yes Done. https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_cur... File remoting/client/gl_cursor_feedback_texture.h (right): https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.h:15: class GlCursorFeedbackTexture { On 2016/07/21 19:28:48, Sergey Ulanov wrote: > add a comment to explain what this class is used for. Done. https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.h:18: const uint8_t* GetTexture() const; On 2016/07/21 19:28:48, Sergey Ulanov wrote: > empty line above this one Done. https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.h:19: int GetTextureDiameter() const; On 2016/07/21 19:28:48, Sergey Ulanov wrote: > nit: potentially these can be static functions as well, just to make this class > easier to use. Done. https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.h:19: int GetTextureDiameter() const; On 2016/07/21 20:10:46, Sergey Ulanov wrote: > On 2016/07/21 19:53:04, Yuwei wrote: > > On 2016/07/21 19:28:48, Sergey Ulanov wrote: > > > nit: potentially these can be static functions as well, just to make this > > class > > > easier to use. > > > > Maybe just a static const? > > Yes. But GetTexture() cannot be static const obviously. Acknowledged. https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.h:27: std::unique_ptr<uint32_t[]> texture_; On 2016/07/21 19:28:48, Sergey Ulanov wrote: > Maybe change this to std::vector<uint8_t> Then you can also return vector > reference from GetTexture, which means the consumer will also know the size of > the array. Done. https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_ren... File remoting/client/gl_render_layer.cc (right): https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_ren... remoting/client/gl_render_layer.cc:11: namespace { On 2016/07/21 19:28:48, Sergey Ulanov wrote: > nit: if you put this inside the remoting namespace below then you wouldn't need > remoting:: to reference GlRendererLayer. Done. https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_ren... remoting/client/gl_render_layer.cc:59: void PackDirtyRegion(uint8_t* dest, On 2016/07/21 19:28:48, Sergey Ulanov wrote: > Move this to the anonymous namespace. Done. https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_ren... File remoting/client/gl_render_layer.h (right): https://codereview.chromium.org/2148743005/diff/200001/remoting/client/gl_ren... remoting/client/gl_render_layer.h:19: enum { kBytesPerPixelRGB32 = 4 }; On 2016/07/21 19:28:48, Sergey Ulanov wrote: > Don't use enum for define constants. > Call it kBytesPerPixel please Done.
lgtm when my comments are addressed https://codereview.chromium.org/2148743005/diff/240001/remoting/client/gl_cur... File remoting/client/gl_cursor_feedback_texture.cc (right): https://codereview.chromium.org/2148743005/diff/240001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.cc:103: const int GlCursorFeedbackTexture::kTextureWidth = move this above GetInstance() https://codereview.chromium.org/2148743005/diff/240001/remoting/client/gl_cur... File remoting/client/gl_cursor_feedback_texture.h (right): https://codereview.chromium.org/2148743005/diff/240001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.h:23: static const int kTextureWidth; constants should precede everything else. Move this above GetInstance. https://google.github.io/styleguide/cppguide.html#Declaration_Order
https://codereview.chromium.org/2148743005/diff/240001/remoting/client/gl_cur... File remoting/client/gl_cursor_feedback_texture.h (right): https://codereview.chromium.org/2148743005/diff/240001/remoting/client/gl_cur... remoting/client/gl_cursor_feedback_texture.h:23: static const int kTextureWidth; On 2016/07/21 21:55:31, Sergey Ulanov wrote: > constants should precede everything else. Move this above GetInstance. > https://google.github.io/styleguide/cppguide.html#Declaration_Order Done.
The CQ bit was checked by yuweih@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/2148743005/#ps260001 (title: "Reviewer's Feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #12 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== [Remoting Android] Cursor & Cursor Feedback for OpenGL Renderer This CL implements classes for drawing cursor and cursor feedback on the OpenGL canvas. This is part of the project of using OpenGL to render remoting host on Android. BUG=385924 ========== to ========== [Remoting Android] Cursor & Cursor Feedback for OpenGL Renderer This CL implements classes for drawing cursor and cursor feedback on the OpenGL canvas. This is part of the project of using OpenGL to render remoting host on Android. BUG=385924 Committed: https://crrev.com/2495ff068c075749e36f20117db5d1c368ffa42e Cr-Commit-Position: refs/heads/master@{#406991} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/2495ff068c075749e36f20117db5d1c368ffa42e Cr-Commit-Position: refs/heads/master@{#406991} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
