661843 - GeckoSurfaceView may double memory requirement for painting

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Doug Turner (:dougt)]

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

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

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

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

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

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

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

Attachment: 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)]

chris, any idea on the time savings?

Comment 9

[Doug Turner (:dougt)]

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

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

Attachment: 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)]

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

Attachment: 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)]

http://hg.mozilla.org/mozilla-central/rev/8c5e954a80b8

Comment 15

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

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

Comment 16

[Doug Turner (:dougt)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/9014e8ba5932