Draw to native window, if available

RESOLVED FIXED in Firefox 9

Status

Fennec Graveyard
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: snorp, Assigned: snorp)

Tracking

({mobile, perf})

Trunk
Firefox 9
ARM
Android
mobile, perf
Dependency tree / graph

Details

(Whiteboard: QA?)

Attachments

(1 attachment, 5 obsolete attachments)

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

6 years ago
This is awesome. I'm up for review when this is ready!
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
Attachment #560482 - Attachment is obsolete: true
Attachment #560516 - Flags: review?(chrislord.net)

Comment 4

6 years ago
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).
Attachment #560516 - Flags: review?(chrislord.net) → review+

Comment 5

6 years ago
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.
Attachment #560516 - Flags: review+ → review-
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
Attachment #560516 - Attachment is obsolete: true
Attachment #560602 - Flags: review?(chrislord.net)
Keywords: mobile, perf

Comment 7

6 years ago
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.
Attachment #560602 - Flags: review?(chrislord.net) → review+

Comment 8

6 years ago
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.
Attachment #560602 - Flags: review+ → review-
Attachment #560602 - Attachment is obsolete: true
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
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
Attachment #560617 - Attachment is obsolete: true
Attachment #560640 - Flags: review?(chrislord.net)

Updated

6 years ago
Assignee: nobody → snorp
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
Attachment #560640 - Attachment is obsolete: true
Attachment #560640 - Flags: review?(chrislord.net)
Attachment #561548 - Flags: review?(blassey.bugs)
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.
Attachment #561548 - Flags: review?(blassey.bugs) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ec8974f0917
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0ec8974f0917
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
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.
Backed out: https://hg.mozilla.org/mozilla-central/rev/f08484868187
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 9 → ---
(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? :)
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).
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!
Re-landed: https://hg.mozilla.org/mozilla-central/rev/48df3b328875
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
Depends on: 689948
Depends on: 690260
tracking-fennec: --- → 9+
Blocks: 684860
status-firefox9: --- → fixed

Updated

6 years ago
Whiteboard: QA?
You need to log in before you can comment on or make changes to this bug.