Closed Bug 686992 Opened 8 years ago Closed 8 years ago

Draw to native window, if available

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set

Tracking

(firefox9 fixed, fennec9+)

RESOLVED FIXED
Firefox 9
Tracking Status
firefox9 --- fixed
fennec 9+ ---

People

(Reporter: snorp, Assigned: snorp)

References

Details

(Keywords: mobile, perf, Whiteboard: QA?)

Attachments

(1 file, 5 obsolete files)

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.
This avoids usage of an intermediate bitmap on 2.3+ devices
This is awesome. I'm up for review when this is ready!
This avoids usage of an intermediate bitmap on 2.3+ devices
Attachment #560482 - Attachment is obsolete: true
Attachment #560516 - Flags: review?(chrislord.net)
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 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-
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 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 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
This avoids usage of an intermediate bitmap on 2.3+ devices
This avoids usage of an intermediate bitmap on 2.3+ devices
Attachment #560617 - Attachment is obsolete: true
Attachment #560640 - Flags: review?(chrislord.net)
Assignee: nobody → snorp
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+
https://hg.mozilla.org/mozilla-central/rev/0ec8974f0917
Status: NEW → RESOLVED
Closed: 8 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
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 9
Depends on: 689948
Depends on: 690260
tracking-fennec: --- → 9+
Blocks: 684860
Whiteboard: QA?
You need to log in before you can comment on or make changes to this bug.