Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2101783002:
[Chromoting] Feedback animation has stopped (Closed)
Description[Chromoting] Feedback animation has stopped
This change fixes a code defect in DesktopView, which causes feedback animation
won't execute correctly and stop in an unexpected progress.
BUG=623725
Committed: https://crrev.com/09f89f87c27a71547b5304ed8dccc7a7dda6e7b7
Cr-Commit-Position: refs/heads/master@{#402885}
Patch Set 1 #
Total comments: 17
Patch Set 2 : Add comments to explain onPaint behavior. #
Total comments: 2
Patch Set 3 : Resolve review comments #
Total comments: 2
Messages
Total messages: 29 (6 generated)
Description was changed from ========== [Chromoting] Feedback animation has stopped This change fixes a code defect in DesktopView, which causes feedback animation won't execute correctly and stop in an unexpected progress. BUG=623725 ========== to ========== [Chromoting] Feedback animation has stopped This change fixes a code defect in DesktopView, which causes feedback animation won't execute correctly and stop in an unexpected progress. BUG=623725 ==========
zijiehe@chromium.org changed reviewers: + lambroslambrou@chromium.org, yuweih@chromium.org
https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:205: @Override I don't understand why removing the mInputAnimationRunning check should fix the bug? We don't want to have paint() called every 30ms, do we, if there's no animation running? https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:220: } else if (!mOnPaint.isEmpty()) { I don't understand this logic. Why would we trigger more painting here? Is this going to paint every 30ms, even when nothing is happening?
https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:205: @Override On 2016/06/27 23:16:16, Lambros wrote: > I don't understand why removing the mInputAnimationRunning check should fix the > bug? > We don't want to have paint() called every 30ms, do we, if there's no animation > running? FeedbackAnimator adds itself as SelfRemovableParameterRunnable into mOnPaint, so if mOnPaint is not empty, we will need to repaint it in the next round. After we moving AnimationJob into the same implementation, we do not need to lock and change mInputAnimationRunning anymore. https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:220: } else if (!mOnPaint.isEmpty()) { On 2016/06/27 23:16:16, Lambros wrote: > I don't understand this logic. Why would we trigger more painting here? Is this > going to paint every 30ms, even when nothing is happening? Same as above, the only difference is we will render one more frame after SelfRemovableParameterRunnable has finished. i.e. A SelfRemovableParameterRunnable will remove itself from mOnPaint after a run call.
sergeyu@chromium.org changed reviewers: + sergeyu@chromium.org
Is that bug a regression? If yes, do you know which CL caused it? https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:326: mInputAnimationRunning = enabled; Should this be set before requestRepaint() is called? Is repaint always asychronous? Also, it's better to call requestRepaint() without holding the lock.
On 2016/06/28 00:13:33, Sergey Ulanov wrote: > Is that bug a regression? If yes, do you know which CL caused it? > > https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... > File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): > > https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... > remoting/android/java/src/org/chromium/chromoting/DesktopView.java:326: > mInputAnimationRunning = enabled; > Should this be set before requestRepaint() is called? Is repaint always > asychronous? > > Also, it's better to call requestRepaint() without holding the lock. It's introduced by change 06d70696de0b8fac307927064bd36be2d486fe2a, and partially fixed by change 53e826563f4c551bb640ec10eb21ed40b1687141. Yes, it should not block M52, as 2743 does not include both changes.
https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:326: mInputAnimationRunning = enabled; On 2016/06/28 00:13:33, Sergey Ulanov wrote: > Should this be set before requestRepaint() is called? Is repaint always > asychronous? > > Also, it's better to call requestRepaint() without holding the lock. Yes, requestRepaint just schedules a task in native rendering thread.
https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:326: mInputAnimationRunning = enabled; On 2016/06/28 00:13:33, Sergey Ulanov wrote: > Should this be set before requestRepaint() is called? Is repaint always > asychronous? > > Also, it's better to call requestRepaint() without holding the lock. I think it is safe to remove the lock. After mInputAnimationRunning is removed from paint(), it will only be used on the UI thread.
https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:205: @Override On 2016/06/27 23:16:16, Lambros wrote: > I don't understand why removing the mInputAnimationRunning check should fix the > bug? > We don't want to have paint() called every 30ms, do we, if there's no animation > running? If I understand it right, the bug is: * When calling showInputFeedback(), a FeedbackAnimator is attached to the |mOnPaint| event and requestRepaint() is called. * In the middle of paint(), mOnPaint is triggered and the feedback circle is drawn. * On line 205, since mInputAnimationRunning has not been set to true, processAnimation won't be called and nothing will happen after the first frame is rendered :( Removing the check will force processAnimation() to be called, which will keep requesting repaint until FeedbackAnimator removes itself from the event listener list, which fixes this problem... processAnimation() will only request repaint when necessary but I still don't feel right to always call/schedule processAnimation() even if there is no animation and it has no side effect... https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:220: } else if (!mOnPaint.isEmpty()) { On 2016/06/27 23:45:45, Hzj_jie wrote: > On 2016/06/27 23:16:16, Lambros wrote: > > I don't understand this logic. Why would we trigger more painting here? Is > this > > going to paint every 30ms, even when nothing is happening? > > Same as above, the only difference is we will render one more frame after > SelfRemovableParameterRunnable has finished. i.e. A > SelfRemovableParameterRunnable will remove itself from mOnPaint after a run > call. Could we do something like: if (mInputHandler.processAnimation() || !mOnPaint.isEmpty()) { requestRepaint(); } and make mInputHandler.processAnimation() return true if the animation is not done yet? If it turns out we still need the mInputAnimationRunning flag somehow then don't need to take my suggestion :)
https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:205: @Override On 2016/06/27 23:45:45, Hzj_jie wrote: > On 2016/06/27 23:16:16, Lambros wrote: > > I don't understand why removing the mInputAnimationRunning check should fix > the > > bug? > > We don't want to have paint() called every 30ms, do we, if there's no > animation > > running? > > FeedbackAnimator adds itself as SelfRemovableParameterRunnable into mOnPaint, so > if mOnPaint is not empty, we will need to repaint it in the next round. After we > moving AnimationJob into the same implementation, we do not need to lock and > change mInputAnimationRunning anymore. I think if you use mOnPaint for TouchInputHandler animations then you can remove both mInputAnimationRunning and processAnimation() and do something like: mOnPaint.raise(...); if (!mOnPaint.isEmpty()) { getHandler().postAtTime(requestRepaint, startTimeMs + 30); } in the painting loop
On 2016/06/28 08:43:40, Yuwei wrote: > https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... > File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): > > https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... > remoting/android/java/src/org/chromium/chromoting/DesktopView.java:205: > @Override > On 2016/06/27 23:45:45, Hzj_jie wrote: > > On 2016/06/27 23:16:16, Lambros wrote: > > > I don't understand why removing the mInputAnimationRunning check should fix > > the > > > bug? > > > We don't want to have paint() called every 30ms, do we, if there's no > > animation > > > running? > > > > FeedbackAnimator adds itself as SelfRemovableParameterRunnable into mOnPaint, > so > > if mOnPaint is not empty, we will need to repaint it in the next round. After > we > > moving AnimationJob into the same implementation, we do not need to lock and > > change mInputAnimationRunning anymore. > > I think if you use mOnPaint for TouchInputHandler animations then you can remove > both mInputAnimationRunning and processAnimation() and do something like: > > mOnPaint.raise(...); > if (!mOnPaint.isEmpty()) { > getHandler().postAtTime(requestRepaint, startTimeMs + 30); > } > > in the painting loop Why do we use SelfRemovable... for FeedbackAnimator? I didn't know this. This implies that the animation only runs for 1 frame. What re-adds the ParameterRunnable to draw more frames of the animation? If you're going to use mOnPaint for animations, the animation-handler itself should decide when it's finished and should remove itself?
https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:101: return mOnPaint; Why is this publicly exposed, if you're only using this for animations? If somebody calls this method, and adds a ParameterRunnable to the event, it will become non-empty, and it will cause painting to be constantly triggered every 30ms until the ParameterRunnable is removed. Is this by design? If so, please document this.
On 2016/06/28 17:45:46, Lambros wrote: > On 2016/06/28 08:43:40, Yuwei wrote: > > > https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... > > File remoting/android/java/src/org/chromium/chromoting/DesktopView.java > (right): > > > > > https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... > > remoting/android/java/src/org/chromium/chromoting/DesktopView.java:205: > > @Override > > On 2016/06/27 23:45:45, Hzj_jie wrote: > > > On 2016/06/27 23:16:16, Lambros wrote: > > > > I don't understand why removing the mInputAnimationRunning check should > fix > > > the > > > > bug? > > > > We don't want to have paint() called every 30ms, do we, if there's no > > > animation > > > > running? > > > > > > FeedbackAnimator adds itself as SelfRemovableParameterRunnable into > mOnPaint, > > so > > > if mOnPaint is not empty, we will need to repaint it in the next round. > After > > we > > > moving AnimationJob into the same implementation, we do not need to lock and > > > change mInputAnimationRunning anymore. > > > > I think if you use mOnPaint for TouchInputHandler animations then you can > remove > > both mInputAnimationRunning and processAnimation() and do something like: > > > > mOnPaint.raise(...); > > if (!mOnPaint.isEmpty()) { > > getHandler().postAtTime(requestRepaint, startTimeMs + 30); > > } > > > > in the painting loop > > Why do we use SelfRemovable... for FeedbackAnimator? I didn't know this. > This implies that the animation only runs for 1 frame. What re-adds the > ParameterRunnable to draw more frames of the animation? > > If you're going to use mOnPaint for animations, the animation-handler > itself should decide when it's finished and should remove itself? I think the SelfRemovable will only remove the listener if the run method returns false
I think I get it now! SelfRemovable.. doesn't say "remove after one shot", it says "remove when the handler returns true". So the animation code returns false within the time of the animation, and true once the animation is finished. You are updating paint() to say "keep painting as long as we have pending animations registered to onPaint() event" https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:101: return mOnPaint; On 2016/06/28 17:53:44, Lambros wrote: > Why is this publicly exposed, if you're only using this for animations? > > If somebody calls this method, and adds a ParameterRunnable to the event, > it will become non-empty, and it will cause painting to be constantly > triggered every 30ms until the ParameterRunnable is removed. > > Is this by design? If so, please document this. > This should be documented in DesktopViewInterface, since this onPaint() method is part of the interface. This is more than simply an event notification. When you add event-handlers to this event, it causes painting to trigger continuously until every handler is removed.
https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:101: return mOnPaint; On 2016/06/28 17:53:44, Lambros wrote: > Why is this publicly exposed, if you're only using this for animations? > > If somebody calls this method, and adds a ParameterRunnable to the event, > it will become non-empty, and it will cause painting to be constantly > triggered every 30ms until the ParameterRunnable is removed. > > Is this by design? If so, please document this. > Done. https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:101: return mOnPaint; On 2016/06/28 18:09:20, Lambros wrote: > On 2016/06/28 17:53:44, Lambros wrote: > > Why is this publicly exposed, if you're only using this for animations? > > > > If somebody calls this method, and adds a ParameterRunnable to the event, > > it will become non-empty, and it will cause painting to be constantly > > triggered every 30ms until the ParameterRunnable is removed. > > > > Is this by design? If so, please document this. > > > > This should be documented in DesktopViewInterface, since this onPaint() method > is part of the interface. This is more than simply an event notification. When > you add event-handlers to this event, it causes painting to trigger continuously > until every handler is removed. > I will add comments in this simple fix. But yes, this is an issue we should address more seriously. As continually repainting usually cannot be caught by manual testing. I am considering a more comprehensive solution for this case. https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:205: @Override On 2016/06/28 08:43:40, Yuwei wrote: > On 2016/06/27 23:45:45, Hzj_jie wrote: > > On 2016/06/27 23:16:16, Lambros wrote: > > > I don't understand why removing the mInputAnimationRunning check should fix > > the > > > bug? > > > We don't want to have paint() called every 30ms, do we, if there's no > > animation > > > running? > > > > FeedbackAnimator adds itself as SelfRemovableParameterRunnable into mOnPaint, > so > > if mOnPaint is not empty, we will need to repaint it in the next round. After > we > > moving AnimationJob into the same implementation, we do not need to lock and > > change mInputAnimationRunning anymore. > > I think if you use mOnPaint for TouchInputHandler animations then you can remove > both mInputAnimationRunning and processAnimation() and do something like: > > mOnPaint.raise(...); > if (!mOnPaint.isEmpty()) { > getHandler().postAtTime(requestRepaint, startTimeMs + 30); > } > > in the painting loop Yes, that's the plan. This root cause of this bug is the mix of callback pattern and event pattern. https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:220: } else if (!mOnPaint.isEmpty()) { On 2016/06/28 04:26:51, Yuwei wrote: > On 2016/06/27 23:45:45, Hzj_jie wrote: > > On 2016/06/27 23:16:16, Lambros wrote: > > > I don't understand this logic. Why would we trigger more painting here? Is > > this > > > going to paint every 30ms, even when nothing is happening? > > > > Same as above, the only difference is we will render one more frame after > > SelfRemovableParameterRunnable has finished. i.e. A > > SelfRemovableParameterRunnable will remove itself from mOnPaint after a run > > call. > > Could we do something like: > > if (mInputHandler.processAnimation() || !mOnPaint.isEmpty()) { > requestRepaint(); > } > > and make mInputHandler.processAnimation() return true if the animation is not > done yet? > > If it turns out we still need the mInputAnimationRunning flag somehow then don't > need to take my suggestion :) Yes, we will remove mAnimationLock and mInputAnimationRunning. https://codereview.chromium.org/2101783002/diff/1/remoting/android/java/src/o... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:326: mInputAnimationRunning = enabled; On 2016/06/28 00:28:09, Yuwei wrote: > On 2016/06/28 00:13:33, Sergey Ulanov wrote: > > Should this be set before requestRepaint() is called? Is repaint always > > asychronous? > > > > Also, it's better to call requestRepaint() without holding the lock. > > I think it is safe to remove the lock. After mInputAnimationRunning is removed > from paint(), it will only be used on the UI thread. No matter, I will remove the mix of callback pattern and event pattern soon. Meanwhile, mAnimationLock and mInputAnimationRunning will be removed.
https://codereview.chromium.org/2101783002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2101783002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:204: getHandler().postAtTime(new Runnable() { I'm not sure whether this is a good solution if it ends up to be the last CL that fixes the problem in M53. Compared to previous versions it always call procesAnimation even if that is a no-op... Are you going to bundle the change of TouchInputHandler.processAnimation with some other changes? I just feel like it is easy to turn it into an event listener pattern...
https://codereview.chromium.org/2101783002/diff/20001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2101783002/diff/20001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:204: getHandler().postAtTime(new Runnable() { On 2016/06/29 01:17:43, Yuwei wrote: > I'm not sure whether this is a good solution if it ends up to be the last CL > that fixes the problem in M53. Compared to previous versions it always call > procesAnimation even if that is a no-op... > > Are you going to bundle the change of TouchInputHandler.processAnimation with > some other changes? I just feel like it is easy to turn it into an event > listener pattern... Done.
https://codereview.chromium.org/2101783002/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2101783002/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:205: if (mInputAnimationRunning || !mOnPaint.isEmpty()) { lgtm as a simple fix of this problem. I think you still need to get LGTM from Lambros in order to be able to commit your CL :-/
lgtm
https://codereview.chromium.org/2101783002/diff/40001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/DesktopView.java (right): https://codereview.chromium.org/2101783002/diff/40001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/DesktopView.java:205: if (mInputAnimationRunning || !mOnPaint.isEmpty()) { On 2016/06/29 02:26:25, Yuwei wrote: > lgtm as a simple fix of this problem. I think you still need to get LGTM from > Lambros in order to be able to commit your CL :-/ Done.
The CQ bit was checked by zijiehe@chromium.org
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 ========== [Chromoting] Feedback animation has stopped This change fixes a code defect in DesktopView, which causes feedback animation won't execute correctly and stop in an unexpected progress. BUG=623725 ========== to ========== [Chromoting] Feedback animation has stopped This change fixes a code defect in DesktopView, which causes feedback animation won't execute correctly and stop in an unexpected progress. BUG=623725 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Feedback animation has stopped This change fixes a code defect in DesktopView, which causes feedback animation won't execute correctly and stop in an unexpected progress. BUG=623725 ========== to ========== [Chromoting] Feedback animation has stopped This change fixes a code defect in DesktopView, which causes feedback animation won't execute correctly and stop in an unexpected progress. BUG=623725 Committed: https://crrev.com/09f89f87c27a71547b5304ed8dccc7a7dda6e7b7 Cr-Commit-Position: refs/heads/master@{#402885} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/09f89f87c27a71547b5304ed8dccc7a7dda6e7b7 Cr-Commit-Position: refs/heads/master@{#402885} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
