904245 - Lag when drawing with the S-Pen on a canvas (Galaxy Note)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[ofirr.dev]

User Agent: Mozilla/5.0 (compatible; MSIE 10.0; Windows NT 6.1; WOW64; Trident/6.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; .NET4.0C)

Steps to reproduce:

Trying to draw or write with the S-Pen on a canvas in a Galaxy Note.
Test page:
https://bug827247.bugzilla.mozilla.org/attachment.cgi?id=698584

There is a similar issue on the desktop:
https://bugzilla.mozilla.org/show_bug.cgi?id=827247


Actual results:

When pressing the pen and starting to move it, there is a lag before FF starts drawing on the screen. This forces the user to draw and write very slowly and the result looks bad.


Expected results:

The S-Pen should be responsive like it is in native Android apps.

Comment 1

[Aaron Train [:aaronmt]]

Do you see better performance using Nightly for Android (http://nightly.mozilla.org)? ALso which version of Android are you testing with?

Comment 2

[ofirr.dev]

Android 4.0 and 4.1

Can't test with Nightly.

Comment 3

[Wesley Johnston (:wesj)]

So there's a bit of intentional "lag" we had to add here for legacy web content. Namely, we avoid sending touchmove until you've moved some minimum distance:

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/AsyncPanZoomController.cpp#465

I'm going to try removing that threshold if you're using a device that ISN'T a finger. Or maybe if the touch radius is less than a minimum would be better. We can try both.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: fix3

Here's something that might help with our initial lag when drawing with a pen. This uses the input device's touch radius to determine how big our PAN_THRESHOLD should be (we should sync that number up with gfx.azpc.touch_start_tolerance somewhere else maybe...) That lets me at least draw much smaller things on the demo page, but double taps on google maps still work

There's a build with this at:
http://people.mozilla.com/~wjohnston/touchlag.apk

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 790916 [details] [diff] [review]
fix3

Review of attachment 790916 [details] [diff] [review]:
-----------------------------------------------------------------

I'm a little concerned that this will blow up on us on devices that don't populate the touch-minor value properly. If that is set to 0 on a device then we'll break google maps. Do you know if we can detect the S-Pen? I would assume that it sends stylus events or something, so we should be able to do this just in that case. Minusing for now, but open to discussion here.

Comment 6

[ofirr.dev]

There must be a way to detect stylus because pointer events expose this info to javascript
https://bugzilla.mozilla.org/show_bug.cgi?id=822898

Comment 7

[Wesley Johnston (:wesj)]

Stylus support for Android wasn't added until ICS. There's no way to detect the S-Pen on all Note's beyond looking at the radius (I'm not sure if the ICS Notes or newer ones use the new api's or not either).

TBH, I think radius is a better way to do this than filtering on device type anyway though. I'll look at refining how we get this number and ensuring its always > 0.

Comment 8

[ofirr.dev]

Wesley, the link to the apk seems to be broken.

Is the package signed? Am I supposed to be able to install it as usual or do I need to enter a developer mode?

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Patch v2

This is a little more advanced and puts a minimum size on the radius as well as the maximum. I found a minimum by playing guess and check a bit with a pen. It makes double tapping on Maps with the pen not quite as easy as I'd like, but also lets you draw very small circles in the demo page.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 795647 [details] [diff] [review]
Patch v2

I don't have overall problems with this approach but the patch has some bugs that need addressing first.

>diff --git a/mobile/android/base/GeckoEvent.java b/mobile/android/base/GeckoEvent.java
>+    private static Resources mResources;
>+    private DisplayMetrics displaymetrics;
> 

The displayMetrics field is never used.

>+    public static Point GetEventRadius(MotionEvent event, int index, float orientation) {
>+        if (Build.VERSION.SDK_INT >= 9) {
>+            // the radius is found by removing the orientation and measuring the x and y
>+            // radius of the resulting ellipse
>+            // for android orientations >= 0 and < 90, the major axis should correspond to
>+            // just reporting the y radius as the major one, and x as minor
>+            // however, for a radius < 0, we have to shift the orientation by adding 90, and
>+            // reverse which radius is major and minor
>+            if (orientation < 0) {
>+                orientation += 90;

This looks like a bad refactoring; the orientation variable passed here from the original code will already have 90 added to it so this condition needs to be adjusted. Also adding 90 here is unnecessary since this variable is not used past this point.

>+            DisplayMetrics displaymetrics = mResources.getDisplayMetrics();
>+            size = size*Math.min(displaymetrics.heightPixels, displaymetrics.widthPixels);

nit: spaces around *

>diff --git a/mobile/android/base/gfx/LayerView.java b/mobile/android/base/gfx/LayerView.java
>+import android.app.Activity;
>+import android.graphics.Point;
>+import android.util.DisplayMetrics;

Some of these are not meeded.

Comment 11

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 796295 [details] [diff] [review]
Patch v2

Review of attachment 796295 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoEvent.java
@@ +434,5 @@
>              mPoints[index] = new Point(0, 0);
>          }
>      }
>  
> +    public static Point GetEventRadius(MotionEvent event, int index, float orientation) {

rename to getEventRadius (camel case)

@@ +442,5 @@
> +            // for android orientations >= 0 and < 90, the major axis should correspond to
> +            // just reporting the y radius as the major one, and x as minor
> +            // however, for a radius < 0, we have to shift the orientation by adding 90, and
> +            // reverse which radius is major and minor
> +            if (orientation < 0) {

This still doesn't make sense. In the normal code path, where this is called from addMotionPoint, the orientation has already been changed. orientation will never be < 0 when this is called from addMotionPoint.

Comment 13

[ofirr.dev]

.

Comment 14

[Wesley Johnston (:wesj)]

Attachment: Patch

I tried for awhile to combine that logic and am giving up :) We're doing a lot of math there to convert Android orientations to w3c orientations. We don't need any of it here, so it seems like overkill to try sharing anyway.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 816146 [details] [diff] [review]
Patch

Review of attachment 816146 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comment addressed

::: mobile/android/base/gfx/LayerView.java
@@ +135,5 @@
> +
> +        float size = event.getSize();
> +        DisplayMetrics displaymetrics = getContext().getResources().getDisplayMetrics();
> +        size = size*Math.min(displaymetrics.heightPixels, displaymetrics.widthPixels);
> +        return new Point((int)size,(int)size);

spaces around the '*' operator and between the arguments to new Point.

Comment 16

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/e6406d223b50

Comment 17

[Wesley Johnston (:wesj)]

Grr. Forgot the whitespace:
https://hg.mozilla.org/integration/fx-team/rev/863087fdc7da

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e6406d223b50
https://hg.mozilla.org/mozilla-central/rev/863087fdc7da