686992 - Draw to native window, if available

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

As of 2.3, Android exposes the native window bits to NDK applications.  We can avoid a copy if we draw to that directly instead of a bitmap.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 686992 - Draw to Android window/surface directly

This avoids usage of an intermediate bitmap on 2.3+ devices

Comment 2

[Chris Lord [:cwiiis]]

This is awesome. I'm up for review when this is ready!

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 686992 - Draw to Android window/surface directly

This avoids usage of an intermediate bitmap on 2.3+ devices

Comment 4

[Chris Lord [:cwiiis]]

Comment on attachment 560516 [details] [diff] [review]
Bug 686992 - Draw to Android window/surface directly

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

I'm just going to fix up those couple of niggles and push to inbound (after it builds). Great stuff!

::: widget/src/android/AndroidBridge.cpp
@@ +951,5 @@
> +            ANativeWindow_fromSurface = (ANativeWindow* (*)(JNIEnv *, jobject))dlsym(handle, "ANativeWindow_fromSurface");
> +            ANativeWindow_release = (void (*)(ANativeWindow *))dlsym(handle, "ANativeWindow_release");
> +            ANativeWindow_setBuffersGeometry = (int (*)(ANativeWindow*, int, int, int)) dlsym(handle, "ANativeWindow_setBuffersGeometry");
> +            ANativeWindow_lock = (int (*)(ANativeWindow*, ANativeWindow_Buffer*, ARect*)) dlsym(handle, "ANativeWindow_lock");
> +            ANativeWindow_unlockAndPost = (int (*)(ANativeWindow* window))dlsym(handle, "ANativeWindow_unlockAndPost");

Should probably add a similar ALOG_BRIDGE as the one above.

::: widget/src/android/nsWindow.cpp
@@ +853,5 @@
> +            if (AndroidBridge::Bridge()->HasNativeWindowAccess()) {
> +                AndroidGeckoSurfaceView& sview(AndroidBridge::Bridge()->SurfaceView());
> +                jobject surface = sview.GetSurface();
> +                if (surface) {
> +                    sNativeWindow = AndroidBridge::Bridge()->AcquireNativeWindow(surface);

This can return NULL and the next call doesn't check it's arguments, so we should either check here or add a check to SetNativeWindowFormat (or both).

Comment 5

[Chris Lord [:cwiiis]]

Comment on attachment 560516 [details] [diff] [review]
Bug 686992 - Draw to Android window/surface directly

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

Ah, didn't notice about the extra header. We need to copy the ANativeWindow struct from the header into ours - we do the same for BitmapInfo when using libjnigraphics.so. Once that and the two above are done, it'll be an r+ from me.

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 686992 - Draw to Android window/surface directly

This avoids usage of an intermediate bitmap on 2.3+ devices

Comment 7

[Chris Lord [:cwiiis]]

Comment on attachment 560602 [details] [diff] [review]
Bug 686992 - Draw to Android window/surface directly

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

Looks good to me - will build and check in.

Comment 8

[Chris Lord [:cwiiis]]

Comment on attachment 560602 [details] [diff] [review]
Bug 686992 - Draw to Android window/surface directly

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

Sorry to flip-flop on this, but after building and running on an HTC Flyer, I see some issues:

- The stride seems incorrect in portrait orientation
- It often crashes when the surface size changes
- When the above doesn't crash, there's a blank white frame between surface-size changes (we have a similar issue to this on GL)

I think we'll need to fix at least the first two, and perhaps the third, before checking this in.

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 686992 - Draw to Android window/surface directly

This avoids usage of an intermediate bitmap on 2.3+ devices

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 686992 - Draw to Android window/surface directly

This avoids usage of an intermediate bitmap on 2.3+ devices

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 686992 - Draw to Android window/surface directly

This avoids usage of an intermediate bitmap on 2.3+ devices

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

The current patch appears to work fine through all of my testing.  The refactoring in GeckoSurfaceView::surfaceChanged isn't strictly necessary, but it helped to make things easier to follow (IMHO).  The main thing that needed changed there is the drawRGB() call which caused a white flash when the surface size was changed (orientation change, keyboard visibility change, etc).  Also, locking the canvas there would often fail since we are now locking it in Gecko too.

Using a simple hacked up scrolling benchmark, I got the following numbers to draw one frame:

pure java: ~20ms
native bitmap: 15-17ms
native window (this patch): 11-13ms

Given that we need to stay under 16ms or so for 60fps, I'd say this is a pretty nice improvement.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ec8974f0917

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0ec8974f0917

Comment 15

[Chris Lord [:cwiiis]]

I don't think this is ready yet. Now it's in, I get crashes, what look like stride errors and even whole-device reboots on my HTC Flyer. I think this ought to be backed out, pending further testing.

Comment 16

[Matt Brubeck (:mbrubeck)]

Backed out: https://hg.mozilla.org/mozilla-central/rev/f08484868187

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Chris Lord [:cwiiis] from comment #15)
> I don't think this is ready yet. Now it's in, I get crashes, what look like
> stride errors and even whole-device reboots on my HTC Flyer. I think this
> ought to be backed out, pending further testing.

Ugh.  I tested on all devices I have here and didn't have any issues (honeycomb & gingerbread).  Did it happen during orientation change, or what?  Starting to wonder if that thing is just buggy....feel like debugging it when you are back? :)

Comment 18

[Chris Lord [:cwiiis]]

I'll get on the case as soon I'm back from holiday, we definitely want this in. I just see the problems when scrolling around, hopefully it isn't device specific... The results when run on an original Galaxy Tab upgraded to Gingerbread might be interesting (similar device profile).

Comment 19

[Chris Lord [:cwiiis]]

This is absolutely my bad, the layers acceleration config was enabled and it was actually GL layers that was causing my issue. Please re-apply, this patch is fine!

Comment 20

[Matt Brubeck (:mbrubeck)]

Re-landed: https://hg.mozilla.org/mozilla-central/rev/48df3b328875