Last Comment Bug 661843 - GeckoSurfaceView may double memory requirement for painting
: GeckoSurfaceView may double memory requirement for painting
Status: RESOLVED FIXED
: footprint, mobile
Product: Core
Classification: Components
Component: Widget: Android (show other bugs)
: unspecified
: All Android
: -- normal (vote)
: mozilla6
Assigned To: Chris Lord [:cwiiis]
:
:
Mentors:
Depends on: 664996
Blocks: 663464
  Show dependency treegraph
 
Reported: 2011-06-03 08:58 PDT by Doug Turner (:dougt)
Modified: 2011-07-28 15:13 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
Remove extra alloc/memcpy when drawing (9.70 KB, patch)
2011-06-07 04:01 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Splinter Review
Optionally remove extra alloc/memcpy when drawing (18.72 KB, patch)
2011-06-08 10:18 PDT, Chris Lord [:cwiiis]
doug.turner: review+
Details | Diff | Splinter Review
Optionally remove extra alloc/memcpy when drawing (revised) (19.24 KB, patch)
2011-06-16 02:03 PDT, Chris Lord [:cwiiis]
doug.turner: review+
mark.finkle: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Doug Turner (:dougt) 2011-06-03 08:58:40 PDT
Inspecting the way we draw on Android, I noticed that we have both a ByteBuffer and a BitMap in the GeckoSurfaceView.  It looks like we use the memory in the ByteBuffer as the image surface that Gecko draws into.  The BitMap is used only so that we can draw into the android Canvas:

http://mxr.mozilla.org/mozilla-central/source/embedding/android/GeckoSurfaceView.java#333

My concern is that the we have an extra memcpy per draw and double the memory used.

I think we should double check this observation.  I hope I am wrong.

If I am not, maybe it is possible to (1) copy a ByteBuffer directly into a Canvas, or (2) make a new BitMap implementation that allows buffer sharing.
Comment 1 Chris Lord [:cwiiis] 2011-06-06 02:29:14 PDT
I've not gotten as far as setting up a debugger to trace this call yet, but reading the Android reference docs, I suspect that this is indeed causing an extra allocation and copy per call that may be unnecessary.

Also based on this initial look, it may be possible to use the Canvas drawBitmap call with an integer array, and use the ByteBuffer directly with asIntBuffer, assuming that the pixel format is the same and width==stride, which I assume is the case seeing as the copyPixelsFromBuffer code doesn't seem to have any context that would tell it otherwise.

I'll give this a go and report back with either problems or a patch.
Comment 2 Chris Lord [:cwiiis] 2011-06-06 03:12:01 PDT
ok, not quite as easy to fix as I'd hoped. Kind of obviously, I suppose, you can't access the array after converting a ByteBuffer to an IntBuffer, so you can't get an int[] from the original byte[].

Still looking to see if (1) is possible, otherwise will start looking at (2).
Comment 3 Chris Lord [:cwiiis] 2011-06-06 04:04:09 PDT
Just to confirm the original suspicion, creating a new Bitmap in this way does indeed involve an extra memcpy from the ByteBuffer to another allocated piece of memory.

Confirmed by reading the source, here:

http://android.git.kernel.org/?p=platform/frameworks/base.git;a=blob;f=graphics/java/android/graphics/Bitmap.java;h=dd83c3bfafba4e61687ce55ed21eb3522b874850;hb=HEAD#l276

and here:

http://android.git.kernel.org/?p=platform/frameworks/base.git;a=blob;f=core/jni/android/graphics/Bitmap.cpp;h=083e3b126ca96f601dba76fe5dabdc93e20e5ecf;hb=HEAD#l526
Comment 4 Chris Lord [:cwiiis] 2011-06-06 04:23:08 PDT
This discussion of the matter doesn't look hopeful...

http://groups.google.com/group/android-developers/browse_thread/thread/b3de51ec56c0f3fc?pli=1

Perhaps we could provide a method to get back an int array for drawing onto the canvas?
Comment 5 Chris Lord [:cwiiis] 2011-06-06 06:46:34 PDT
I'm currently at a loss as to the correct way of fixing this. I'm hoping that I've just missed something very obvious, but it seems that there's no way of creating a bitmap from arbitrary buffers without copying the data, and you can't extend Bitmap as it's final.

Hopefully more delving will expose something useful. Any hints greatly appreciated!
Comment 6 Chris Lord [:cwiiis] 2011-06-06 07:45:10 PDT
ok, I've gone back and forth on this a bit now and I'm reasonably confident in saying that without Android NDK from 2.2+, what we want to do isn't (officially) possible.

The only way to blit to a Canvas without copying is via a Bitmap, and there are no methods of creating bitmaps from existing data, and no methods for accessing the backing data store for a Bitmap. I've given the API docs, the source-code and Google a thorough going over trying to find a way around this, but nothing is coming up (beyond using unadvertised, unstable API).

What we're doing now is probably the most efficient way of going about it without using Android NDK r4b (where you then have native access to the pixel buffer used behind the Bitmap and we could just use that in OnDraw in nsWindow).
Comment 7 Chris Lord [:cwiiis] 2011-06-07 04:01:21 PDT
Created attachment 537766 [details] [diff] [review]
Remove extra alloc/memcpy when drawing

This patch uses NDK r4b (API level 8, i.e. Android 2.2+) API to draw directly to a Bitmap, and thus avoid the extra allocate and memory copy of the current code.
Comment 8 Doug Turner (:dougt) 2011-06-07 12:35:37 PDT
chris, any idea on the time savings?
Comment 9 Doug Turner (:dougt) 2011-06-07 13:13:40 PDT
also, is there a way to do a feature test without too much code ugliness

For example, if we are on 2.1, do it the old way.  if we are >= 2.2 do it the new way?
Comment 10 Chris Lord [:cwiiis] 2011-06-08 01:39:34 PDT
I'll have a look at some performance tests in a moment and report back.

We could dlopen libjnigraphics.so and use the success of the dlopen/presence of the symbols as the test.

We can also check the build version - http://developer.android.com/reference/android/os/Build.VERSION.html, though we'll need to error-check the result of the dlopen anyway, so I'd have thought providing two draw/get commands in GeckoSurfaceView (get/draw Buffer/Bitmap) and using them depending on the result of the dlopen would make sense.

This would also have the advantage of using the fast-path on devices that have an incorrect version number/have added libjnigraphics independently (if any exist).

A revised patch will be incoming.
Comment 11 Chris Lord [:cwiiis] 2011-06-08 10:18:04 PDT
Created attachment 538048 [details] [diff] [review]
Optionally remove extra alloc/memcpy when drawing

This patch uses dlopen/dlsym to get the function pointers for the 2.2 NDK Bitmap functions. If they aren't found, it reverts to the old method of using a ByteBuffer and an intermediate Bitmap to copy it to the Canvas.

I've also changed the draw commands to synchronise on the draw bitmap/buffer - previously, it would wait until the canvas was locked, then check if it changed and paint the canvas white (this rarely(/never?) happened) due to the canvas being painted black when it's locked.

As painting the buffers implies a lock on the buffers anyway, I've put a synchronise block on the drawing buffers and then check that they haven't changed before locking the canvas as well - that avoids the above case, I think, though there may be reasons it's a bad idea (and I'd like to hear them if so).

I ended up getting caught rewriting this patch rather than performance-testing, but initial testing shows no significant performance difference without the intermediate copy on my Xoom (perhaps this will be different on slower/different devices).

I'll find/write a more specific test-case and report back tomorrow with numbers.
Comment 12 Doug Turner (:dougt) 2011-06-15 12:34:27 PDT
Comment on attachment 538048 [details] [diff] [review]
Optionally remove extra alloc/memcpy when drawing

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

Mostly nits.  Looks good.  Could you fix the nits and put up another patch?  Lets get blassey to review the second patch.

::: embedding/android/GeckoSurfaceView.java
@@ +132,5 @@
>       * Called on main thread
>       */
>  
> +    public void draw(SurfaceHolder holder, ByteBuffer buffer) {
> +        if (buffer == null || buffer.capacity() != (mWidth * mHeight * 2))

why 2.  could you comment this being RGB565.

@@ +149,5 @@
> +        }
> +    }
> +
> +    public void draw(SurfaceHolder holder, Bitmap bitmap) {
> +        if (bitmap == null || bitmap.getWidth() != mWidth || bitmap.getHeight() != mHeight)

You could wrap this line.

::: widget/src/android/AndroidBridge.cpp
@@ +872,5 @@
> +        // Android 2.2+ (API level 8)
> +        mOpenedBitmapLibrary = true;
> +
> +        void *handle = dlopen("/system/lib/libjnigraphics.so", RTLD_LAZY | RTLD_LOCAL);
> +        if (handle == NULL)

NULL - > nsnull

And in other places in this patch

@@ +875,5 @@
> +        void *handle = dlopen("/system/lib/libjnigraphics.so", RTLD_LAZY | RTLD_LOCAL);
> +        if (handle == NULL)
> +            return false;
> +
> +        *reinterpret_cast<void **>(&AndroidBitmap_getInfo) = dlsym(handle, "AndroidBitmap_getInfo");

pretty sure that this is compiler dependent -- something about the standard doesn't allow casting to void pointers, iirc.  Does a static cast work - or a c style cast?

@@ +891,5 @@
> +        ALOG_BRIDGE("Successfully opened libjnigraphics.so");
> +        mHasNativeBitmapAccess = true;
> +    }
> +
> +    return mHasNativeBitmapAccess;

return true;

@@ +903,5 @@
> +        uint32_t height;
> +        uint32_t stride;
> +        uint32_t format;
> +        uint32_t flags;
> +    };

Comment that this structure is defined by the header associated with the library we are opening up.

@@ +914,5 @@
> +        return false;
> +    }
> +
> +    if (info.width != width || info.height != height)
> +        return false;

if we fail because the width and height don't match what we think they should, is ValidateBitmap really the right name?  Maybe this method just needs comments.

@@ +927,5 @@
> +    void *buf;
> +
> +    if ((err = AndroidBitmap_lockPixels(JNI(), bitmap, &buf)) != 0) {
> +        ALOG_BRIDGE("AndroidBitmap_lockPixels failed! (error %d)", err);
> +        buf = NULL;

like above, NULL -> nsnull

::: widget/src/android/AndroidBridge.h
@@ +245,4 @@
>  
>      void ScanMedia(const nsAString& aFile, const nsACString& aMimeType);
>  
> +    bool HasNativeBitmapAccess();

comment above that these next 4 methods are for BitMap access.
Comment 13 Chris Lord [:cwiiis] 2011-06-16 02:03:06 PDT
Created attachment 539749 [details] [diff] [review]
Optionally remove extra alloc/memcpy when drawing (revised)

This is a revised patch that addresses the comment above. A couple of notes;

I added the comment about with * height * 2 relating to 565 in getSoftwareDrawBuffer(), as it seemed a more appropriate place (it's where the buffer is allocated, so if you were wondering, you'd probably track it to that point).

AndroidBridge::HasNativeBitmapAccess() can't return true at the point noted, as mHasNativeBitmapAccess may be false (this would be the case if dlopen/dlsym failed in one call and the function is called again later).

I replaced the casts with C-style casts casting the void *'s to the function prototypes. I wanted to avoid having typedefs for the functions, so I've just put the function prototype in the cast again.

I've replaced all the NULLs with nsnull - though elsewhere in the file there is NULL - do we have a policy of NULL/nsnull usage? (I used NULL in HasNativeBitmapAccess as it was using C functions, though the other instance ought to be nsnull)

I think ValidateBitmap is a reasonable name - the parameters make it obvious what's going to happen I would say, and there is the situation that getInfo returns NULL because the Bitmap is invalid for other reasons (bad object pointer, perhaps)
Comment 14 Doug Turner (:dougt) 2011-06-16 11:53:44 PDT
http://hg.mozilla.org/mozilla-central/rev/8c5e954a80b8
Comment 15 Mark Finkle (:mfinkle) (use needinfo?) 2011-06-16 12:24:36 PDT
Comment on attachment 539749 [details] [diff] [review]
Optionally remove extra alloc/memcpy when drawing (revised)

Android only fix. Let it bake on nightly for a few days before pushing to aurora

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