Closed Bug 904245 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: Toolbar, defect)

23 Branch
ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 27

People

(Reporter: ofirr.dev, Assigned: wesj)

Details

Attachments

(1 file, 3 obsolete files)

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.
Do you see better performance using Nightly for Android (http://nightly.mozilla.org)? ALso which version of Android are you testing with?
Android 4.0 and 4.1

Can't test with Nightly.
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.
Attached patch fix3 (obsolete) — Splinter Review
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
Attachment #790916 - Flags: review?(bugmail.mozilla)
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.
Attachment #790916 - Flags: review?(bugmail.mozilla) → review-
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
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.
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?
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #790916 - Attachment is obsolete: true
Attachment #795647 - Flags: review?(bugmail.mozilla)
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.
Attachment #795647 - Flags: review?(bugmail.mozilla) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #795647 - Attachment is obsolete: true
Attachment #796295 - Flags: review?(bugmail.mozilla)
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.
Attachment #796295 - Flags: review?(bugmail.mozilla) → review-
.
Attached patch PatchSplinter Review
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.
Attachment #796295 - Attachment is obsolete: true
Attachment #816146 - Flags: review?(bugmail.mozilla)
Flags: needinfo?(wjohnston)
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.
Attachment #816146 - Flags: review?(bugmail.mozilla) → review+
Assignee: nobody → wjohnston
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Windows 7 → Android
Hardware: x86_64 → ARM
https://hg.mozilla.org/mozilla-central/rev/e6406d223b50
https://hg.mozilla.org/mozilla-central/rev/863087fdc7da
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
You need to log in before you can comment on or make changes to this bug.