Buffer for use with InputStream

[koral--]

Creating one reusable Java's byte array per GifDrawable instance will be more efficient than allocating new array in each streamReadFun() call.

[Wavesonics]

Yes! I was actually doing some profiling on that and the renderFrame() call is some what slow for what I'd expect from decoding a GIF frame.

Finding a good leak safe / thread safe way of not re-allocing every time with this line from streamReadFun():
jbyteArray buffer = (*env)->NewByteArray(env, size);

Could be a pretty good win.

Also, it might be worth looking into this line:
jbyte* data = (*env)->GetByteArrayElements(env, buffer, NULL);

GetByteArrayElements can do one of two things:

1. Copy the array to a temp scratch space which will be copied back to the Java array when ReleaseByteArrayElements() is called
2. Simply pin the Java mem in place so the GC doesn't mess with it.

I'm not actually sure we can control the behavior, but we CAN pass in a jbool pointer into GetByteArrayElements and see which it's doing. That'd be a nasty perf hit if it was doing the copy every time.

[Wavesonics]

Maybe cache the buffer in the userdata on GifFileType?

[Wavesonics]

So i tried 2 things:

1. I verified that our call to GetByteArrayElements() is pinning the memory, not copying it, so that's good.

2. I cached the stream read buffer on StreamController (resizing it in StreamReadFunc as needed). This did indeed improve performance on renderFrame(), but only marginally. Check it out here: 02745ad

Let me know what you think. If we can't come up with something better, I'd say it's still worth taking as it prevents a little GC thrashing.

It's probably worth further investigating perf hits in renderFrame() as it still doesn't seem as performant as I'd expect.

I think we could further optimize by providing a code path for passing a Java byte array instead of an InputStream. In the read func this would allow us to simply memcpy the range we are interested in directly from the jByteArray to the native mem. Rather than copying from the Stream to the buffer, and then from the buffer to the native mem.

We'd need to emulate the file cursor/reset functionality on our own though.

[Wavesonics]

#14 included this optimization.

[koral--]

I think it is good idea to make new GifDrawable constructor which takes byte[] but in addition, not instead of existing one taking InputStream.

[Wavesonics]

Oh sorry I wasn't clear, yes that was my intention.

[koral--]

I think there is no need to use memcpy since GetByteArrayRegion can copy bytes directly from java byte array to native memory. I've added direct byte[] support in f1ca293.
Maybe we can use GetByteArrayRegion instead of GetByteArrayElements in streamReadFun?

[Wavesonics]

Oh yeah I actually looked into GetByteArrayRegion when I was working on
that code. Can't remember why I didn't end up using it. I think I remember
thinking it would work for a raw byte array, but not for the input stream?

Either way it would certainly be better where ever we can use it.
On Nov 23, 2013 6:38 PM, "koral--" notifications@github.com wrote:

I think there is no need to use memcpy since GetByteArrayRegion can copy
bytes directly from java byte array to native memory. I've added direct
byte[] support in f1ca293f1ca293
.
Maybe we can use GetByteArrayRegion instead of GetByteArrayElements in
streamReadFun?

—
Reply to this email directly or view it on GitHubhttps://github.com/koral--/android-gif-drawable/issues/13#issuecomment-29147461
.

[koral--]

OK, I've changed GetByteArrayElements to GetByteArrayRegion.
In d089dbe I've also changed initial size of the buffer to 256 bytes. AFAIK GIFLib reads once at most 256 bytes (max. size of the single GIF block) but initial blocks are small so there is no need to reallocate very small buffers.

[koral--]

Reading from stream was optimized, so I am closing this issue.

[koral--]

Two changes related to this issue was made in d5d3a31:

• byte array length is obtained only once now instead of every read function call
• one required free() call was added