Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2623413004:
[Remoting Android] Fix thread issue with OpenGL drawable (Closed)
Description[Remoting Android] Fix thread issue with OpenGL drawable
This CL has these changes:
* Store drawable's weak_ptr in ctor rather than using thread checker
to check the thread safety of GetWeakPtr() since GetWeakPtr() is called
in multiple threads.
* Avoid all attempts in CreateGlRendererWithDesktop() to dereference
any weak pointer since CreateGlRendererWithDesktop() is supposed to be
called on any thread.
* DCHECK the thread in AddDrawable(). AddDrawable is never safe to be called
on a different thread once GlRenderer gets any side effect.
BUG=680800
Review-Url: https://codereview.chromium.org/2623413004
Cr-Commit-Position: refs/heads/master@{#444574}
Committed: https://chromium.googlesource.com/chromium/src/+/3ce1a24a7e23be0e5f01d38a943b51cf87a01895
Patch Set 1 : PTAL Point #
Total comments: 21
Patch Set 2 : PTAL / Rewrite #
Total comments: 8
Patch Set 3 : Reviewer's Feedback #
Messages
Total messages: 28 (8 generated)
yuweih@chromium.org changed reviewers: + joedow@chromium.org, sergeyu@chromium.org
PTAL https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... File remoting/client/display/gl_cursor.h (left): https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... remoting/client/display/gl_cursor.h:64: base::ThreadChecker thread_checker_; Removed thread_checker_ entirely since it's only used in Draw() but not in other functions with side effect. Not sure what's the point of it...
Description was changed from ========== [Chromoting.com] Fix thread issue with OpenGL drawable This CL has these changes: * Store drawable's weak_ptr in ctor rather than using thread checker to check the thread safety of GetWeakPtr() since GetWeakPtr() is called in multiple threads. * Avoid all attempts in CreateGlRendererWithDesktop() to dereference any weak pointer since CreateGlRendererWithDesktop() is supposed to be called on any thread. * DCHECK the thread in AddDrawable(). AddDrawable is never safe to be called on a different thread once GlRenderer gets any side effect. BUG=680800 ========== to ========== [Remoting Android] Fix thread issue with OpenGL drawable This CL has these changes: * Store drawable's weak_ptr in ctor rather than using thread checker to check the thread safety of GetWeakPtr() since GetWeakPtr() is called in multiple threads. * Avoid all attempts in CreateGlRendererWithDesktop() to dereference any weak pointer since CreateGlRendererWithDesktop() is supposed to be called on any thread. * DCHECK the thread in AddDrawable(). AddDrawable is never safe to be called on a different thread once GlRenderer gets any side effect. BUG=680800 ==========
Sorry... Got used to the [chromoting.com] prefix...
I think a better fix would be to make JniGlDisplayHandler to construct the renderer on the display thread. This would allow to keep renderer single-thread, making it simpler. I think it should be a simple fix: JniGlDisplayHandler::Core::Core() just needs to post a task to the display thread to get renderer initialized. WDYT? https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... File remoting/client/display/gl_cursor.cc (right): https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... remoting/client/display/gl_cursor.cc:22: weak_ptr_ = weak_factory_.GetWeakPtr(); With the current implementation of WeakFactory to make GetWeakPtr() safe you just need to call GetWeakPtr() here, but don't need to keep the result. But see my next comment. https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... remoting/client/display/gl_cursor.cc:108: base::WeakPtr<Drawable> GlCursor::GetWeakPtr() { Does this function really need to be thread-safe? The renderer overall should be single-threaded, so I don't see any case when it can be useful to call this function on any thread other than the one where it's created. https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... File remoting/client/display/gl_cursor.h (left): https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... remoting/client/display/gl_cursor.h:64: base::ThreadChecker thread_checker_; On 2017/01/13 01:29:00, Yuwei wrote: > Removed thread_checker_ entirely since it's only used in Draw() but not in other > functions with side effect. Not sure what's the point of it... It still can be useful, even if used in one place: 1. It verifies that Draw() is called on the same thread that crated the object. 2. It verifies that the object is destroyed on the same thread on which it's created. But I don't think we have thread_checker_ in every single-threaded class, so I don't mind removing it here. https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... File remoting/client/display/gl_renderer.cc (right): https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... remoting/client/display/gl_renderer.cc:135: std::unique_ptr<GlRenderer> GlRenderer::CreateGlRendererWithDesktop() { This code looks weird. If the GlRenderer owns all the Drawables, why do we have to add them to drawables_ here instead of in the constructor? It feels like GlRenderer should really be split into two layers as follows: 1. GlRender: holds list of drawables and knows how to draw them, but doesn't know what they are 2. DesktopRenderer: owns GlRenderer and all the drawables we need. Knows how to update the drawables (e.g. move the cursor, etc.) WDYT? https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... remoting/client/display/gl_renderer.cc:139: std::sort(drawables.begin(), drawables.end(), This is way more complicated than necessary. We have just 3 drawables and the order is known in advance. Is it not possible to just initialize the drawables_ list in the proper order without sorting anything? https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... remoting/client/display/gl_renderer.cc:157: void GlRenderer::AddDrawable(base::WeakPtr<Drawable> drawable) { Is this function still used anywhere?
https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... File remoting/client/display/gl_cursor.h (left): https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... remoting/client/display/gl_cursor.h:64: base::ThreadChecker thread_checker_; On 2017/01/13 02:03:10, Sergey Ulanov wrote: > On 2017/01/13 01:29:00, Yuwei wrote: > > Removed thread_checker_ entirely since it's only used in Draw() but not in > other > > functions with side effect. Not sure what's the point of it... > > It still can be useful, even if used in one place: > 1. It verifies that Draw() is called on the same thread that crated the object. > 2. It verifies that the object is destroyed on the same thread on which it's > created. > > But I don't think we have thread_checker_ in every single-threaded class, so I > don't mind removing it here. Also, why is it called only in Draw()? Is there any case when these functions are called on threads other than the display thread?
I think it makes more sense to refactor the code before trying to fix this directly. What about sending a new CL for refactoring and closing this CL? https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... File remoting/client/display/gl_cursor.h (left): https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... remoting/client/display/gl_cursor.h:64: base::ThreadChecker thread_checker_; On 2017/01/13 02:06:19, Sergey Ulanov wrote: > On 2017/01/13 02:03:10, Sergey Ulanov wrote: > > On 2017/01/13 01:29:00, Yuwei wrote: > > > Removed thread_checker_ entirely since it's only used in Draw() but not in > > other > > > functions with side effect. Not sure what's the point of it... > > > > It still can be useful, even if used in one place: > > 1. It verifies that Draw() is called on the same thread that crated the > object. > > 2. It verifies that the object is destroyed on the same thread on which it's > > created. > > > > But I don't think we have thread_checker_ in every single-threaded class, so I > > don't mind removing it here. > > Also, why is it called only in Draw()? I don't know... Scott may know better O_o > Is there any case when these functions are called on threads > other than the display thread? No. It should be entirely on Display. Previously I didn't bother introducing a thread checker since GlRenderer already thread-checks all functions. https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... File remoting/client/display/gl_renderer.cc (right): https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... remoting/client/display/gl_renderer.cc:135: std::unique_ptr<GlRenderer> GlRenderer::CreateGlRendererWithDesktop() { On 2017/01/13 02:03:10, Sergey Ulanov wrote: > This code looks weird. If the GlRenderer owns all the Drawables, why do we have > to add them to drawables_ here instead of in the constructor? > > It feels like GlRenderer should really be split into two layers as follows: > 1. GlRender: holds list of drawables and knows how to draw them, but doesn't > know what they are > 2. DesktopRenderer: owns GlRenderer and all the drawables we need. Knows how to > update the drawables (e.g. move the cursor, etc.) > > WDYT? That's exactly what I plan to do. https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... remoting/client/display/gl_renderer.cc:157: void GlRenderer::AddDrawable(base::WeakPtr<Drawable> drawable) { On 2017/01/13 02:03:10, Sergey Ulanov wrote: > Is this function still used anywhere? I think the unit test is still using it. Also IIRC Scott has some fancy demo Drawable for debugging that has not been checked in?
https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... File remoting/client/display/gl_cursor.h (left): https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... remoting/client/display/gl_cursor.h:64: base::ThreadChecker thread_checker_; On 2017/01/13 02:21:08, Yuwei wrote: > On 2017/01/13 02:06:19, Sergey Ulanov wrote: > > On 2017/01/13 02:03:10, Sergey Ulanov wrote: > > > On 2017/01/13 01:29:00, Yuwei wrote: > > > > Removed thread_checker_ entirely since it's only used in Draw() but not in > > > other > > > > functions with side effect. Not sure what's the point of it... > > > > > > It still can be useful, even if used in one place: > > > 1. It verifies that Draw() is called on the same thread that crated the > > object. > > > 2. It verifies that the object is destroyed on the same thread on which > it's > > > created. > > > > > > But I don't think we have thread_checker_ in every single-threaded class, so > I > > > don't mind removing it here. > > > > Also, why is it called only in Draw()? > > I don't know... Scott may know better O_o > > > Is there any case when these functions are called on threads > > other than the display thread? > > No. It should be entirely on Display. Previously I didn't bother introducing a > thread checker since GlRenderer already thread-checks all functions. Threadchecker was added to ensure the WeakPtr() was created on the same thread the class was created on. It could have been added to every method but was not done in the original CL. WeakPtrs are only guaranteed to be valid if they are used on the thread they are bound to, if you pass one to another thread, it is possible for it to be out of sync if it is invalidated on the original thread. Is the design such that the GL objects are created and used on a single thread or does it invlove multiple threads? https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... File remoting/client/display/gl_renderer.cc (right): https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... remoting/client/display/gl_renderer.cc:135: std::unique_ptr<GlRenderer> GlRenderer::CreateGlRendererWithDesktop() { On 2017/01/13 02:21:08, Yuwei wrote: > On 2017/01/13 02:03:10, Sergey Ulanov wrote: > > This code looks weird. If the GlRenderer owns all the Drawables, why do we > have > > to add them to drawables_ here instead of in the constructor? > > > > It feels like GlRenderer should really be split into two layers as follows: > > 1. GlRender: holds list of drawables and knows how to draw them, but doesn't > > know what they are > > 2. DesktopRenderer: owns GlRenderer and all the drawables we need. Knows how > to > > update the drawables (e.g. move the cursor, etc.) > > > > WDYT? > > That's exactly what I plan to do. To my knowledge Scott is actively working on this code, have you talked to him/are coordinating your changes and plans with him?
sergeyu@chromium.org changed reviewers: + nicholss@chromium.org
+nicholss@
I do not see a test that added that proves this has any issue currently. The change described here is counter to the direction I was trying to bring the code and breaks the future integration with iOS. I was asked to commit in stages so the fact that some functions are not used at the moment is only a consequence of a multi-commit effort on my side. I am saddened by the lack of communication around this change. A lot of effort and wasted time could have been avoided with a simple conversation. I think this is an underlaying culture problem in our team we need to band together to fix. I think I am also to blame for my lack of design doc on the future of the renderer on mobile. I did not imagine there would be competing changes on-top of my changes, before the rest of the migration could be finished. I would be happy to work together to fix any threading issues that may have resulted in my previous change if provided with a unit test that allows me to see the error. As it stood, all unit tests passed. We have too many target architectures to depend on manually testing each individually. https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... File remoting/client/display/gl_cursor.cc (right): https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... remoting/client/display/gl_cursor.cc:108: base::WeakPtr<Drawable> GlCursor::GetWeakPtr() { On 2017/01/13 02:03:10, Sergey Ulanov wrote: > Does this function really need to be thread-safe? The renderer overall should be > single-threaded, so I don't see any case when it can be useful to call this > function on any thread other than the one where it's created. I think in the future it will be accessed via many threads potentially. https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... File remoting/client/display/gl_cursor.h (left): https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... remoting/client/display/gl_cursor.h:64: base::ThreadChecker thread_checker_; For the current usage, these classes could get away with just assuming it is being called on the network thread but that is because this system was designed highly coupled and my goals have been to break up the graph and allow other classes to add and control the drawable elements. For example: the renderer should not be the class that delegates the drawing of the decoded frames to the desktop. It should be the direct responsibility of the desktop or related class to push that to the desktop object and then the desktop object should request a render cycle from the renderer. The renderer should not be tied to the implementation of chromoting at all. So in this future, these classes are independent from the renderer and potentially getting called from a thread that is not the rendering thread, and thread checker is a good idea. https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... File remoting/client/display/gl_renderer.cc (left): https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... remoting/client/display/gl_renderer.cc:175: std::unique_ptr<GlRenderer> GlRenderer::CreateGlRendererWithDesktop() { I think it is a mistake to remove this and use the other version. too much duplicated logic and it breaks the future expandability of the renderer. https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... File remoting/client/display/gl_renderer.cc (right): https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... remoting/client/display/gl_renderer.cc:23: template <typename DrawablePtr> I don't understand why this is better. https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... remoting/client/display/gl_renderer.cc:135: std::unique_ptr<GlRenderer> GlRenderer::CreateGlRendererWithDesktop() { On 2017/01/13 02:03:10, Sergey Ulanov wrote: > This code looks weird. If the GlRenderer owns all the Drawables, why do we have > to add them to drawables_ here instead of in the constructor? > > It feels like GlRenderer should really be split into two layers as follows: > 1. GlRender: holds list of drawables and knows how to draw them, but doesn't > know what they are > 2. DesktopRenderer: owns GlRenderer and all the drawables we need. Knows how to > update the drawables (e.g. move the cursor, etc.) > > WDYT? This is close to what the plan is. A single object that is the endpoint for the events I think is the wrong strategy. I would like to see it decoupled so each object that understands how to draw a component is tied to the outlet of that event from the decode/protocol side maybe with a router of some type to help shuffle the messages into where they need to go. In the end I would like to add the ability to add other drawables, like a FPS stat display or help message bubble into the rendering layer with very little codechange. https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... remoting/client/display/gl_renderer.cc:139: std::sort(drawables.begin(), drawables.end(), On 2017/01/13 02:03:10, Sergey Ulanov wrote: > This is way more complicated than necessary. We have just 3 drawables and the > order is known in advance. Is it not possible to just initialize the drawables_ > list in the proper order without sorting anything? that is if you assume nothing can be added or removed at runtime, which will not be true always. https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... remoting/client/display/gl_renderer.cc:141: for (auto* drawable : drawables) { Why would you sort them, they are going to be sorted as they are added. This is just duplicating code to avoid sorting a list of a very small amount of elements a couple times. No need to optimize this at all as it only happens once per session. webRTC is way slower than this operation. https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... remoting/client/display/gl_renderer.cc:157: void GlRenderer::AddDrawable(base::WeakPtr<Drawable> drawable) { On 2017/01/13 02:21:08, Yuwei wrote: > On 2017/01/13 02:03:10, Sergey Ulanov wrote: > > Is this function still used anywhere? > > I think the unit test is still using it. Also IIRC Scott has some fancy demo > Drawable for debugging that has not been checked in? This method is the entire point of my last commit. I am attempting to make the entire renderer dynamic with layers than can be added and removed at will. If this is removed then my work has been for nothing.
Sorry for the lack of communication before sending out this CL. Most classes are able to be created on one thread and thereafter used on another thread, so I wanted to keep CreateGlDesktopWithRenderer thread unspecified. Since fixing this requires lots of unnecessary logic, I agree that we should simply enforce GlRenderer to be created on the display thread. Let's chat after I'm back. <nicholss@chromium.org>于2017年1月13日 周五上午8:45写道： > > > > > > > > > > > I do not see a test that added that proves this has any issue currently. > The > change described here is counter to the direction I was trying to bring > the code > and breaks the future integration with iOS. I was asked to commit in > stages so > the fact that some functions are not used at the moment is only a > consequence of > a multi-commit effort on my side. > > I am saddened by the lack of communication around this change. A lot of > effort > and wasted time could have been avoided with a simple conversation. I > think this > is an underlaying culture problem in our team we need to band together to > fix. I > think I am also to blame for my lack of design doc on the future of the > renderer > on mobile. I did not imagine there would be competing changes on-top of my > changes, before the rest of the migration could be finished. > > I would be happy to work together to fix any threading issues that may have > resulted in my previous change if provided with a unit test that allows me > to > see the error. As it stood, all unit tests passed. We have too many target > architectures to depend on manually testing each individually. > > > > https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... > File remoting/client/display/gl_cursor.cc (right): > > > https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... > remoting/client/display/gl_cursor.cc:108: base::WeakPtr<Drawable> > GlCursor::GetWeakPtr() { > On 2017/01/13 02:03:10, Sergey Ulanov wrote: > > Does this function really need to be thread-safe? The renderer overall > should be > > single-threaded, so I don't see any case when it can be useful to call > this > > function on any thread other than the one where it's created. > > I think in the future it will be accessed via many threads potentially. > > > > https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... > File remoting/client/display/gl_cursor.h (left): > > > https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... > remoting/client/display/gl_cursor.h:64: base::ThreadChecker > thread_checker_; > > For the current usage, these classes could get away with just assuming > it is being called on the network thread but that is because this system > was designed highly coupled and my goals have been to break up the graph > and allow other classes to add and control the drawable elements. > > For example: the renderer should not be the class that delegates the > drawing of the decoded frames to the desktop. It should be the direct > responsibility of the desktop or related class to push that to the > desktop object and then the desktop object should request a render cycle > from the renderer. The renderer should not be tied to the implementation > of chromoting at all. So in this future, these classes are independent > from the renderer and potentially getting called from a thread that is > not the rendering thread, and thread checker is a good idea. > > > https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... > File remoting/client/display/gl_renderer.cc (left): > > > https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... > remoting/client/display/gl_renderer.cc:175: std::unique_ptr<GlRenderer> > GlRenderer::CreateGlRendererWithDesktop() { > I think it is a mistake to remove this and use the other version. too > much duplicated logic and it breaks the future expandability of the > renderer. > > > > https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... > File remoting/client/display/gl_renderer.cc (right): > > > https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... > remoting/client/display/gl_renderer.cc:23: template <typename > DrawablePtr> > I don't understand why this is better. > > > > https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... > remoting/client/display/gl_renderer.cc:135: std::unique_ptr<GlRenderer> > GlRenderer::CreateGlRendererWithDesktop() { > On 2017/01/13 02:03:10, Sergey Ulanov wrote: > > This code looks weird. If the GlRenderer owns all the Drawables, why > do we have > > to add them to drawables_ here instead of in the constructor? > > > > It feels like GlRenderer should really be split into two layers as > follows: > > 1. GlRender: holds list of drawables and knows how to draw them, but > doesn't > > know what they are > > 2. DesktopRenderer: owns GlRenderer and all the drawables we need. > Knows how to > > update the drawables (e.g. move the cursor, etc.) > > > > WDYT? > > This is close to what the plan is. A single object that is the endpoint > for the events I think is the wrong strategy. I would like to see it > decoupled so each object that understands how to draw a component is > tied to the outlet of that event from the decode/protocol side maybe > with a router of some type to help shuffle the messages into where they > need to go. > > In the end I would like to add the ability to add other drawables, like > a FPS stat display or help message bubble into the rendering layer with > very little codechange. > > > > https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... > remoting/client/display/gl_renderer.cc:139: std::sort(drawables.begin(), > drawables.end(), > On 2017/01/13 02:03:10, Sergey Ulanov wrote: > > This is way more complicated than necessary. We have just 3 drawables > and the > > order is known in advance. Is it not possible to just initialize the > drawables_ > > list in the proper order without sorting anything? > > that is if you assume nothing can be added or removed at runtime, which > will not be true always. > > > https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... > remoting/client/display/gl_renderer.cc:141: for (auto* drawable : > drawables) { > Why would you sort them, they are going to be sorted as they are added. > This is just duplicating code to avoid sorting a list of a very small > amount of elements a couple times. No need to optimize this at all as it > only happens once per session. webRTC is way slower than this operation. > > > > https://codereview.chromium.org/2623413004/diff/1/remoting/client/display/gl_... > remoting/client/display/gl_renderer.cc:157: void > GlRenderer::AddDrawable(base::WeakPtr<Drawable> drawable) { > On 2017/01/13 02:21:08, Yuwei wrote: > > On 2017/01/13 02:03:10, Sergey Ulanov wrote: > > > Is this function still used anywhere? > > > > I think the unit test is still using it. Also IIRC Scott has some > fancy demo > > Drawable for debugging that has not been checked in? > > This method is the entire point of my last commit. I am attempting to > make the entire renderer dynamic with layers than can be added and > removed at will. If this is removed then my work has been for nothing. > > https://codereview.chromium.org/2623413004/ > > > > > > > > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
PTAL. I've rewritten the CL such that JniGlDisplayHandler::Core creates GlRenderer on display thread. Ideally GlRenderer can still be created on any thread if we can detach the thread checkers of the WeakPtrs but it doesn't look like we can do that... Discussed offline with Scott. He has a refactoring plan for GlRenderer so I would like to keep this CL minimal to just fix the thread issue.
On 2017/01/13 20:17:27, Yuwei wrote: > PTAL. > > I've rewritten the CL such that JniGlDisplayHandler::Core creates GlRenderer on > display thread. > > Ideally GlRenderer can still be created on any thread if we can detach the > thread checkers of the WeakPtrs but it doesn't look like we can do that... > > Discussed offline with Scott. He has a refactoring plan for GlRenderer so I > would like to keep this CL minimal to just fix the thread issue. Ping O_o
On 2017/01/17 19:48:52, Yuwei wrote: > On 2017/01/13 20:17:27, Yuwei wrote: > > PTAL. > > > > I've rewritten the CL such that JniGlDisplayHandler::Core creates GlRenderer > on > > display thread. > > > > Ideally GlRenderer can still be created on any thread if we can detach the > > thread checkers of the WeakPtrs but it doesn't look like we can do that... > > > > Discussed offline with Scott. He has a refactoring plan for GlRenderer so I > > would like to keep this CL minimal to just fix the thread issue. > > Ping O_o Any way to write a unit test for this?
On 2017/01/17 19:52:23, nicholss wrote: > On 2017/01/17 19:48:52, Yuwei wrote: > > On 2017/01/13 20:17:27, Yuwei wrote: > > > PTAL. > > > > > > I've rewritten the CL such that JniGlDisplayHandler::Core creates GlRenderer > > on > > > display thread. > > > > > > Ideally GlRenderer can still be created on any thread if we can detach the > > > thread checkers of the WeakPtrs but it doesn't look like we can do that... > > > > > > Discussed offline with Scott. He has a refactoring plan for GlRenderer so I > > > would like to keep this CL minimal to just fix the thread issue. > > > > Ping O_o > > Any way to write a unit test for this? I think the issue is now that we enforce a strict single thread restriction on GlRenderer, constructing the GlRenderer on a different thread is not even a use case or allowed by the class definition.
On 2017/01/17 20:00:29, Yuwei wrote: > On 2017/01/17 19:52:23, nicholss wrote: > > On 2017/01/17 19:48:52, Yuwei wrote: > > > On 2017/01/13 20:17:27, Yuwei wrote: > > > > PTAL. > > > > > > > > I've rewritten the CL such that JniGlDisplayHandler::Core creates > GlRenderer > > > on > > > > display thread. > > > > > > > > Ideally GlRenderer can still be created on any thread if we can detach the > > > > thread checkers of the WeakPtrs but it doesn't look like we can do that... > > > > > > > > Discussed offline with Scott. He has a refactoring plan for GlRenderer so > I > > > > would like to keep this CL minimal to just fix the thread issue. > > > > > > Ping O_o > > > > Any way to write a unit test for this? > > I think the issue is now that we enforce a strict single thread restriction on > GlRenderer, constructing the GlRenderer on a different thread is not even a use > case or allowed by the class definition. What I meant is there is probably no new unit test we can add to GlRenderer to make sure everything still works. We may add test to JniGlDisplayHandler::Core instead but currently we haven't done anything on it yet...
lgtm when my comments are addressed https://codereview.chromium.org/2623413004/diff/20001/remoting/client/jni/jni... File remoting/client/jni/jni_gl_display_handler.cc (right): https://codereview.chromium.org/2623413004/diff/20001/remoting/client/jni/jni... remoting/client/jni/jni_gl_display_handler.cc:85: Initialize(); runtime_->display_task_runner()->PostTask( FROM_HERE, base::Bind(&JniGlDisplayHandler::Core::Initialize, base::Unretained(this))); https://codereview.chromium.org/2623413004/diff/20001/remoting/client/jni/jni... remoting/client/jni/jni_gl_display_handler.cc:197: if (!runtime_->display_task_runner()->BelongsToCurrentThread()) { PostTask in the constructor and replace this code with DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread()). https://codereview.chromium.org/2623413004/diff/20001/remoting/client/jni/jni... remoting/client/jni/jni_gl_display_handler.cc:200: weak_ptr_)); Don't need to use weak pointer here because Core is destroyed on the same thread.
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2623413004/diff/20001/remoting/client/jni/jni... File remoting/client/jni/jni_gl_display_handler.cc (right): https://codereview.chromium.org/2623413004/diff/20001/remoting/client/jni/jni... remoting/client/jni/jni_gl_display_handler.cc:85: Initialize(); On 2017/01/18 22:33:40, Sergey Ulanov wrote: > runtime_->display_task_runner()->PostTask( > FROM_HERE, base::Bind(&JniGlDisplayHandler::Core::Initialize, > base::Unretained(this))); > Since we still need the weak pointer for other purposes, why not just use weak pointer here so that we don't need to check the thread safety in mind when reading this code?
https://codereview.chromium.org/2623413004/diff/20001/remoting/client/jni/jni... File remoting/client/jni/jni_gl_display_handler.cc (right): https://codereview.chromium.org/2623413004/diff/20001/remoting/client/jni/jni... remoting/client/jni/jni_gl_display_handler.cc:85: Initialize(); On 2017/01/18 23:11:56, Yuwei wrote: > On 2017/01/18 22:33:40, Sergey Ulanov wrote: > > runtime_->display_task_runner()->PostTask( > > FROM_HERE, base::Bind(&JniGlDisplayHandler::Core::Initialize, > > base::Unretained(this))); > > > > Since we still need the weak pointer for other purposes, why not just use weak > pointer here so that we don't need to check the thread safety in mind when > reading this code? The "Core" class used here to switch to a different thread is a common pattern that is used in many other places. This pattern doesn't need WeakPtr. Some issues I see with using weak_ptr here: 1. Someone reads this code, recognizes the pattern, but gets confused by WeakPtr. 2. Someone removes weak_ptr dependency from DualBufferFrameConsumer, but assumes it's still needed for this PostTask() keeps weak_factory_. 3. Someone uses this code as reference for the pattern, assumes that weak_ptr is meaningful here, and copies it in a new class. Overall WeakPtr is not a silver-bullet. If it's used here it doesn't automatically make this PostTask call safe (particularly because WeakPtr is not thread-safe). You are just trading one set of potential issues for another.
Thanks! https://codereview.chromium.org/2623413004/diff/20001/remoting/client/jni/jni... File remoting/client/jni/jni_gl_display_handler.cc (right): https://codereview.chromium.org/2623413004/diff/20001/remoting/client/jni/jni... remoting/client/jni/jni_gl_display_handler.cc:85: Initialize(); On 2017/01/18 23:32:43, Sergey Ulanov wrote: > On 2017/01/18 23:11:56, Yuwei wrote: > > On 2017/01/18 22:33:40, Sergey Ulanov wrote: > > > runtime_->display_task_runner()->PostTask( > > > FROM_HERE, base::Bind(&JniGlDisplayHandler::Core::Initialize, > > > base::Unretained(this))); > > > > > > > Since we still need the weak pointer for other purposes, why not just use weak > > pointer here so that we don't need to check the thread safety in mind when > > reading this code? > > The "Core" class used here to switch to a different thread is a common pattern > that is used in many other places. This pattern doesn't need WeakPtr. Some > issues I see with using weak_ptr here: > 1. Someone reads this code, recognizes the pattern, but gets confused by > WeakPtr. > 2. Someone removes weak_ptr dependency from DualBufferFrameConsumer, but > assumes it's still needed for this PostTask() keeps weak_factory_. > 3. Someone uses this code as reference for the pattern, assumes that weak_ptr > is meaningful here, and copies it in a new class. > > Overall WeakPtr is not a silver-bullet. If it's used here it doesn't > automatically make this PostTask call safe (particularly because WeakPtr is not > thread-safe). You are just trading one set of potential issues for another. Acknowledged. Done. https://codereview.chromium.org/2623413004/diff/20001/remoting/client/jni/jni... remoting/client/jni/jni_gl_display_handler.cc:197: if (!runtime_->display_task_runner()->BelongsToCurrentThread()) { On 2017/01/18 22:33:40, Sergey Ulanov wrote: > PostTask in the constructor and replace this code with > DCHECK(runtime_->display_task_runner()->BelongsToCurrentThread()). Done. https://codereview.chromium.org/2623413004/diff/20001/remoting/client/jni/jni... remoting/client/jni/jni_gl_display_handler.cc:200: weak_ptr_)); On 2017/01/18 22:33:40, Sergey Ulanov wrote: > Don't need to use weak pointer here because Core is destroyed on the same > thread. 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/2623413004/#ps60001 (title: "Reviewer's Feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484785822160710,
"parent_rev": "67d828ef9b351a52fde60df3bf0cc1326bf2fd69", "commit_rev":
"3ce1a24a7e23be0e5f01d38a943b51cf87a01895"}
Message was sent while issue was closed.
Description was changed from ========== [Remoting Android] Fix thread issue with OpenGL drawable This CL has these changes: * Store drawable's weak_ptr in ctor rather than using thread checker to check the thread safety of GetWeakPtr() since GetWeakPtr() is called in multiple threads. * Avoid all attempts in CreateGlRendererWithDesktop() to dereference any weak pointer since CreateGlRendererWithDesktop() is supposed to be called on any thread. * DCHECK the thread in AddDrawable(). AddDrawable is never safe to be called on a different thread once GlRenderer gets any side effect. BUG=680800 ========== to ========== [Remoting Android] Fix thread issue with OpenGL drawable This CL has these changes: * Store drawable's weak_ptr in ctor rather than using thread checker to check the thread safety of GetWeakPtr() since GetWeakPtr() is called in multiple threads. * Avoid all attempts in CreateGlRendererWithDesktop() to dereference any weak pointer since CreateGlRendererWithDesktop() is supposed to be called on any thread. * DCHECK the thread in AddDrawable(). AddDrawable is never safe to be called on a different thread once GlRenderer gets any side effect. BUG=680800 Review-Url: https://codereview.chromium.org/2623413004 Cr-Commit-Position: refs/heads/master@{#444574} Committed: https://chromium.googlesource.com/chromium/src/+/3ce1a24a7e23be0e5f01d38a943b... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/3ce1a24a7e23be0e5f01d38a943b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
