Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2045963004:
[Remoting] OpenGL Rendering Layer (Closed)
|
Created:
4 years, 6 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] OpenGL Rendering Layer
This CL implements the OpenGL canvas, rendering layer, and desktop. This is part
of the project of implementing OpenGL rendering component for Android and iOS.
Please see design doc in crbug for more information.
BUG=385924
Committed: https://crrev.com/2c0936a2a4e515ea10c74e05022de6bc932089d2
Cr-Commit-Position: refs/heads/master@{#405278}
Patch Set 1 #
Total comments: 37
Patch Set 2 : Rebase ToT #Patch Set 3 : Reviewer's Feedback #Patch Set 4 : Rename opengl_wrapper to sys_opengl #Patch Set 5 : Rebase ToT #Patch Set 6 : Resolves some feedbacks #Patch Set 7 : Move OpenGL components out of the directory #Patch Set 8 : Inlines Shaders & Fixes build files #
Total comments: 47
Patch Set 9 : Reviewer's Feedback #
Total comments: 1
Patch Set 10 : sort of fix build error #
Total comments: 5
Patch Set 11 : Revert "sort of fix build error" #Patch Set 12 : Reviewer's Feedback #
Messages
Total messages: 81 (36 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
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/patch-status/2045963004/40001
Description was changed from ========== OpenGL Rendering Framework and Desktop Layer This CL implements the OpenGL basic rendering framework and the desktop layer. This is the first CL for the project of implementing OpenGL client rendering component. Doc: https://goo.gl/v549OW BUG=385924 ========== to ========== This CL implements the OpenGL render layer and the desktop layer. This is the first CL for the project of implementing OpenGL client rendering component. Doc: https://goo.gl/v549OW BUG=385924 ==========
Patchset #1 (id:40001) has been deleted
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/patch-status/2045963004/60001
yuweih@chromium.org changed reviewers: + sergeyu@chromium.org
PTAL. Please be careful since I am not very familiar with OpenGL stuffs :)
On 2016/06/08 17:50:51, Yuwei wrote: > PTAL. Please be careful since I am not very familiar with OpenGL stuffs :) Sorry, I should have noticed you have been implementing this change. Before you do any more work, I would review https://chromoting-internal.git.corp.google.com/chromoting-ios/+/master/ui/ and compare it to how you have implemented this. It seems more complicated in this CL then how iOS has implemented it.
On 2016/06/08 18:13:03, nicholss wrote: > On 2016/06/08 17:50:51, Yuwei wrote: > > PTAL. Please be careful since I am not very familiar with OpenGL stuffs :) > > Sorry, I should have noticed you have been implementing this change. Before you > do any more work, I would review > https://chromoting-internal.git.corp.google.com/chromoting-ios/+/master/ui/ and > compare it to how you have implemented this. It seems more complicated in this > CL then how iOS has implemented it. Also, be sure to add the gyp changes to the build. There are still gyp builders.
Haven't look at the details yet. But from a quick glance it appears that you are using a single texture for the whole screen. Problem is that texture size is usually limited, so it will not work for screens above certain size. The renderer we have on iOS splits the screen in to multiple textures to work around this issue. I'm not sure it's worth worrying about it now. AFAICT practically all modern devices, even the most crappy ones, support 4096x4096 which is the same limit that we have on the desktop. iPhone 4 is the only device I can think of that that matters and that has lower limit: 2048x2048. But I think we still need to detect that the screen size is too big and show some error message in that case. I'm very excited about getting better renderer for Android!
On 2016/06/08 18:13:03, nicholss wrote: > On 2016/06/08 17:50:51, Yuwei wrote: > > PTAL. Please be careful since I am not very familiar with OpenGL stuffs :) > > Sorry, I should have noticed you have been implementing this change. Before you > do any more work, I would review > https://chromoting-internal.git.corp.google.com/chromoting-ios/+/master/ui/ and > compare it to how you have implemented this. It seems more complicated in this > CL then how iOS has implemented it. Hmm... Looks like iOS is already using OpenGL to render the desktop :) It uses a lot of things in GLKit library that don't exist on Android... I'll compare this with the existing iOS code.
On 2016/06/08 18:13:03, nicholss wrote: > On 2016/06/08 17:50:51, Yuwei wrote: > > PTAL. Please be careful since I am not very familiar with OpenGL stuffs :) > > Sorry, I should have noticed you have been implementing this change. Before you > do any more work, I would review > https://chromoting-internal.git.corp.google.com/chromoting-ios/+/master/ui/ and > compare it to how you have implemented this. It seems more complicated in this > CL then how iOS has implemented it. It look be the the same level of complexity, just a different language.
On 2016/06/08 18:13:48, nicholss wrote: > On 2016/06/08 18:13:03, nicholss wrote: > > On 2016/06/08 17:50:51, Yuwei wrote: > > > PTAL. Please be careful since I am not very familiar with OpenGL stuffs :) > > > > Sorry, I should have noticed you have been implementing this change. Before > you > > do any more work, I would review > > https://chromoting-internal.git.corp.google.com/chromoting-ios/+/master/ui/ > and > > compare it to how you have implemented this. It seems more complicated in this > > CL then how iOS has implemented it. > > Also, be sure to add the gyp changes to the build. There are still gyp builders. Sorry that I always forgot the gyp changes since Android has already dropped the gyp support :P
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/06/08 18:45:22, Sergey Ulanov wrote: > Haven't look at the details yet. But from a quick glance it appears that you are > using a single texture for the whole screen. Problem is that texture size is > usually limited, so it will not work for screens above certain size. The > renderer we have on iOS splits the screen in to multiple textures to work around > this issue. > I'm not sure it's worth worrying about it now. AFAICT practically all modern > devices, even the most crappy ones, support 4096x4096 which is the same limit > that we have on the desktop. iPhone 4 is the only device I can think of that > that matters and that has lower limit: 2048x2048. But I think we still need to > detect that the screen size is too big and show some error message in that case. > I'm very excited about getting better renderer for Android! Wow. We are still supporting iPhone 4? I'm not sure how demanding we are to fix this problem. Logic for updating region across multiple textures and so on will be quite complex...
On 2016/06/08 20:16:12, Yuwei wrote: > On 2016/06/08 18:45:22, Sergey Ulanov wrote: > > Haven't look at the details yet. But from a quick glance it appears that you > are > > using a single texture for the whole screen. Problem is that texture size is > > usually limited, so it will not work for screens above certain size. The > > renderer we have on iOS splits the screen in to multiple textures to work > around > > this issue. > > I'm not sure it's worth worrying about it now. AFAICT practically all modern > > devices, even the most crappy ones, support 4096x4096 which is the same limit > > that we have on the desktop. iPhone 4 is the only device I can think of that > > that matters and that has lower limit: 2048x2048. But I think we still need to > > detect that the screen size is too big and show some error message in that > case. > > I'm very excited about getting better renderer for Android! > > Wow. We are still supporting iPhone 4? I'm not sure how demanding we are to fix > this problem. Logic for updating region across multiple textures and so on will > be quite complex... There will be some code changes. Please hold on until the next patch is uploaded :)
Patchset #2 (id:80001) has been deleted
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/patch-status/2045963004/100001
This CL is ready for review :)
Message was sent while issue was closed.
On 2016/06/21 22:18:48, Yuwei wrote: > This CL is ready for review :) Looks like I need to reconsider the current design. Currently there will be separate OpenGL program and transformation information stored for each render layer (desktop, cursor, etc.) Since all layers will basically go through the same zooming transformation and texture drawing procedure with slightly different vertex position configurations, I think it's better to just use one program and one zooming transformation to draw all layers. I'll reopen this CL when it's ready to be reviewed... Sorry for keep delaying review for this CL :-(
Message was sent while issue was closed.
Patchset #1 (id:60001) has been deleted
Message was sent while issue was closed.
Patchset #1 (id:100001) has been deleted
Message was sent while issue was closed.
Patchset #1 (id:120001) has been deleted
Message was sent while issue was closed.
Patchset #1 (id:140001) has been deleted
Message was sent while issue was closed.
Patchset #1 (id:160001) has been deleted
Message was sent while issue was closed.
Patchset #1 (id:180001) has been deleted
Message was sent while issue was closed.
Description was changed from ========== This CL implements the OpenGL render layer and the desktop layer. This is the first CL for the project of implementing OpenGL client rendering component. Doc: https://goo.gl/v549OW BUG=385924 ========== to ========== [Remoting] OpenGL Rendering Layer This CL implements the OpenGL canvas, rendering layer, and desktop. This is part of the project of implementing OpenGL rendering component for Android and iOS. Please see design doc in crbug for more information. BUG=385924 ==========
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...
On 2016/06/22 22:15:52, Yuwei wrote: > On 2016/06/21 22:18:48, Yuwei wrote: > > This CL is ready for review :) > > Looks like I need to reconsider the current design. > > Currently there will be separate OpenGL program and transformation information > stored for each render layer (desktop, cursor, etc.) Since all layers will > basically go through the same zooming transformation and texture drawing > procedure with slightly different vertex position configurations, I think it's > better to just use one program and one zooming transformation to draw all > layers. > > I'll reopen this CL when it's ready to be reviewed... Sorry for keep delaying > review for this CL :-( Done. PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Is it really necessary to create opengl directory? I don't think we usually create directories for small subcomponents like this. https://codereview.chromium.org/2045963004/diff/200001/remoting/android/BUILD.gn File remoting/android/BUILD.gn (right): https://codereview.chromium.org/2045963004/diff/200001/remoting/android/BUILD... remoting/android/BUILD.gn:28: "//remoting/client/opengl:opengl_renderer", remove ':opengl_renderer' https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... File remoting/client/opengl/BUILD.gn (right): https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/BUILD.gn:3: shaders = [ This file needs a copyright header. https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... File remoting/client/opengl/gl_canvas.cc (right): https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/gl_canvas.cc:5: #include "remoting/client/opengl/gl_canvas.h" please add empty line after this one https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/gl_canvas.cc:46: void GlCanvas::SetNormalizedTransformation(const float* matrix) { This is called only from the constructor. Do you need it as a public method? https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... File remoting/client/opengl/gl_canvas.h (right): https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/gl_canvas.h:18: // For more details, see the design doc: https://goo.gl/v549OW Please don't refer to design docs from the code. Design docs are rarely up to date, so such links will never be useful. https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/gl_canvas.h:33: // By default the transformation is a zero matrix. Why is it 0-matrix? wouldn't it make more sense to set it to identity-matrix instead? https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/gl_canvas.h:49: GLuint transform_loc_; it's not clear from the comment or name what this is. Call them *_location_ and explain in the comments that they are locations of the corresponding shader attributes. https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... File remoting/client/opengl/gl_desktop.cc (right): https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/gl_desktop.cc:37: layer_->SetTexture(frame->data(), frame->size().width(), Here you assume that the the frame lines are packed in DesktopFrame next to each other. That may not be true. There is GL_UNPACK_ROW_LENGTH attribute you need to set when calling glTexImage3D() to make it use correct stride. https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/gl_desktop.cc:37: layer_->SetTexture(frame->data(), frame->size().width(), It's best to avoid updating the whole texture every time. frame->update_region() contains information about parts of the screen that have changed since the previous frame. https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... File remoting/client/opengl/gl_desktop.h (right): https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/gl_desktop.h:17: class GlCanvas; add empty line here. https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... File remoting/client/opengl/gl_helpers.h (right): https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/gl_helpers.h:13: GLuint CompileShader(GLenum shaderType, const char* shaderSource); shader_type, shader_source https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/gl_helpers.h:13: GLuint CompileShader(GLenum shaderType, const char* shaderSource); add documentation for each of these functions https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... File remoting/client/opengl/tex_coord_to_view.vert (right): https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/tex_coord_to_view.vert:1: // Copyright 2016 The Chromium Authors. All rights reserved. Do you need to put shaders in separate files? Why not just put them as string consts in the C++ code, the way they are in the pepper renderer: https://codesearch.chromium.org/chromium/src/remoting/client/plugin/pepper_vi... https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/tex_coord_to_view.vert:27: trans_position.z = 0.0; I think all operations above can be represented as a single matrix multiplication with the following matrix ( 2, 0, -1, 0, -2, 1, 0, 0, 0) So the whole shader can be implemented in one line. But you can also adjust the coordinates in kVertices in gl_render_layer.cc. Here is the corresponding code in the Pepper renderer: https://codesearch.chromium.org/chromium/src/remoting/client/plugin/pepper_vi... https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... File remoting/client/opengl_wrapper.h (right): https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl_wrapper.h:5: #ifndef REMOTING_CLIENT_OPENGL_WRAPPER_H_ opengl_wrapper.h is confusing. Suggest calling this file sys_opengl.h . That's consistent with other similar files, e.g. see net/base/sys_addrinfo.h
https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... File remoting/client/opengl/gl_canvas.cc (right): https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/gl_canvas.cc:46: void GlCanvas::SetNormalizedTransformation(const float* matrix) { On 2016/06/27 23:15:29, Sergey Ulanov wrote: > This is called only from the constructor. Do you need it as a public method? Sorry that current CL doesn't show enough use case of this class... Actually this function will be used by GlRenderer (not in this CL) to change the pan and zoom configuration of the canvas. https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... File remoting/client/opengl/gl_canvas.h (right): https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/gl_canvas.h:33: // By default the transformation is a zero matrix. On 2016/06/27 23:15:29, Sergey Ulanov wrote: > Why is it 0-matrix? wouldn't it make more sense to set it to identity-matrix > instead? I don't have strong opinion... I made it 0-matrix so that nothing will be drawn if the cursor shape comes earlier than the desktop frame (which determines the dimension of the canvas), otherwise a huge cursor will be shown and stretched to the whole screen. https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... File remoting/client/opengl/gl_desktop.cc (right): https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/gl_desktop.cc:37: layer_->SetTexture(frame->data(), frame->size().width(), On 2016/06/27 23:15:29, Sergey Ulanov wrote: > Here you assume that the the frame lines are packed in DesktopFrame next to each > other. That may not be true. There is GL_UNPACK_ROW_LENGTH attribute you need to > set when calling glTexImage3D() to make it use correct stride. By using SoftwareVideoRenderer and SharedDesktopFrame it seems the lines are always packed next to each other... I had considered glPixelStorei(GL_UNPACK_ROW_LENGTH,stride) and only update changed regions but turns out GL_UNPACK_ROW_LENGTH isn't even supported in OpenGL ES 2. In this case we will have to upload texture line by line... Not sure whether we can use OpenGL ES 3 which supports less devices. It supports pixel buffer and we can draw the desktop directly onto the buffer. https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... File remoting/client/opengl/tex_coord_to_view.vert (right): https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/tex_coord_to_view.vert:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/06/27 23:15:30, Sergey Ulanov wrote: > Do you need to put shaders in separate files? Why not just put them as string > consts in the C++ code, the way they are in the pepper renderer: > https://codesearch.chromium.org/chromium/src/remoting/client/plugin/pepper_vi... I feel like it is more maintainable in this way... Actually I made a small script that puts shaders into a C header file. https://codereview.chromium.org/2036023003 https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/tex_coord_to_view.vert:27: trans_position.z = 0.0; On 2016/06/27 23:15:30, Sergey Ulanov wrote: > I think all operations above can be represented as a single matrix > multiplication with the following matrix > ( 2, 0, -1, > 0, -2, 1, > 0, 0, 0) > So the whole shader can be implemented in one line. That's a good point. Everything here is just linear operation :) > But you can also adjust the coordinates in kVertices in gl_render_layer.cc. Here > is the corresponding code in the Pepper renderer: > https://codesearch.chromium.org/chromium/src/remoting/client/plugin/pepper_vi... It's actually the same idea as in Pepper renderer. The first line defines the actual positions on the view and the second line defines the visible area of the texture. I am just trying to use the texture coordinate (x and y ranges [0, 1], very easy to translate from the pixel coordinate) for everything and do the transformation inside the shader.
On 2016/06/27 23:15:30, Sergey Ulanov wrote: > Is it really necessary to create opengl directory? I don't think we usually > create directories for small subcomponents like this. I'm not sure, but there may be more than 28 files in the directory... https://codereview.chromium.org/2032963002
Patchset #2 (id:220001) has been deleted
https://codereview.chromium.org/2045963004/diff/200001/remoting/android/BUILD.gn File remoting/android/BUILD.gn (right): https://codereview.chromium.org/2045963004/diff/200001/remoting/android/BUILD... remoting/android/BUILD.gn:28: "//remoting/client/opengl:opengl_renderer", On 2016/06/27 23:15:29, Sergey Ulanov wrote: > remove ':opengl_renderer' The folder is called `opengl` and the source set is `opengl_renderer`. Are you suggesting renaming the source set to `opengl` or renaming the folder to `opengl_renderer`? https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... File remoting/client/opengl/BUILD.gn (right): https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/BUILD.gn:3: shaders = [ On 2016/06/27 23:15:29, Sergey Ulanov wrote: > This file needs a copyright header. Done. https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... File remoting/client/opengl/gl_canvas.cc (right): https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/gl_canvas.cc:5: #include "remoting/client/opengl/gl_canvas.h" On 2016/06/27 23:15:29, Sergey Ulanov wrote: > please add empty line after this one Done. https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... File remoting/client/opengl/gl_canvas.h (right): https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/gl_canvas.h:18: // For more details, see the design doc: https://goo.gl/v549OW On 2016/06/27 23:15:29, Sergey Ulanov wrote: > Please don't refer to design docs from the code. Design docs are rarely up to > date, so such links will never be useful. Done. Removed. https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/gl_canvas.h:49: GLuint transform_loc_; On 2016/06/27 23:15:29, Sergey Ulanov wrote: > it's not clear from the comment or name what this is. Call them *_location_ and > explain in the comments that they are locations of the corresponding shader > attributes. Done. https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... File remoting/client/opengl/gl_desktop.h (right): https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/gl_desktop.h:17: class GlCanvas; On 2016/06/27 23:15:29, Sergey Ulanov wrote: > add empty line here. Done. https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... File remoting/client/opengl/gl_helpers.h (right): https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/gl_helpers.h:13: GLuint CompileShader(GLenum shaderType, const char* shaderSource); On 2016/06/27 23:15:30, Sergey Ulanov wrote: > add documentation for each of these functions Done. https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... File remoting/client/opengl_wrapper.h (right): https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl_wrapper.h:5: #ifndef REMOTING_CLIENT_OPENGL_WRAPPER_H_ On 2016/06/27 23:15:30, Sergey Ulanov wrote: > opengl_wrapper.h is confusing. Suggest calling this file sys_opengl.h . That's > consistent with other similar files, e.g. see net/base/sys_addrinfo.h Done.
https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... File remoting/client/opengl/gl_desktop.cc (right): https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/gl_desktop.cc:37: layer_->SetTexture(frame->data(), frame->size().width(), On 2016/06/27 23:15:29, Sergey Ulanov wrote: > It's best to avoid updating the whole texture every time. frame->update_region() > contains information about parts of the screen that have changed since the > previous frame. Turns out OpenGL ES 3 is backward compatible with ES 2 and you can decide which OpenGL client version should be used when creating the EGL context. Maybe implement PBO texture update for ES 3 and fall back to either updating row by row or constructing the full image in CPU when ES 3 is not supported?
On 2016/06/28 01:34:47, Yuwei wrote: > I'm not sure, but there may be more than 28 files in the directory... 28 files is not a lot :-) I don't think it justifies creation of a new directory https://codereview.chromium.org/2045963004/diff/200001/remoting/android/BUILD.gn File remoting/android/BUILD.gn (right): https://codereview.chromium.org/2045963004/diff/200001/remoting/android/BUILD... remoting/android/BUILD.gn:28: "//remoting/client/opengl:opengl_renderer", On 2016/06/28 22:51:11, Yuwei wrote: > On 2016/06/27 23:15:29, Sergey Ulanov wrote: > > remove ':opengl_renderer' > > The folder is called `opengl` and the source set is `opengl_renderer`. Are you > suggesting renaming the source set to `opengl` or renaming the folder to > `opengl_renderer`? I think we don't need the directory, which obsoletes this comment, sorry for confusion.
On 2016/06/29 23:26:17, Sergey Ulanov wrote: > > 28 files is not a lot :-) I don't think it justifies creation of a new directory Okay... I'll move them out. https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... File remoting/client/opengl/gl_desktop.cc (right): https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/gl_desktop.cc:37: layer_->SetTexture(frame->data(), frame->size().width(), On 2016/06/29 17:36:22, Yuwei wrote: > On 2016/06/27 23:15:29, Sergey Ulanov wrote: > > It's best to avoid updating the whole texture every time. > frame->update_region() > > contains information about parts of the screen that have changed since the > > previous frame. > > Turns out OpenGL ES 3 is backward compatible with ES 2 and you can decide which > OpenGL client version should be used when creating the EGL context. > > Maybe implement PBO texture update for ES 3 and fall back to either updating row > by row or constructing the full image in CPU when ES 3 is not supported? Just measured some render latencies when playing YouTube: * Use SharedDesktopFrame, updating entire frame, no lock, average 9ms. * Use BasicDesktopFrame & ES 3, update using GL_UNPACK_ROW_LENGTH, average 8ms. * Use BasicDesktopFrame & ES 2, update row by row, average 59ms :( I think using SharedDesktopFrame is mostly good enough? It is quite annoying that ES 2 doesn't support row length. I'll also take a look at wrapping PBO into DesktopFrame. Just have some questions: * Will SharedDesktopFrame be always tightly packed, and in what situation it won't? Is this host dependent? * Is it good to use SharedDesktopFrame without a lock? I'm not touching anything like dirty region so the only concern is sometimes you may see a frame being half-updated...
https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... File remoting/client/opengl/gl_canvas.h (right): https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/gl_canvas.h:33: // By default the transformation is a zero matrix. On 2016/06/28 01:31:20, Yuwei wrote: > On 2016/06/27 23:15:29, Sergey Ulanov wrote: > > Why is it 0-matrix? wouldn't it make more sense to set it to identity-matrix > > instead? > > I don't have strong opinion... I made it 0-matrix so that nothing will be drawn > if the cursor shape comes earlier than the desktop frame (which determines the > dimension of the canvas), otherwise a huge cursor will be shown and stretched to > the whole screen. I guess even if it's 0-matrix it doesn't mean nothing is drawn on the screen. All coordinates will collapse to (0,0), so you still get one pixel drawn there, no? Maybe it's better to require that this function is always called before DrawTexture() and add a DCHECK() for that. https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... File remoting/client/opengl/gl_desktop.cc (right): https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/gl_desktop.cc:37: layer_->SetTexture(frame->data(), frame->size().width(), On 2016/06/30 00:04:55, Yuwei wrote: > On 2016/06/29 17:36:22, Yuwei wrote: > > On 2016/06/27 23:15:29, Sergey Ulanov wrote: > > > It's best to avoid updating the whole texture every time. > > frame->update_region() > > > contains information about parts of the screen that have changed since the > > > previous frame. > > > > Turns out OpenGL ES 3 is backward compatible with ES 2 and you can decide > which > > OpenGL client version should be used when creating the EGL context. > > > > Maybe implement PBO texture update for ES 3 and fall back to either updating > row > > by row or constructing the full image in CPU when ES 3 is not supported? > > Just measured some render latencies when playing YouTube: > > * Use SharedDesktopFrame, updating entire frame, no lock, average 9ms. > * Use BasicDesktopFrame & ES 3, update using GL_UNPACK_ROW_LENGTH, average 8ms. Is this when updating the whole frame? I think the difference between these two would be much larger for partial frame updates. > * Use BasicDesktopFrame & ES 2, update row by row, average 59ms :( With ES2 it's probably better to copy all changed rows into a tightly packed buffer and update the texture from there, repeat for each updated rect. That should be faster for partial updates. > > I think using SharedDesktopFrame is mostly good enough? It is quite annoying > that ES 2 doesn't support row length. I'll also take a look at wrapping PBO into > DesktopFrame. Are you suggesting that we use a single frame buffer and keep using it for every frame? If yes, then this should work, but it will be slower for partial updates. > > Just have some questions: > > * Will SharedDesktopFrame be always tightly packed, and in what situation it > won't? Is this host dependent? It's determined by the frame type inside SharedDesktopFrame. BasicDesktopFrame() is always packed. So yes, we can make sure that it's always packed as long as we use BasicDesktopFrame(). > * Is it good to use SharedDesktopFrame without a lock? I'm not touching anything > like dirty region so the only concern is sometimes you may see a frame being > half-updated... Correct, as long as you own the SharedDesktopFrame object. SharedDesktopFrame instances share only the underlying buffer. This may cause artifacts when decoding to the same buffer on a different thread, but no issues otherwise. https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... File remoting/client/opengl/tex_coord_to_view.vert (right): https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/tex_coord_to_view.vert:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/06/28 01:31:20, Yuwei wrote: > On 2016/06/27 23:15:30, Sergey Ulanov wrote: > > Do you need to put shaders in separate files? Why not just put them as string > > consts in the C++ code, the way they are in the pepper renderer: > > > https://codesearch.chromium.org/chromium/src/remoting/client/plugin/pepper_vi... > > I feel like it is more maintainable in this way... Actually I made a small > script that puts shaders into a C header file. > > https://codereview.chromium.org/2036023003 This is great stuff, but I don't think we need it. I see these issues: 1. readability: Usually shader and the code are coupled together and need to be looked at together. Separating them makes it harder to understand the code. Shaders are akin functions (that run on GPU), and we don't put every single function in a separate file. 2. discoverability: this is a non-standard way to use shaders. There is no integration with code-search, so it would be hard for someone to figure out where the shaders are defined, unless they are already familiar with it. 3. build complexity: every custom action like this makes it harder to maintain our build (also making it a tad slower). So overall this approach adds a lot of complexity with no real benefits. I can see how separating shaders might be useful if we did some kind of pre-processing on them (e.g. type-checking). But we don't do any of it and I don't think it's worth adding this complexity for the few shaders we will need.
On 2016/06/30 00:34:52, Sergey Ulanov wrote: > https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... > File remoting/client/opengl/gl_canvas.h (right): > > https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... > remoting/client/opengl/gl_canvas.h:33: // By default the transformation is a > zero matrix. > On 2016/06/28 01:31:20, Yuwei wrote: > > On 2016/06/27 23:15:29, Sergey Ulanov wrote: > > > Why is it 0-matrix? wouldn't it make more sense to set it to identity-matrix > > > instead? > > > > I don't have strong opinion... I made it 0-matrix so that nothing will be > drawn > > if the cursor shape comes earlier than the desktop frame (which determines the > > dimension of the canvas), otherwise a huge cursor will be shown and stretched > to > > the whole screen. > > I guess even if it's 0-matrix it doesn't mean nothing is drawn on the screen. > All coordinates will collapse to (0,0), so you still get one pixel drawn there, > no? Not sure whether that is true since it is the view coordinates being used which is in range [-1,1], not in pixel coordinates. A polygon with 0 length edges probably won't be drawn on the screen? > > Maybe it's better to require that this function is always called before > DrawTexture() and add a DCHECK() for that. I think this is better :) > > https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... > File remoting/client/opengl/gl_desktop.cc (right): > > https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... > remoting/client/opengl/gl_desktop.cc:37: layer_->SetTexture(frame->data(), > frame->size().width(), > On 2016/06/30 00:04:55, Yuwei wrote: > > On 2016/06/29 17:36:22, Yuwei wrote: > > > On 2016/06/27 23:15:29, Sergey Ulanov wrote: > > > > It's best to avoid updating the whole texture every time. > > > frame->update_region() > > > > contains information about parts of the screen that have changed since the > > > > previous frame. > > > > > > Turns out OpenGL ES 3 is backward compatible with ES 2 and you can decide > > which > > > OpenGL client version should be used when creating the EGL context. > > > > > > Maybe implement PBO texture update for ES 3 and fall back to either updating > > row > > > by row or constructing the full image in CPU when ES 3 is not supported? > > > > Just measured some render latencies when playing YouTube: > > > > * Use SharedDesktopFrame, updating entire frame, no lock, average 9ms. > > * Use BasicDesktopFrame & ES 3, update using GL_UNPACK_ROW_LENGTH, average > 8ms. > > Is this when updating the whole frame? I think the difference between these two > would be much larger for partial frame updates. No. This is iterating through dirty regions... I guess this is because of the overhead of creating BasicDesktopFrame for each frame received. > > > > * Use BasicDesktopFrame & ES 2, update row by row, average 59ms :( > > With ES2 it's probably better to copy all changed rows into a tightly packed > buffer and update the texture from there, repeat for each updated rect. That > should be faster for partial updates. I'll try this. Is this eventually row by row memcpy? > > > > > > I think using SharedDesktopFrame is mostly good enough? It is quite annoying > > that ES 2 doesn't support row length. I'll also take a look at wrapping PBO > into > > DesktopFrame. > > Are you suggesting that we use a single frame buffer and keep using it for every > frame? > If yes, then this should work, but it will be slower for partial updates. Yes. If we don't do partial updates then this will be good. If we do partial updates then I think it can still reduce the overhead of creating BasicDesktopFrame for each update. The problem is just to get the dirty regions synchronized. Maybe I can modify SoftwareVideoRenderer to get a snapshot of the dirty regions on the decode thread when decoding is done? Anyway. Looks like using OpenGL for rendering can cut down the rendering time by half (from 21ms to 9ms texture upload + <=1ms actual drawing). The most latency still comes from decoding (>30ms)... > > > > > > Just have some questions: > > > > * Will SharedDesktopFrame be always tightly packed, and in what situation it > > won't? Is this host dependent? > > It's determined by the frame type inside SharedDesktopFrame. BasicDesktopFrame() > is always packed. So yes, we can make sure that it's always packed as long as we > use BasicDesktopFrame(). Yes. I'm using BasicDesktopFrame > > > > * Is it good to use SharedDesktopFrame without a lock? I'm not touching > anything > > like dirty region so the only concern is sometimes you may see a frame being > > half-updated... > > Correct, as long as you own the SharedDesktopFrame object. SharedDesktopFrame > instances share only the underlying buffer. This may cause artifacts when > decoding to the same buffer on a different thread, but no issues otherwise. Now I need synchronization for the dirty regions :) > > https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... > File remoting/client/opengl/tex_coord_to_view.vert (right): > > https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... > remoting/client/opengl/tex_coord_to_view.vert:1: // Copyright 2016 The Chromium > Authors. All rights reserved. > On 2016/06/28 01:31:20, Yuwei wrote: > > On 2016/06/27 23:15:30, Sergey Ulanov wrote: > > > Do you need to put shaders in separate files? Why not just put them as > string > > > consts in the C++ code, the way they are in the pepper renderer: > > > > > > https://codesearch.chromium.org/chromium/src/remoting/client/plugin/pepper_vi... > > > > I feel like it is more maintainable in this way... Actually I made a small > > script that puts shaders into a C header file. > > > > https://codereview.chromium.org/2036023003 > > This is great stuff, but I don't think we need it. I see these issues: > 1. readability: Usually shader and the code are coupled together and need to be > looked at together. Separating them makes it harder to understand the code. > Shaders are akin functions (that run on GPU), and we don't put every single > function in a separate file. > 2. discoverability: this is a non-standard way to use shaders. There is no > integration with code-search, so it would be hard for someone to figure out > where the shaders are defined, unless they are already familiar with it. > 3. build complexity: every custom action like this makes it harder to maintain > our build (also making it a tad slower). > > So overall this approach adds a lot of complexity with no real benefits. I can > see how separating shaders might be useful if we did some kind of pre-processing > on them (e.g. type-checking). But we don't do any of it and I don't think it's > worth adding this complexity for the few shaders we will need. Okay. I will write them inside the cc file.
Just did an experiment for partial update vs full update and got quite confusing result. Setup: Variable 1: Playing Youtube video in full screen (left of the spreadsheet) vs playing the same Youtube video in a small window that shows only a small part of it (right). Variable 2: * Manual packing (For OpenGL ES 2): Tightly pack the updated regions in CPU side (into a reusable buffer scoped to the life time of the object) and use glTexSubImage to update the packed updated region. * Auto packing (For OpenGL ES 3): Use glTexSubImage with the correct stride setting. Packing is handled by the API. * Full update: Use glTexSubImage but update the whole desktop frame (frame->data()) instead of just dirty regions. Constant: Use SharedDesktopFrame with BasicDesktopFrame as the underlying object. No locks. Result: https://goo.gl/qawlr4 Which is a little bit confusing: * There is no evidence that only updating dirty regions is better than always updating the whole desktop frame. * The performance of updating the whole desktop frame is actually dependent on the size of the dirty region (variable 1). I don't have a reasonable theory for this. Maybe the API implementation is doing some clever things?
Are these numbers in milliseconds? What is being measured? Does the test flush gl buffers and is that included in the result? It also can be that the time you measure includes vsync for each frame, which would affect numbers you get. I think a more reliable test would be to measure maximum FPS. You don't really need to run youtube on the host. Just make the client update the texture as quickly as possible from the frame it has. It can use static frame for that. On 2016/07/06 20:27:13, Yuwei wrote: > Just did an experiment for partial update vs full update and got quite > confusing result. > > Setup: > > Variable 1: Playing Youtube video in full screen (left of the spreadsheet) > vs playing the same Youtube video in a small window that shows only a small > part of it (right). > > Variable 2: > * Manual packing (For OpenGL ES 2): Tightly pack the updated regions in CPU > side (into a reusable buffer scoped to the life time of the object) and use > glTexSubImage to update the packed updated region. > * Auto packing (For OpenGL ES 3): Use glTexSubImage with the correct stride > setting. Packing is handled by the API. > * Full update: Use glTexSubImage but update the whole desktop frame > (frame->data()) instead of just dirty regions. > > Constant: Use SharedDesktopFrame with BasicDesktopFrame as the underlying > object. No locks. > > Result: > https://goo.gl/qawlr4 > > Which is a little bit confusing: > > * There is no evidence that only updating dirty regions is better than always > updating the whole desktop frame. > * The performance of updating the whole desktop frame is actually dependent > on the size of the dirty region (variable 1). I don't have a reasonable > theory for this. Maybe the API implementation is doing some clever things?
On 2016/07/06 20:53:32, Sergey Ulanov wrote:
> Are these numbers in milliseconds?
Yes, in milliseconds.
> What is being measured?
Texture uploading time. Namely the glTexSubImage2d calls
TimeTicks start = Now();
glTexSubImage2d(...);
LOG(INFO) << (Now() - start).InMilliseconds();
or
TimeTicks start = Now();
for (...) {
...
glTexSubImage2d(...);
}
LOG(INFO) << (Now() - start).InMilliseconds();
depending on the implementation.
> Does the test flush gl buffers and is that included in the result?
I don't think so. Texture uploading and rendering are different logic and
I'm just measuring the uploading time. Rendering takes about 5ms in
average.
> It also can be that the time you measure includes vsync for each
> frame, which would affect numbers you get.
>
> I think a more reliable test would be to measure maximum FPS.
So the rendering time should be included in the result?
> You don't really need to run youtube on the host. Just make the client
> update the texture as quickly as possible from the frame it has. It can
> use static frame for that.
That's a reasonable setup :) I may expect zero upload time if the
performance of full update is really dependent on the size of the dirty
region...
>
> On 2016/07/06 20:27:13, Yuwei wrote:
> > Just did an experiment for partial update vs full update and got quite
> > confusing result.
> >
> > Setup:
> >
> > Variable 1: Playing Youtube video in full screen (left of the spreadsheet)
> > vs playing the same Youtube video in a small window that shows only a small
> > part of it (right).
> >
> > Variable 2:
> > * Manual packing (For OpenGL ES 2): Tightly pack the updated regions in CPU
> > side (into a reusable buffer scoped to the life time of the object) and use
> > glTexSubImage to update the packed updated region.
> > * Auto packing (For OpenGL ES 3): Use glTexSubImage with the correct stride
> > setting. Packing is handled by the API.
> > * Full update: Use glTexSubImage but update the whole desktop frame
> > (frame->data()) instead of just dirty regions.
> >
> > Constant: Use SharedDesktopFrame with BasicDesktopFrame as the underlying
> > object. No locks.
> >
> > Result:
> > https://goo.gl/qawlr4
> >
> > Which is a little bit confusing:
> >
> > * There is no evidence that only updating dirty regions is better than
always
> > updating the whole desktop frame.
> > * The performance of updating the whole desktop frame is actually dependent
> > on the size of the dirty region (variable 1). I don't have a reasonable
> > theory for this. Maybe the API implementation is doing some clever things?
Just tried a simplified experiment that only draws a red texture
(2048*2048 filled with color (0,0,255,255)) on the screen. And I use
a simple loop to do measuring and rendering:
test.SetColor(0, 0, 255);
while (true) {
base::TimeTicks start = base::TimeTicks::Now();
test.??????(); // Update texture here.
LOG(INFO) << "Update ms: " << (base::TimeTicks::Now() -
start).InMilliseconds();
test.Draw();
egl_context_->SwapBuffers();
base::TimeTicks now = base::TimeTicks::Now();
frame_count++;
if ((now - fps_time).InMilliseconds() > 1000) {
LOG(INFO) << "FPS: " << ((float) frame_count)/(now -
fps_time).InMilliseconds() * 1000.f;
fps_time = now;
frame_count = 0;
}
usleep(500);
}
And here is the result:
* It takes 13~16ms to update the whole texture.
* It takes 0~1ms to update a 150px*150px sub region.
* It takes 59~63ms to use a lot of 16*16 blocks to update the
whole texture.
* It takes 58~59ms to use 16*16 blocks to update the texture but
pack them in CPU side. No idea why it is faster than letting API
to pack the blocks.
* If I try to measure the time for packing and uploading separately,
packing in CPU side takes about 15ms and uploading packed texture
takes about 60ms.
* FPS is at most 60. Looks like vsync happens when swapping the
buffer.
The test suggests that calling glTexSubimage2d too many times will
add up significant overhead. I'm not sure whether we threshold the
number or the size of updated regions.
I have also double checked with my host. Looks like I always get
only one updated region with the size of the whole screen no matter
how large the actual updated region is, so the previous test doesn't
help since the updated region wasn't even a variable O_o
On 2016/07/07 01:12:15, Yuwei wrote:
> Just tried a simplified experiment that only draws a red texture
> (2048*2048 filled with color (0,0,255,255)) on the screen. And I use
> a simple loop to do measuring and rendering:
>
> test.SetColor(0, 0, 255);
> while (true) {
> base::TimeTicks start = base::TimeTicks::Now();
> test.??????(); // Update texture here.
> LOG(INFO) << "Update ms: " << (base::TimeTicks::Now() -
> start).InMilliseconds();
> test.Draw();
> egl_context_->SwapBuffers();
> base::TimeTicks now = base::TimeTicks::Now();
> frame_count++;
> if ((now - fps_time).InMilliseconds() > 1000) {
> LOG(INFO) << "FPS: " << ((float) frame_count)/(now -
> fps_time).InMilliseconds() * 1000.f;
> fps_time = now;
> frame_count = 0;
> }
> usleep(500);
> }
>
> And here is the result:
>
> * It takes 13~16ms to update the whole texture.
> * It takes 0~1ms to update a 150px*150px sub region.
> * It takes 59~63ms to use a lot of 16*16 blocks to update the
> whole texture.
> * It takes 58~59ms to use 16*16 blocks to update the texture but
> pack them in CPU side. No idea why it is faster than letting API
> to pack the blocks.
> * If I try to measure the time for packing and uploading separately,
> packing in CPU side takes about 15ms and uploading packed texture
> takes about 60ms.
> * FPS is at most 60. Looks like vsync happens when swapping the
> buffer.
>
> The test suggests that calling glTexSubimage2d too many times will
> add up significant overhead. I'm not sure whether we threshold the
> number or the size of updated regions.
>
> I have also double checked with my host. Looks like I always get
> only one updated region with the size of the whole screen no matter
> how large the actual updated region is, so the previous test doesn't
> help since the updated region wasn't even a variable O_o
Tried playing youtube video in window mode with a Mac host and I got
reasonable result.
Full update: 16.11ms
Partial update packed by API: 9.21ms
Partial update packed manually: 11.42ms
I'll go ahead and implement partial update and use manual packing
as fallback for OpenGL ES 2 :)
ptal https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... File remoting/client/opengl/gl_canvas.h (right): https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/gl_canvas.h:33: // By default the transformation is a zero matrix. On 2016/06/30 00:34:52, Sergey Ulanov wrote: > On 2016/06/28 01:31:20, Yuwei wrote: > > On 2016/06/27 23:15:29, Sergey Ulanov wrote: > > > Why is it 0-matrix? wouldn't it make more sense to set it to identity-matrix > > > instead? > > > > I don't have strong opinion... I made it 0-matrix so that nothing will be > drawn > > if the cursor shape comes earlier than the desktop frame (which determines the > > dimension of the canvas), otherwise a huge cursor will be shown and stretched > to > > the whole screen. > > I guess even if it's 0-matrix it doesn't mean nothing is drawn on the screen. > All coordinates will collapse to (0,0), so you still get one pixel drawn there, > no? > > Maybe it's better to require that this function is always called before > DrawTexture() and add a DCHECK() for that. Just made it draw nothing if the transformation matrix is not set. DCHECK may be too strict for the use case. There is no guarantee that a transformation will be set before the desktop frame is received. https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... File remoting/client/opengl/gl_desktop.cc (right): https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/gl_desktop.cc:37: layer_->SetTexture(frame->data(), frame->size().width(), On 2016/06/30 00:34:52, Sergey Ulanov wrote: > On 2016/06/30 00:04:55, Yuwei wrote: > > On 2016/06/29 17:36:22, Yuwei wrote: > > > On 2016/06/27 23:15:29, Sergey Ulanov wrote: > > > > It's best to avoid updating the whole texture every time. > > > frame->update_region() > > > > contains information about parts of the screen that have changed since the > > > > previous frame. > > > > > > Turns out OpenGL ES 3 is backward compatible with ES 2 and you can decide > > which > > > OpenGL client version should be used when creating the EGL context. > > > > > > Maybe implement PBO texture update for ES 3 and fall back to either updating > > row > > > by row or constructing the full image in CPU when ES 3 is not supported? > > > > Just measured some render latencies when playing YouTube: > > > > * Use SharedDesktopFrame, updating entire frame, no lock, average 9ms. > > * Use BasicDesktopFrame & ES 3, update using GL_UNPACK_ROW_LENGTH, average > 8ms. > > Is this when updating the whole frame? I think the difference between these two > would be much larger for partial frame updates. > > > > * Use BasicDesktopFrame & ES 2, update row by row, average 59ms :( > > With ES2 it's probably better to copy all changed rows into a tightly packed > buffer and update the texture from there, repeat for each updated rect. That > should be faster for partial updates. > > > > > > I think using SharedDesktopFrame is mostly good enough? It is quite annoying > > that ES 2 doesn't support row length. I'll also take a look at wrapping PBO > into > > DesktopFrame. > > Are you suggesting that we use a single frame buffer and keep using it for every > frame? > If yes, then this should work, but it will be slower for partial updates. > > > > > > Just have some questions: > > > > * Will SharedDesktopFrame be always tightly packed, and in what situation it > > won't? Is this host dependent? > > It's determined by the frame type inside SharedDesktopFrame. BasicDesktopFrame() > is always packed. So yes, we can make sure that it's always packed as long as we > use BasicDesktopFrame(). > > > > * Is it good to use SharedDesktopFrame without a lock? I'm not touching > anything > > like dirty region so the only concern is sometimes you may see a frame being > > half-updated... > > Correct, as long as you own the SharedDesktopFrame object. SharedDesktopFrame > instances share only the underlying buffer. This may cause artifacts when > decoding to the same buffer on a different thread, but no issues otherwise. Done. https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... File remoting/client/opengl/tex_coord_to_view.vert (right): https://codereview.chromium.org/2045963004/diff/200001/remoting/client/opengl... remoting/client/opengl/tex_coord_to_view.vert:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/06/30 00:34:52, Sergey Ulanov wrote: > On 2016/06/28 01:31:20, Yuwei wrote: > > On 2016/06/27 23:15:30, Sergey Ulanov wrote: > > > Do you need to put shaders in separate files? Why not just put them as > string > > > consts in the C++ code, the way they are in the pepper renderer: > > > > > > https://codesearch.chromium.org/chromium/src/remoting/client/plugin/pepper_vi... > > > > I feel like it is more maintainable in this way... Actually I made a small > > script that puts shaders into a C header file. > > > > https://codereview.chromium.org/2036023003 > > This is great stuff, but I don't think we need it. I see these issues: > 1. readability: Usually shader and the code are coupled together and need to be > looked at together. Separating them makes it harder to understand the code. > Shaders are akin functions (that run on GPU), and we don't put every single > function in a separate file. > 2. discoverability: this is a non-standard way to use shaders. There is no > integration with code-search, so it would be hard for someone to figure out > where the shaders are defined, unless they are already familiar with it. > 3. build complexity: every custom action like this makes it harder to maintain > our build (also making it a tad slower). > > So overall this approach adds a lot of complexity with no real benefits. I can > see how separating shaders might be useful if we did some kind of pre-processing > on them (e.g. type-checking). But we don't do any of it and I don't think it's > worth adding this complexity for the few shaders we will need. Done. Moved shaders into GlCanvas.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2045963004/diff/360001/remoting/client/BUILD.gn File remoting/client/BUILD.gn (right): https://codereview.chromium.org/2045963004/diff/360001/remoting/client/BUILD.... remoting/client/BUILD.gn:46: "//remoting/client/gl_canvas.cc", Don't need full paths here. https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_can... File remoting/client/gl_canvas.cc (right): https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_can... remoting/client/gl_canvas.cc:16: "// Region of the texture to be used (normally the whole texture).\n" nit: comments can be kept outside of the string. We don't need the comments embedded in the binaries we ship. https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_can... remoting/client/gl_canvas.cc:51: " // when uploading the pixel and swap b and r in the frag shader.\n" The decoder can generate frames in both formats, see https://codereview.chromium.org/2088953004, so you may not need this anymore. https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_can... File remoting/client/gl_canvas.h (right): https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_can... remoting/client/gl_canvas.h:44: void SetNormalizedTransformation(const float* matrix); nit: would be more typesafe to use std::array<float, 9> here. https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_des... File remoting/client/gl_desktop.cc (right): https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_des... remoting/client/gl_desktop.cc:15: const int kBytesPerPixel = 4; use webrtc::DesktopFrame::kBytesPerPixel https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_des... remoting/client/gl_desktop.cc:17: } // namespace https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_des... remoting/client/gl_desktop.cc:39: if (!last_frame_ || frame->size().width() != last_frame_->size().width() || !frame->size().equal(last_frame_->size()) to avoid comparing width and height separately. https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_des... remoting/client/gl_desktop.cc:47: reinterpret_cast<const uint8_t*>(frame->data()) + Don't need this cast. https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_des... remoting/client/gl_desktop.cc:48: frame->stride() * i.rect().top() + i.rect().left() * kBytesPerPixel; Use DesktopFrame::GetFrameDataAtPos(i.rect().top_left()); https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_des... remoting/client/gl_desktop.cc:51: frame->stride() / kBytesPerPixel); stride() may not be multiple of kBytesPerPixel. Pass stride as it is, in bytes? https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_ren... File remoting/client/gl_render_layer.cc (right): https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_ren... remoting/client/gl_render_layer.cc:58: void GlRenderLayer::UpdateTexture(const void* subtexture, Use uint8_t* for the buffer? https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_ren... remoting/client/gl_render_layer.cc:65: CHECK(texture_set_); Do you really need CHECK() instead of DCHECK()? https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_ren... remoting/client/gl_render_layer.cc:66: CHECK(width > 0 && height > 0); DCHECK? https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_ren... remoting/client/gl_render_layer.cc:88: PackDirtyRegion(update_buffer_.get(), subtexture, width, height, This can be avoided when stride == pixel_size*width. Add a special case for this? https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_ren... remoting/client/gl_render_layer.cc:124: remove extra empty line https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_ren... remoting/client/gl_render_layer.cc:126: void GlRenderLayer::PackDirtyRegion(void* dest, Doesn't need to be class member. https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_ren... remoting/client/gl_render_layer.cc:133: reinterpret_cast<const uint8_t*>(source) + kBytesPerPixel * stride * i; You wouldn't need this cast with uint8_t* used for arguments instead of void*. https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_ren... remoting/client/gl_render_layer.cc:135: reinterpret_cast<uint8_t*>(dest) + kBytesPerPixel * width * i; Instead of using multiplication to calculate position you could add stride after copying each line: dest += kBytesPerPixel * width; source += stride; This makes code cleaner and potentially slightly faster. https://codereview.chromium.org/2045963004/diff/360001/remoting/client/opengl... File remoting/client/opengl_renderer.gyp (right): https://codereview.chromium.org/2045963004/diff/360001/remoting/client/opengl... remoting/client/opengl_renderer.gyp:1: # Copyright (c) 2016 Google Inc. All rights reserved. Do we still need to update GYP? According to crbug.com/359249 GN migration is complete for Android
https://codereview.chromium.org/2045963004/diff/360001/remoting/client/BUILD.gn File remoting/client/BUILD.gn (right): https://codereview.chromium.org/2045963004/diff/360001/remoting/client/BUILD.... remoting/client/BUILD.gn:44: source_set("opengl_renderer") { BTW is there a way to stop non-android trybots from compiling this source set? https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_can... File remoting/client/gl_canvas.h (right): https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_can... remoting/client/gl_canvas.h:44: void SetNormalizedTransformation(const float* matrix); On 2016/07/09 01:01:20, Sergey Ulanov wrote: > nit: would be more typesafe to use std::array<float, 9> here. Just curious will there be performance drawback compared to C array? https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_des... File remoting/client/gl_desktop.cc (right): https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_des... remoting/client/gl_desktop.cc:51: frame->stride() / kBytesPerPixel); On 2016/07/09 01:01:20, Sergey Ulanov wrote: > stride() may not be multiple of kBytesPerPixel. Pass stride as it is, in bytes? Hmm... The problem is GL_UNPACK_ROW_LENGTH expects stride in pixel. Maybe in UpdateTexture, if |stride| is not multiple of kBytesPerPixel, then manually pack the updated region? https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_ren... File remoting/client/gl_render_layer.cc (right): https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_ren... remoting/client/gl_render_layer.cc:88: PackDirtyRegion(update_buffer_.get(), subtexture, width, height, On 2016/07/09 01:01:20, Sergey Ulanov wrote: > This can be avoided when stride == pixel_size*width. Add a special case for > this? Isn't it covered by line 70? https://codereview.chromium.org/2045963004/diff/360001/remoting/client/opengl... File remoting/client/opengl_renderer.gyp (right): https://codereview.chromium.org/2045963004/diff/360001/remoting/client/opengl... remoting/client/opengl_renderer.gyp:1: # Copyright (c) 2016 Google Inc. All rights reserved. On 2016/07/09 01:01:21, Sergey Ulanov wrote: > Do we still need to update GYP? > According to crbug.com/359249 GN migration is complete for Android This is for iOS.
Patchset #9 (id:380001) has been deleted
PTAL https://codereview.chromium.org/2045963004/diff/360001/remoting/client/BUILD.gn File remoting/client/BUILD.gn (right): https://codereview.chromium.org/2045963004/diff/360001/remoting/client/BUILD.... remoting/client/BUILD.gn:46: "//remoting/client/gl_canvas.cc", On 2016/07/09 01:01:20, Sergey Ulanov wrote: > Don't need full paths here. Done. https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_can... File remoting/client/gl_canvas.cc (right): https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_can... remoting/client/gl_canvas.cc:51: " // when uploading the pixel and swap b and r in the frag shader.\n" On 2016/07/09 01:01:20, Sergey Ulanov wrote: > The decoder can generate frames in both formats, see > https://codereview.chromium.org/2088953004, so you may not need this anymore. Done. https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_can... File remoting/client/gl_canvas.h (right): https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_can... remoting/client/gl_canvas.h:44: void SetNormalizedTransformation(const float* matrix); On 2016/07/09 01:01:20, Sergey Ulanov wrote: > nit: would be more typesafe to use std::array<float, 9> here. Done. Also use std::array everywhere else. https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_des... File remoting/client/gl_desktop.cc (right): https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_des... remoting/client/gl_desktop.cc:15: const int kBytesPerPixel = 4; On 2016/07/09 01:01:20, Sergey Ulanov wrote: > use webrtc::DesktopFrame::kBytesPerPixel Done. https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_des... remoting/client/gl_desktop.cc:17: } On 2016/07/09 01:01:20, Sergey Ulanov wrote: > // namespace Done. https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_des... remoting/client/gl_desktop.cc:39: if (!last_frame_ || frame->size().width() != last_frame_->size().width() || On 2016/07/09 01:01:20, Sergey Ulanov wrote: > !frame->size().equal(last_frame_->size()) > to avoid comparing width and height separately. Done. https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_des... remoting/client/gl_desktop.cc:47: reinterpret_cast<const uint8_t*>(frame->data()) + On 2016/07/09 01:01:20, Sergey Ulanov wrote: > Don't need this cast. Done. https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_ren... File remoting/client/gl_render_layer.cc (right): https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_ren... remoting/client/gl_render_layer.cc:58: void GlRenderLayer::UpdateTexture(const void* subtexture, On 2016/07/09 01:01:20, Sergey Ulanov wrote: > Use uint8_t* for the buffer? Done. https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_ren... remoting/client/gl_render_layer.cc:65: CHECK(texture_set_); On 2016/07/09 01:01:20, Sergey Ulanov wrote: > Do you really need CHECK() instead of DCHECK()? Not sure when CHECK() will be useful... https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_ren... remoting/client/gl_render_layer.cc:66: CHECK(width > 0 && height > 0); On 2016/07/09 01:01:20, Sergey Ulanov wrote: > DCHECK? Done. https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_ren... remoting/client/gl_render_layer.cc:124: On 2016/07/09 01:01:20, Sergey Ulanov wrote: > remove extra empty line Obsolete. https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_ren... remoting/client/gl_render_layer.cc:126: void GlRenderLayer::PackDirtyRegion(void* dest, On 2016/07/09 01:01:20, Sergey Ulanov wrote: > Doesn't need to be class member. Done. Made it inline function. https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_ren... remoting/client/gl_render_layer.cc:133: reinterpret_cast<const uint8_t*>(source) + kBytesPerPixel * stride * i; On 2016/07/09 01:01:20, Sergey Ulanov wrote: > You wouldn't need this cast with uint8_t* used for arguments instead of void*. Done. https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_ren... remoting/client/gl_render_layer.cc:135: reinterpret_cast<uint8_t*>(dest) + kBytesPerPixel * width * i; On 2016/07/09 01:01:20, Sergey Ulanov wrote: > Instead of using multiplication to calculate position you could add stride after > copying each line: > dest += kBytesPerPixel * width; > source += stride; > > This makes code cleaner and potentially slightly faster. Done. https://codereview.chromium.org/2045963004/diff/400001/remoting/android/BUILD.gn File remoting/android/BUILD.gn (right): https://codereview.chromium.org/2045963004/diff/400001/remoting/android/BUILD... remoting/android/BUILD.gn:17: "java/src/org/chromium/chromoting/jni/GlDisplay.java", Comes from ToT.
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...
The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
https://codereview.chromium.org/2045963004/diff/420001/remoting/client/opengl... File remoting/client/opengl_renderer/BUILD.gn (right): https://codereview.chromium.org/2045963004/diff/420001/remoting/client/opengl... remoting/client/opengl_renderer/BUILD.gn:5: source_set("opengl_renderer") { Not sure what's the best way to stop build-all to compile this source set. This change just moves the source set back to a folder...
The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_can... File remoting/client/gl_canvas.h (right): https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_can... remoting/client/gl_canvas.h:44: void SetNormalizedTransformation(const float* matrix); On 2016/07/09 01:21:24, Yuwei wrote: > On 2016/07/09 01:01:20, Sergey Ulanov wrote: > > nit: would be more typesafe to use std::array<float, 9> here. > > Just curious will there be performance drawback compared to C array? no, as long as it's passed by reference or pointer. https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_des... File remoting/client/gl_desktop.cc (right): https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_des... remoting/client/gl_desktop.cc:51: frame->stride() / kBytesPerPixel); On 2016/07/09 01:21:24, Yuwei wrote: > On 2016/07/09 01:01:20, Sergey Ulanov wrote: > > stride() may not be multiple of kBytesPerPixel. Pass stride as it is, in > bytes? > > Hmm... The problem is GL_UNPACK_ROW_LENGTH expects stride in pixel. Maybe in > UpdateTexture, if |stride| is not multiple of kBytesPerPixel, then manually pack > the updated region? hm, yeah. I don't know of any real world scenario where stride() is not multiple of 4, so there is no performance concern about it. It's just there is nothing that requires stride() to be multiple of 4 and it's cleaner to always pass stride measured in bytes. https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_ren... File remoting/client/gl_render_layer.cc (right): https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_ren... remoting/client/gl_render_layer.cc:65: CHECK(texture_set_); On 2016/07/11 22:38:24, Yuwei wrote: > On 2016/07/09 01:01:20, Sergey Ulanov wrote: > > Do you really need CHECK() instead of DCHECK()? > > Not sure when CHECK() will be useful... I CHECK is useful in two cases: - when violation of the verified condition may cause security vulnerability (e.g. buffer overflow). - When there is a hard to debug crash in release builds. CHECK() may make it easier to debug in some cases. Also see https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_ren... remoting/client/gl_render_layer.cc:88: PackDirtyRegion(update_buffer_.get(), subtexture, width, height, On 2016/07/09 01:21:24, Yuwei wrote: > On 2016/07/09 01:01:20, Sergey Ulanov wrote: > > This can be avoided when stride == pixel_size*width. Add a special case for > > this? > > Isn't it covered by line 70? yes, sorry, I missed it. https://codereview.chromium.org/2045963004/diff/360001/remoting/client/opengl... File remoting/client/opengl_renderer.gyp (right): https://codereview.chromium.org/2045963004/diff/360001/remoting/client/opengl... remoting/client/opengl_renderer.gyp:1: # Copyright (c) 2016 Google Inc. All rights reserved. On 2016/07/09 01:21:24, Yuwei wrote: > On 2016/07/09 01:01:21, Sergey Ulanov wrote: > > Do we still need to update GYP? > > According to crbug.com/359249 GN migration is complete for Android > > This is for iOS. I see. Then do you need a separate .gyp file instead of adding a separate target in remoting_client.gypi? https://codereview.chromium.org/2045963004/diff/420001/remoting/client/gl_ren... File remoting/client/gl_render_layer.cc (right): https://codereview.chromium.org/2045963004/diff/420001/remoting/client/gl_ren... remoting/client/gl_render_layer.cc:58: inline void PackDirtyRegion(uint8_t* dest, nit: Don't need to mark anything inline explicitly. Compilers are normally smart enough to inline stuff when it makes sense. Also they are allowed to ignore inline keyword. https://codereview.chromium.org/2045963004/diff/420001/remoting/client/opengl... File remoting/client/opengl_renderer/BUILD.gn (right): https://codereview.chromium.org/2045963004/diff/420001/remoting/client/opengl... remoting/client/opengl_renderer/BUILD.gn:5: source_set("opengl_renderer") { On 2016/07/13 00:37:43, Yuwei wrote: > Not sure what's the best way to stop build-all to compile this source set. This > change just moves the source set back to a folder... You can just wrap it in if (is_android || is_ios) {...} Also add #else case with #error in sys_opengl.h to make this issue easier to debug.
https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_can... File remoting/client/gl_canvas.h (right): https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_can... remoting/client/gl_canvas.h:44: void SetNormalizedTransformation(const float* matrix); On 2016/07/13 04:42:11, Sergey Ulanov wrote: > On 2016/07/09 01:21:24, Yuwei wrote: > > On 2016/07/09 01:01:20, Sergey Ulanov wrote: > > > nit: would be more typesafe to use std::array<float, 9> here. > > > > Just curious will there be performance drawback compared to C array? > > no, as long as it's passed by reference or pointer. Acknowledged. https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_des... File remoting/client/gl_desktop.cc (right): https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_des... remoting/client/gl_desktop.cc:51: frame->stride() / kBytesPerPixel); On 2016/07/13 04:42:11, Sergey Ulanov wrote: > On 2016/07/09 01:21:24, Yuwei wrote: > > On 2016/07/09 01:01:20, Sergey Ulanov wrote: > > > stride() may not be multiple of kBytesPerPixel. Pass stride as it is, in > > bytes? > > > > Hmm... The problem is GL_UNPACK_ROW_LENGTH expects stride in pixel. Maybe in > > UpdateTexture, if |stride| is not multiple of kBytesPerPixel, then manually > pack > > the updated region? > > hm, yeah. I don't know of any real world scenario where stride() is not multiple > of 4, so there is no performance concern about it. It's just there is nothing > that requires stride() to be multiple of 4 and it's cleaner to always pass > stride measured in bytes. Done. Acknowledged. https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_ren... File remoting/client/gl_render_layer.cc (right): https://codereview.chromium.org/2045963004/diff/360001/remoting/client/gl_ren... remoting/client/gl_render_layer.cc:65: CHECK(texture_set_); On 2016/07/13 04:42:11, Sergey Ulanov wrote: > On 2016/07/11 22:38:24, Yuwei wrote: > > On 2016/07/09 01:01:20, Sergey Ulanov wrote: > > > Do you really need CHECK() instead of DCHECK()? > > > > Not sure when CHECK() will be useful... > > I CHECK is useful in two cases: > - when violation of the verified condition may cause security vulnerability > (e.g. buffer overflow). > - When there is a hard to debug crash in release builds. CHECK() may make it > easier to debug in some cases. > > Also see > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md Acknowledged. https://codereview.chromium.org/2045963004/diff/360001/remoting/client/opengl... File remoting/client/opengl_renderer.gyp (right): https://codereview.chromium.org/2045963004/diff/360001/remoting/client/opengl... remoting/client/opengl_renderer.gyp:1: # Copyright (c) 2016 Google Inc. All rights reserved. On 2016/07/13 04:42:11, Sergey Ulanov wrote: > On 2016/07/09 01:21:24, Yuwei wrote: > > On 2016/07/09 01:01:21, Sergey Ulanov wrote: > > > Do we still need to update GYP? > > > According to crbug.com/359249 GN migration is complete for Android > > > > This is for iOS. > > I see. Then do you need a separate .gyp file instead of adding a separate target > in remoting_client.gypi? I don't have a strong opinion... Just put this into remoting_client.gypi https://codereview.chromium.org/2045963004/diff/420001/remoting/client/gl_ren... File remoting/client/gl_render_layer.cc (right): https://codereview.chromium.org/2045963004/diff/420001/remoting/client/gl_ren... remoting/client/gl_render_layer.cc:58: inline void PackDirtyRegion(uint8_t* dest, On 2016/07/13 04:42:11, Sergey Ulanov wrote: > nit: Don't need to mark anything inline explicitly. Compilers are normally smart > enough to inline stuff when it makes sense. Also they are allowed to ignore > inline keyword. Done. https://codereview.chromium.org/2045963004/diff/420001/remoting/client/opengl... File remoting/client/opengl_renderer/BUILD.gn (right): https://codereview.chromium.org/2045963004/diff/420001/remoting/client/opengl... remoting/client/opengl_renderer/BUILD.gn:5: source_set("opengl_renderer") { On 2016/07/13 04:42:11, Sergey Ulanov wrote: > On 2016/07/13 00:37:43, Yuwei wrote: > > Not sure what's the best way to stop build-all to compile this source set. > This > > change just moves the source set back to a folder... > > You can just wrap it in if (is_android || is_ios) {...} > Also add #else case with #error in sys_opengl.h to make this issue easier to > debug. Done.
Patchset #12 (id:460001) has been deleted
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/2045963004/#ps480001 (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.
Description was changed from ========== [Remoting] OpenGL Rendering Layer This CL implements the OpenGL canvas, rendering layer, and desktop. This is part of the project of implementing OpenGL rendering component for Android and iOS. Please see design doc in crbug for more information. BUG=385924 ========== to ========== [Remoting] OpenGL Rendering Layer This CL implements the OpenGL canvas, rendering layer, and desktop. This is part of the project of implementing OpenGL rendering component for Android and iOS. Please see design doc in crbug for more information. BUG=385924 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:480001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== [Remoting] OpenGL Rendering Layer This CL implements the OpenGL canvas, rendering layer, and desktop. This is part of the project of implementing OpenGL rendering component for Android and iOS. Please see design doc in crbug for more information. BUG=385924 ========== to ========== [Remoting] OpenGL Rendering Layer This CL implements the OpenGL canvas, rendering layer, and desktop. This is part of the project of implementing OpenGL rendering component for Android and iOS. Please see design doc in crbug for more information. BUG=385924 Committed: https://crrev.com/2c0936a2a4e515ea10c74e05022de6bc932089d2 Cr-Commit-Position: refs/heads/master@{#405278} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/2c0936a2a4e515ea10c74e05022de6bc932089d2 Cr-Commit-Position: refs/heads/master@{#405278} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
