Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2175963002:
[Chromoting] Implement GlRenderer (Closed)
|
Created:
4 years, 5 months ago by Yuwei Modified:
4 years, 4 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[Chromoting] Implement GlRenderer
This CL implements the GlRenderer class for rendering remote desktop frame
and cursor on the current OpenGL surface. This is part of the project of
using OpenGL ES to render host desktop on Android/iOS client.
BUG=385924
Committed: https://crrev.com/bb2a31fad09e6375c54fc6390548f1c17db3c59a
Cr-Commit-Position: refs/heads/master@{#408213}
Patch Set 1 #Patch Set 2 : Add namespace comments #
Total comments: 28
Patch Set 3 : Fix glUniformMatrix3fv bug in OpenGL ES 2 environment #
Total comments: 1
Patch Set 4 : Reviewer's Feedback #Patch Set 5 : Fix Typo and Move OnFrameRendered() above #
Total comments: 9
Patch Set 6 : Reviewer's Feedback #
Total comments: 3
Patch Set 7 : Implements full transpose #
Messages
Total messages: 27 (8 generated)
Patchset #1 (id:1) 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/v2/patch-status/codereview.chromium.or...
yuweih@chromium.org changed reviewers: + sergeyu@chromium.org
ptal If you are interested in the usage of this class, you can see remoting/client/jni/jni_gl_display_handler.cc in the WIP CL: https://codereview.chromium.org/2032963002
https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... File remoting/client/gl_renderer.cc (right): https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... remoting/client/gl_renderer.cc:133: base::ThreadTaskRunnerHandle::Get()->PostTask( The cursor will occasionally shake when moving around if the task runs without delay. Still trying to figure out what is going on...
https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... File remoting/client/gl_renderer.cc (right): https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... remoting/client/gl_renderer.cc:133: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2016/07/23 01:02:02, Yuwei wrote: > The cursor will occasionally shake when moving around if the task runs without > delay. Still trying to figure out what is going on... Just did some printf debugging and turns out the problem is that OnCursorMoved will be occasionally blocked by eglSwapBuffers then we will get one frame with only OnTransformationChanged and the next frame with two OnCursorMoved calls. This makes the cursor move back and forth on the screen. If we post the task with some delay, then we will consistently get the sequence of move viewport -- move cursor -- swap buffer and the cursor will not shake. Interestingly even just delaying the render task for 1ms the cursor won't shake...
https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... File remoting/client/gl_renderer.cc (right): https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... remoting/client/gl_renderer.cc:43: std::unique_ptr<std::array<float, 9>> mat) { nit: Pass the argument as a const reference to the array. This function doesn't really need to take ownership https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... File remoting/client/gl_renderer.h (right): https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... remoting/client/gl_renderer.h:38: // void RenderCallback(const base::Closure& draw_closure) this line looks like commented method declaration. Do you need it? https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... remoting/client/gl_renderer.h:50: // ui_task_runner_->PostTask(FROM_HERE, render_done_); What is render_done_ and how is it relevant here? https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... remoting/client/gl_renderer.h:53: using RenderCallback = base::Callback<void(const base::Closure&)>; add name of the argument for the callback, e.g. base::Callback<void(const base::Closure& done_callback)> https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... remoting/client/gl_renderer.h:53: using RenderCallback = base::Callback<void(const base::Closure&)>; So if I understand correctly the handler is responsible for deciding if the frame should be rendered or not. Is that correct understanding? Would it make code simpler if you replace this with a delegate interface that tells the renderer if we can render or not. class Delegate() { virtual bool CanRenderFrame() = 0; virtual void OnFrameRendered() = 0; virtual void OnSizeChanged(int width, int height) = 0; } You can also use the same delegate to replace SizeCallback, which would help to avoid 2 Set*Callback() methods. https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... remoting/client/gl_renderer.h:55: // void SizeCallback(int width, int height) remove this line? https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... remoting/client/gl_renderer.h:56: using SizeCallback = base::Callback<void(int, int)>; base::Callback<void(int width, int height)> https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... remoting/client/gl_renderer.h:160: DISALLOW_COPY_AND_ASSIGN(GlRenderer); nit: empty line here https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... remoting/client/gl_renderer.h:164: #endif // REMOTING_CLIENT_GL_RENDERER_H_ nit: empty line here
https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... File remoting/client/gl_renderer.cc (right): https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... remoting/client/gl_renderer.cc:43: std::unique_ptr<std::array<float, 9>> mat) { On 2016/07/25 19:34:13, Sergey Ulanov wrote: > nit: Pass the argument as a const reference to the array. This function doesn't > really need to take ownership Currently NormalizeTransformationMatrix is done directly on the array. Passing owned array may avoid copying it for normalization. Copying just an array of 9 probably won't have noticable performance impact though... https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... remoting/client/gl_renderer.cc:133: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2016/07/23 04:41:14, Yuwei wrote: > On 2016/07/23 01:02:02, Yuwei wrote: > > The cursor will occasionally shake when moving around if the task runs without > > delay. Still trying to figure out what is going on... > > Just did some printf debugging and turns out the problem is that OnCursorMoved > will be occasionally blocked by eglSwapBuffers then we will get one frame with > only OnTransformationChanged and the next frame with two OnCursorMoved calls. > This makes the cursor move back and forth on the screen. > > If we post the task with some delay, then we will consistently get the sequence > of move viewport -- move cursor -- swap buffer and the cursor will not shake. > > Interestingly even just delaying the render task for 1ms the cursor won't > shake... For this issue, should I just add back the minimum rendering interval logic? https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... File remoting/client/gl_renderer.h (right): https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... remoting/client/gl_renderer.h:38: // void RenderCallback(const base::Closure& draw_closure) On 2016/07/25 19:34:14, Sergey Ulanov wrote: > this line looks like commented method declaration. Do you need it? Just try to show how the header of the callback will look like. Maybe it's unnecessary... https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... remoting/client/gl_renderer.h:50: // ui_task_runner_->PostTask(FROM_HERE, render_done_); On 2016/07/25 19:34:13, Sergey Ulanov wrote: > What is render_done_ and how is it relevant here? Just an example of how the caller may implement this callback... The caller may want to notify the UI thread to continue processing animation after the frame is rendered.
https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... File remoting/client/gl_renderer.cc (right): https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... remoting/client/gl_renderer.cc:43: std::unique_ptr<std::array<float, 9>> mat) { On 2016/07/25 19:52:18, Yuwei wrote: > On 2016/07/25 19:34:13, Sergey Ulanov wrote: > > nit: Pass the argument as a const reference to the array. This function > doesn't > > really need to take ownership > > Currently NormalizeTransformationMatrix is done directly on the array. Passing > owned array may avoid copying it for normalization. Copying just an array of 9 > probably won't have noticable performance impact though... The way this function is implemented it doesn't really need to take ownership of the matrix. Problem is that if the caller doesn't have the array on heap or doesn't have ownership then it would have to copy it itself. https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... remoting/client/gl_renderer.cc:43: std::unique_ptr<std::array<float, 9>> mat) { call it |matrix| instead of |mat|. https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... remoting/client/gl_renderer.cc:133: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2016/07/25 19:52:18, Yuwei wrote: > On 2016/07/23 04:41:14, Yuwei wrote: > > On 2016/07/23 01:02:02, Yuwei wrote: > > > The cursor will occasionally shake when moving around if the task runs > without > > > delay. Still trying to figure out what is going on... > > > > Just did some printf debugging and turns out the problem is that OnCursorMoved > > will be occasionally blocked by eglSwapBuffers then we will get one frame with > > only OnTransformationChanged and the next frame with two OnCursorMoved calls. > > This makes the cursor move back and forth on the screen. > > > > If we post the task with some delay, then we will consistently get the > sequence > > of move viewport -- move cursor -- swap buffer and the cursor will not shake. > > > > Interestingly even just delaying the render task for 1ms the cursor won't > > shake... > > For this issue, should I just add back the minimum rendering interval logic? As far as I can tell here is what happens: 1. Display thread: The renderer renders a frame. 2. Display thread: JniGlDisplayHandler::RenderOnDisplayThread() posts a NotifyRenderDoneOnUiThread() task to notify that rendering has been done. 3. UI thread: FlingAnimationJob moves the cursor in response to the OnRenderDone event. 4. UI thread: JniGlDisplayHandler::OnCursorPixelPositionChanged() post a task to display thread to move the cursor, which may or may not be executed before the next frame is rendered because the desktop thread may be already rendering another frame. Adding a delay before each render may appear to fix the problem, but it's not going to be reliable (e.g. if (3) takes more than 1ms due to garbage collection). I think the renderer should wait for notification that all animations have been processed and render another frame only after that. I.e. it would looks something like this: 1. Render frame. 2. Request the UI layer (java) to process animations, e.g. move the cursor, scroll, etc. This is effectively what "RenderDone" is doing right now. 3. Wait for notification that (2) has been done. 4. Post task for (1) to render next frame if necessary. Step 3 is missing right now, and that's what causes jerkiness right now.
https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... File remoting/client/gl_renderer.cc (right): https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... remoting/client/gl_renderer.cc:133: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2016/07/25 21:22:41, Sergey Ulanov wrote: > On 2016/07/25 19:52:18, Yuwei wrote: > > On 2016/07/23 04:41:14, Yuwei wrote: > > > On 2016/07/23 01:02:02, Yuwei wrote: > > > > The cursor will occasionally shake when moving around if the task runs > > without > > > > delay. Still trying to figure out what is going on... > > > > > > Just did some printf debugging and turns out the problem is that > OnCursorMoved > > > will be occasionally blocked by eglSwapBuffers then we will get one frame > with > > > only OnTransformationChanged and the next frame with two OnCursorMoved > calls. > > > This makes the cursor move back and forth on the screen. > > > > > > If we post the task with some delay, then we will consistently get the > > sequence > > > of move viewport -- move cursor -- swap buffer and the cursor will not > shake. > > > > > > Interestingly even just delaying the render task for 1ms the cursor won't > > > shake... > > > > For this issue, should I just add back the minimum rendering interval logic? > > As far as I can tell here is what happens: > 1. Display thread: The renderer renders a frame. > 2. Display thread: JniGlDisplayHandler::RenderOnDisplayThread() posts a > NotifyRenderDoneOnUiThread() task to notify that rendering has been done. > 3. UI thread: FlingAnimationJob moves the cursor in response to the > OnRenderDone event. > 4. UI thread: JniGlDisplayHandler::OnCursorPixelPositionChanged() post a task > to display thread to move the cursor, which may or may not be executed before > the next frame is rendered because the desktop thread may be already rendering > another frame. > > Adding a delay before each render may appear to fix the problem, but it's not > going to be reliable (e.g. if (3) takes more than 1ms due to garbage > collection). I think the renderer should wait for notification that all > animations have been processed and render another frame only after that. I.e. it > would looks something like this: > 1. Render frame. > 2. Request the UI layer (java) to process animations, e.g. move the cursor, > scroll, etc. This is effectively what "RenderDone" is doing right now. > 3. Wait for notification that (2) has been done. > 4. Post task for (1) to render next frame if necessary. > > Step 3 is missing right now, and that's what causes jerkiness right now. > That's a fair enough solution. I think I just need to block the render request until both animations are processed.
https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... File remoting/client/gl_renderer.cc (right): https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... remoting/client/gl_renderer.cc:43: std::unique_ptr<std::array<float, 9>> mat) { On 2016/07/25 21:22:41, Sergey Ulanov wrote: > On 2016/07/25 19:52:18, Yuwei wrote: > > On 2016/07/25 19:34:13, Sergey Ulanov wrote: > > > nit: Pass the argument as a const reference to the array. This function > > doesn't > > > really need to take ownership > > > > Currently NormalizeTransformationMatrix is done directly on the array. Passing > > owned array may avoid copying it for normalization. Copying just an array of 9 > > probably won't have noticable performance impact though... > > The way this function is implemented it doesn't really need to take ownership of > the matrix. Problem is that if the caller doesn't have the array on heap or > doesn't have ownership then it would have to copy it itself. But base::Bind will still copy it, right? For current usage in the WIP JniGlDisplayHandler, it creates an array and fills it up then move it to the closure. By having const std::array& we will need to do the first copy to pass the array to the closure and a second copy to get a non-const array. Anyway, I will just use const reference so that it can be more friendly to the caller. https://codereview.chromium.org/2175963002/diff/60001/remoting/client/gl_canv... File remoting/client/gl_canvas.cc (left): https://codereview.chromium.org/2175963002/diff/60001/remoting/client/gl_canv... remoting/client/gl_canvas.cc:87: glUniformMatrix3fv(transform_location_, 1, GL_TRUE, matrix.data()); There was a bug in GlCanvas. It turns out the ES 2 spec (terrible spec :( ) says the |transpose| argument of glUniformMatrix3fv must be set to GL_FALSE, which mean we need to manually transpose the matrix before sending it to GPU. This bug doesn't appear on my Nexus 6P since it automatically uses the ES 3 lib...
https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... File remoting/client/gl_renderer.cc (right): https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... remoting/client/gl_renderer.cc:133: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2016/07/25 21:22:41, Sergey Ulanov wrote: > On 2016/07/25 19:52:18, Yuwei wrote: > > On 2016/07/23 04:41:14, Yuwei wrote: > > > On 2016/07/23 01:02:02, Yuwei wrote: > > > > The cursor will occasionally shake when moving around if the task runs > > without > > > > delay. Still trying to figure out what is going on... > > > > > > Just did some printf debugging and turns out the problem is that > OnCursorMoved > > > will be occasionally blocked by eglSwapBuffers then we will get one frame > with > > > only OnTransformationChanged and the next frame with two OnCursorMoved > calls. > > > This makes the cursor move back and forth on the screen. > > > > > > If we post the task with some delay, then we will consistently get the > > sequence > > > of move viewport -- move cursor -- swap buffer and the cursor will not > shake. > > > > > > Interestingly even just delaying the render task for 1ms the cursor won't > > > shake... > > > > For this issue, should I just add back the minimum rendering interval logic? > > As far as I can tell here is what happens: > 1. Display thread: The renderer renders a frame. > 2. Display thread: JniGlDisplayHandler::RenderOnDisplayThread() posts a > NotifyRenderDoneOnUiThread() task to notify that rendering has been done. > 3. UI thread: FlingAnimationJob moves the cursor in response to the > OnRenderDone event. > 4. UI thread: JniGlDisplayHandler::OnCursorPixelPositionChanged() post a task > to display thread to move the cursor, which may or may not be executed before > the next frame is rendered because the desktop thread may be already rendering > another frame. > > Adding a delay before each render may appear to fix the problem, but it's not > going to be reliable (e.g. if (3) takes more than 1ms due to garbage > collection). I think the renderer should wait for notification that all > animations have been processed and render another frame only after that. I.e. it > would looks something like this: > 1. Render frame. > 2. Request the UI layer (java) to process animations, e.g. move the cursor, > scroll, etc. This is effectively what "RenderDone" is doing right now. > 3. Wait for notification that (2) has been done. > 4. Post task for (1) to render next frame if necessary. > > Step 3 is missing right now, and that's what causes jerkiness right now. > Looks like this only solves the specific problem of animation. User interaction that moves both the cursor and viewport will still make the cursor jump a little bit... This will be hard to resolve unless you totally bundle cursor movement with the viewport movement. Delaying just 1ms will not be reliable, but making it render not earlier than 15ms will statistically have fewer problem (events out of sync, frame skipping, etc.) and require less hacks. https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... File remoting/client/gl_renderer.h (right): https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... remoting/client/gl_renderer.h:53: using RenderCallback = base::Callback<void(const base::Closure&)>; On 2016/07/25 19:34:13, Sergey Ulanov wrote: > add name of the argument for the callback, e.g. > base::Callback<void(const base::Closure& done_callback)> Obsolete. https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... remoting/client/gl_renderer.h:53: using RenderCallback = base::Callback<void(const base::Closure&)>; On 2016/07/25 19:34:13, Sergey Ulanov wrote: > So if I understand correctly the handler is responsible for deciding if the > frame should be rendered or not. Is that correct understanding? > Would it make code simpler if you replace this with a delegate interface that > tells the renderer if we can render or not. > > class Delegate() { > virtual bool CanRenderFrame() = 0; > virtual void OnFrameRendered() = 0; > virtual void OnSizeChanged(int width, int height) = 0; > } > > You can also use the same delegate to replace SizeCallback, which would help to > avoid 2 Set*Callback() methods. Done. https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... remoting/client/gl_renderer.h:55: // void SizeCallback(int width, int height) On 2016/07/25 19:34:13, Sergey Ulanov wrote: > remove this line? Obsolete. https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... remoting/client/gl_renderer.h:56: using SizeCallback = base::Callback<void(int, int)>; On 2016/07/25 19:34:13, Sergey Ulanov wrote: > base::Callback<void(int width, int height)> Obsolete. https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... remoting/client/gl_renderer.h:160: DISALLOW_COPY_AND_ASSIGN(GlRenderer); On 2016/07/25 19:34:13, Sergey Ulanov wrote: > nit: empty line here Done. https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... remoting/client/gl_renderer.h:164: #endif // REMOTING_CLIENT_GL_RENDERER_H_ On 2016/07/25 19:34:13, Sergey Ulanov wrote: > nit: empty line here Done.
https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... File remoting/client/gl_renderer.cc (right): https://codereview.chromium.org/2175963002/diff/40001/remoting/client/gl_rend... remoting/client/gl_renderer.cc:133: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2016/07/26 05:01:43, Yuwei wrote: > On 2016/07/25 21:22:41, Sergey Ulanov wrote: > > On 2016/07/25 19:52:18, Yuwei wrote: > > > On 2016/07/23 04:41:14, Yuwei wrote: > > > > On 2016/07/23 01:02:02, Yuwei wrote: > > > > > The cursor will occasionally shake when moving around if the task runs > > > without > > > > > delay. Still trying to figure out what is going on... > > > > > > > > Just did some printf debugging and turns out the problem is that > > OnCursorMoved > > > > will be occasionally blocked by eglSwapBuffers then we will get one frame > > with > > > > only OnTransformationChanged and the next frame with two OnCursorMoved > > calls. > > > > This makes the cursor move back and forth on the screen. > > > > > > > > If we post the task with some delay, then we will consistently get the > > > sequence > > > > of move viewport -- move cursor -- swap buffer and the cursor will not > > shake. > > > > > > > > Interestingly even just delaying the render task for 1ms the cursor won't > > > > shake... > > > > > > For this issue, should I just add back the minimum rendering interval logic? > > > > As far as I can tell here is what happens: > > 1. Display thread: The renderer renders a frame. > > 2. Display thread: JniGlDisplayHandler::RenderOnDisplayThread() posts a > > NotifyRenderDoneOnUiThread() task to notify that rendering has been done. > > 3. UI thread: FlingAnimationJob moves the cursor in response to the > > OnRenderDone event. > > 4. UI thread: JniGlDisplayHandler::OnCursorPixelPositionChanged() post a task > > to display thread to move the cursor, which may or may not be executed before > > the next frame is rendered because the desktop thread may be already rendering > > another frame. > > > > Adding a delay before each render may appear to fix the problem, but it's not > > going to be reliable (e.g. if (3) takes more than 1ms due to garbage > > collection). I think the renderer should wait for notification that all > > animations have been processed and render another frame only after that. I.e. > it > > would looks something like this: > > 1. Render frame. > > 2. Request the UI layer (java) to process animations, e.g. move the cursor, > > scroll, etc. This is effectively what "RenderDone" is doing right now. > > 3. Wait for notification that (2) has been done. > > 4. Post task for (1) to render next frame if necessary. > > > > Step 3 is missing right now, and that's what causes jerkiness right now. > > > > Looks like this only solves the specific problem of animation. User interaction > that moves both the cursor and viewport will still make the cursor jump a little > bit... This will be hard to resolve unless you totally bundle cursor movement > with the viewport movement. > > Delaying just 1ms will not be reliable, but making it render not earlier than > 15ms will statistically have fewer problem (events out of sync, frame skipping, > etc.) and require less hacks. Hmm... Looks like stuttering can still happen at the beginning of the movement. It will only go away if we unconditionally delay the rendering task, which will hurt the rendering latency of the desktop frame... Alternatively we can bundle cursor movement with the viewport movement or have different delay for different reasons of requesting render. I think I will just hold this issue for the JniGlDisplayHandler CL and have the simplest implementation in this CL...
Patchset #5 (id:100001) has been deleted
ptal
lgtm when my comments are addressed. https://codereview.chromium.org/2175963002/diff/120001/remoting/client/gl_can... File remoting/client/gl_canvas.h (right): https://codereview.chromium.org/2175963002/diff/120001/remoting/client/gl_can... remoting/client/gl_canvas.h:34: // [ m0, m3, m6, m1, m4, m7, m2, m5, m8 ]. This is not intuitive and is hard to work with. Maybe just transpose the matrix before calling glUniformMatrix3fv()? https://codereview.chromium.org/2175963002/diff/120001/remoting/client/gl_ren... File remoting/client/gl_renderer.cc (right): https://codereview.chromium.org/2175963002/diff/120001/remoting/client/gl_ren... remoting/client/gl_renderer.cc:25: DCHECK(thread_checker_.CalledOnValidThread()); Don't need this. thread_checker already verifies thread in its destructor. https://codereview.chromium.org/2175963002/diff/120001/remoting/client/gl_ren... remoting/client/gl_renderer.cc:58: cursor_feedback_.StartAnimation(((float) x) / canvas_width_, Do not use c-style casts. https://google.github.io/styleguide/cppguide.html#Casting Instead please use static_cast<float>(x) https://codereview.chromium.org/2175963002/diff/120001/remoting/client/gl_ren... File remoting/client/gl_renderer_delegate.h (right): https://codereview.chromium.org/2175963002/diff/120001/remoting/client/gl_ren... remoting/client/gl_renderer_delegate.h:14: class GlRendererDelegate { Add private virtual destructor.
https://codereview.chromium.org/2175963002/diff/120001/remoting/client/gl_can... File remoting/client/gl_canvas.h (right): https://codereview.chromium.org/2175963002/diff/120001/remoting/client/gl_can... remoting/client/gl_canvas.h:34: // [ m0, m3, m6, m1, m4, m7, m2, m5, m8 ]. On 2016/07/27 17:45:33, Sergey Ulanov wrote: > This is not intuitive and is hard to work with. Maybe just transpose the matrix > before calling glUniformMatrix3fv()? Done. Wrote separate function for matrix transposing. https://codereview.chromium.org/2175963002/diff/120001/remoting/client/gl_ren... File remoting/client/gl_renderer.cc (right): https://codereview.chromium.org/2175963002/diff/120001/remoting/client/gl_ren... remoting/client/gl_renderer.cc:25: DCHECK(thread_checker_.CalledOnValidThread()); On 2016/07/27 17:45:33, Sergey Ulanov wrote: > Don't need this. thread_checker already verifies thread in its destructor. Done. https://codereview.chromium.org/2175963002/diff/120001/remoting/client/gl_ren... remoting/client/gl_renderer.cc:58: cursor_feedback_.StartAnimation(((float) x) / canvas_width_, On 2016/07/27 17:45:33, Sergey Ulanov wrote: > Do not use c-style casts. > https://google.github.io/styleguide/cppguide.html#Casting > Instead please use static_cast<float>(x) Done. https://codereview.chromium.org/2175963002/diff/120001/remoting/client/gl_ren... File remoting/client/gl_renderer_delegate.h (right): https://codereview.chromium.org/2175963002/diff/120001/remoting/client/gl_ren... remoting/client/gl_renderer_delegate.h:14: class GlRendererDelegate { On 2016/07/27 17:45:33, Sergey Ulanov wrote: > Add private virtual destructor. Done. I think you mean protected?
https://codereview.chromium.org/2175963002/diff/120001/remoting/client/gl_ren... File remoting/client/gl_renderer_delegate.h (right): https://codereview.chromium.org/2175963002/diff/120001/remoting/client/gl_ren... remoting/client/gl_renderer_delegate.h:14: class GlRendererDelegate { On 2016/07/27 18:23:11, Yuwei wrote: > On 2016/07/27 17:45:33, Sergey Ulanov wrote: > > Add private virtual destructor. > > Done. I think you mean protected? yes https://codereview.chromium.org/2175963002/diff/140001/remoting/client/gl_math.h File remoting/client/gl_math.h (right): https://codereview.chromium.org/2175963002/diff/140001/remoting/client/gl_mat... remoting/client/gl_math.h:38: // The result is only valid if m1=m3=m6=m7=0. Why do you need this requirement?
https://codereview.chromium.org/2175963002/diff/140001/remoting/client/gl_math.h File remoting/client/gl_math.h (right): https://codereview.chromium.org/2175963002/diff/140001/remoting/client/gl_mat... remoting/client/gl_math.h:38: // The result is only valid if m1=m3=m6=m7=0. On 2016/07/27 18:36:41, Sergey Ulanov wrote: > Why do you need this requirement? If I remove this constraint then I will need to swap everything on different sides of the diagonal... That doesn't seem expensive so I think I will just implement this...
https://codereview.chromium.org/2175963002/diff/140001/remoting/client/gl_math.h File remoting/client/gl_math.h (right): https://codereview.chromium.org/2175963002/diff/140001/remoting/client/gl_mat... remoting/client/gl_math.h:38: // The result is only valid if m1=m3=m6=m7=0. On 2016/07/27 18:36:41, Sergey Ulanov wrote: > Why do you need this requirement? 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/2175963002/#ps160001 (title: "Implements full transpose")
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 #7 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Implement GlRenderer This CL implements the GlRenderer class for rendering remote desktop frame and cursor on the current OpenGL surface. This is part of the project of using OpenGL ES to render host desktop on Android/iOS client. BUG=385924 ========== to ========== [Chromoting] Implement GlRenderer This CL implements the GlRenderer class for rendering remote desktop frame and cursor on the current OpenGL surface. This is part of the project of using OpenGL ES to render host desktop on Android/iOS client. BUG=385924 Committed: https://crrev.com/bb2a31fad09e6375c54fc6390548f1c17db3c59a Cr-Commit-Position: refs/heads/master@{#408213} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/bb2a31fad09e6375c54fc6390548f1c17db3c59a Cr-Commit-Position: refs/heads/master@{#408213} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
