Last Comment Bug 686992 - Draw to native window, if available
: Draw to native window, if available
Status: RESOLVED FIXED
QA?
: mobile, perf
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: Firefox 9
Assigned To: James Willcox (:snorp) (jwillcox@mozilla.com)
:
Mentors:
Depends on: 689948 690260
Blocks: 684860
  Show dependency treegraph
 
Reported: 2011-09-15 16:10 PDT by James Willcox (:snorp) (jwillcox@mozilla.com)
Modified: 2011-10-10 01:14 PDT (History)
10 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Bug 686992 - Draw to Android window/surface directly (11.66 KB, patch)
2011-09-15 16:11 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
no flags Details | Diff | Review
Bug 686992 - Draw to Android window/surface directly (13.37 KB, patch)
2011-09-15 23:54 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
chrislord.net: review-
Details | Diff | Review
Bug 686992 - Draw to Android window/surface directly (14.33 KB, patch)
2011-09-16 12:11 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
chrislord.net: review-
Details | Diff | Review
Bug 686992 - Draw to Android window/surface directly (14.47 KB, patch)
2011-09-16 13:55 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
no flags Details | Diff | Review
Bug 686992 - Draw to Android window/surface directly (14.92 KB, patch)
2011-09-16 15:21 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
no flags Details | Diff | Review
Bug 686992 - Draw to Android window/surface directly (22.94 KB, patch)
2011-09-21 12:46 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
blassey.bugs: review+
Details | Diff | Review

Description James Willcox (:snorp) (jwillcox@mozilla.com) 2011-09-15 16:10:53 PDT
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) (jwillcox@mozilla.com) 2011-09-15 16:11:48 PDT
Created attachment 560482 [details] [diff] [review]
Bug 686992 - Draw to Android window/surface directly

This avoids usage of an intermediate bitmap on 2.3+ devices
Comment 2 Chris Lord [:cwiiis] 2011-09-15 16:23:07 PDT
This is awesome. I'm up for review when this is ready!
Comment 3 James Willcox (:snorp) (jwillcox@mozilla.com) 2011-09-15 23:54:26 PDT
Created attachment 560516 [details] [diff] [review]
Bug 686992 - Draw to Android window/surface directly

This avoids usage of an intermediate bitmap on 2.3+ devices
Comment 4 Chris Lord [:cwiiis] 2011-09-16 10:23:50 PDT
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] 2011-09-16 10:48:44 PDT
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) (jwillcox@mozilla.com) 2011-09-16 12:11:15 PDT
Created attachment 560602 [details] [diff] [review]
Bug 686992 - Draw to Android window/surface directly

This avoids usage of an intermediate bitmap on 2.3+ devices
Comment 7 Chris Lord [:cwiiis] 2011-09-16 12:16:52 PDT
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] 2011-09-16 13:41:51 PDT
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) (jwillcox@mozilla.com) 2011-09-16 13:55:00 PDT
Created attachment 560617 [details] [diff] [review]
Bug 686992 - Draw to Android window/surface directly

This avoids usage of an intermediate bitmap on 2.3+ devices
Comment 10 James Willcox (:snorp) (jwillcox@mozilla.com) 2011-09-16 15:21:39 PDT
Created attachment 560640 [details] [diff] [review]
Bug 686992 - Draw to Android window/surface directly

This avoids usage of an intermediate bitmap on 2.3+ devices
Comment 11 James Willcox (:snorp) (jwillcox@mozilla.com) 2011-09-21 12:46:37 PDT
Created attachment 561548 [details] [diff] [review]
Bug 686992 - Draw to Android window/surface directly

This avoids usage of an intermediate bitmap on 2.3+ devices
Comment 12 James Willcox (:snorp) (jwillcox@mozilla.com) 2011-09-21 12:54:35 PDT
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?) 2011-09-22 19:34:30 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ec8974f0917
Comment 14 Ed Morley [:emorley] 2011-09-23 04:41:11 PDT
https://hg.mozilla.org/mozilla-central/rev/0ec8974f0917
Comment 15 Chris Lord [:cwiiis] 2011-09-24 08:49:16 PDT
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) 2011-09-24 08:55:27 PDT
Backed out: https://hg.mozilla.org/mozilla-central/rev/f08484868187
Comment 17 James Willcox (:snorp) (jwillcox@mozilla.com) 2011-09-24 10:20:56 PDT
(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] 2011-09-24 18:57:23 PDT
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] 2011-09-26 10:13:44 PDT
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) 2011-09-26 10:22:12 PDT
Re-landed: https://hg.mozilla.org/mozilla-central/rev/48df3b328875

Note You need to log in before you can comment on or make changes to this bug.