[Chromoting] Feedback animation has stopped

remoting/android/java/src/org/chromium/chromoting/DesktopView.java

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.chromoting;
 
 import android.content.Context;
 import android.graphics.Bitmap;
 import android.graphics.Canvas;
 import android.graphics.Color;
@@ skipping 80 matching lines @@
         Preconditions.isNull(mDesktop);
         Preconditions.isNull(mClient);
         Preconditions.notNull(desktop);
         Preconditions.notNull(client);
         mDesktop = desktop;
         mClient = client;
         mInputHandler.init(desktop, new InputEventSender(client));
     }
 
     public Event<PaintEventParameter> onPaint() {
         return mOnPaint;

[Lambros, 2016/06/28 17:53:44]

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.

[Lambros, 2016/06/28 18:09:20]

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.

[Hzj_jie, 2016/06/28 18:57:49]

Done.

[Hzj_jie, 2016/06/28 18:57:49]

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.

     }
 
     public Event<SizeChangedEventParameter> onClientSizeChanged() {
         return mOnClientSizeChanged;
     }
 
     public Event<SizeChangedEventParameter> onHostSizeChanged() {
         return mOnHostSizeChanged;
     }
 
@@ skipping 82 matching lines @@
             Bitmap cursorBitmap = mClient.getDisplay().getCursorBitmap();
             if (cursorBitmap != null) {
                 Point hotspot = mClient.getDisplay().getCursorHotspot();
                 canvas.drawBitmap(cursorBitmap, cursorPosition.x - hotspot.x,
                         cursorPosition.y - hotspot.y, new Paint());
             }
         }
 
         getHolder().unlockCanvasAndPost(canvas);
 
-        synchronized (mAnimationLock) {
-            if (mInputAnimationRunning) {
-                getHandler().postAtTime(new Runnable() {
-                    @Override
-                    public void run() {
-                        processAnimation();
-                    }
-                }, startTimeMs + 30);
+        getHandler().postAtTime(new Runnable() {
+            @Override

[Lambros, 2016/06/27 23:16:16]

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?

[Hzj_jie, 2016/06/27 23:45:45]

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.

[Yuwei, 2016/06/28 04:26:51]

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...

[Yuwei, 2016/06/28 08:43:40]

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

[Hzj_jie, 2016/06/28 18:57:49]

Yes, that's the plan. This root cause of this bug is the mix of callback pattern
and event pattern.

+            public void run() {
+                processAnimation();
             }
-        }
+        }, startTimeMs + 30);
     }
 
     private void processAnimation() {
         boolean running;
         synchronized (mAnimationLock) {
             running = mInputAnimationRunning;
         }
         if (running) {
             mInputHandler.processAnimation();
             requestRepaint();
         } else if (!mOnPaint.isEmpty()) {

[Lambros, 2016/06/27 23:16:16]

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?

[Hzj_jie, 2016/06/27 23:45:45]

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.

[Yuwei, 2016/06/28 04:26:51]

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 :)

[Hzj_jie, 2016/06/28 18:57:49]

Yes, we will remove mAnimationLock and mInputAnimationRunning.

             requestRepaint();
         }
     }
 
     /**
      * Called after the canvas is initially created, then after every subsequent resize, as when
      * the display is rotated.
      */
     @Override
     public void surfaceChanged(SurfaceHolder holder, int format, int width, int height) {
@@ skipping 85 matching lines @@
     public void transformationChanged() {
         requestRepaint();
     }
 
     @Override
     public void setAnimationEnabled(boolean enabled) {
         synchronized (mAnimationLock) {
             if (enabled && !mInputAnimationRunning) {
                 requestRepaint();
             }
             mInputAnimationRunning = enabled;

[Sergey Ulanov, 2016/06/28 00:13:33]

Should this be set before requestRepaint() is called? Is repaint always
asychronous?

Also, it's better to call requestRepaint() without holding the lock.

[Hzj_jie, 2016/06/28 00:22:32]

Yes, requestRepaint just schedules a task in native rendering thread.

[Yuwei, 2016/06/28 00:28:09]

I think it is safe to remove the lock. After mInputAnimationRunning is removed
from paint(), it will only be used on the UI thread.

[Hzj_jie, 2016/06/28 18:57:49]

No matter, I will remove the mix of callback pattern and event pattern soon.
Meanwhile, mAnimationLock and mInputAnimationRunning will be removed.

         }
     }
 }