AOMediaCodec / libavif

libavif - Library for encoding and decoding .avif files
Other
1.58k stars 206 forks source link

Android JNI - Incorrect Premultiplied Default? #1362

Closed joedrago closed 1 year ago

joedrago commented 1 year ago

Hello @vigneshvg --

I was doing some internal testing with a teammate and we noticed that decoded Bitmap objects coming out of AvifDecoder.decode() are flagged as premultiplied (as all Bitmaps must be in order to be drawn by Android draw calls), but in the guts of the JNI decode() function, we are just using the default value of avifRGBImage.alphaPremultiplied, which is AVIF_FALSE. This results in a Bitmap which contains the actual unpremultiplied pixels in them, yet the Bitmap itself is flagged as premultiplied. This causes incorrect blending.

If I'm right (please correct me if not), there are two solutions to this:

... after the call to avifRGBImageSetDefaults().

Thoughts?

vigneshvg commented 1 year ago

Hello @vigneshvg --

I was doing some internal testing with a teammate and we noticed that decoded Bitmap objects coming out of AvifDecoder.decode() are flagged as premultiplied

Can you elaborate a bit more on what exactly you mean by "Bitmaps are flagged as premultiplied"? Because the bitmap is an input parameter that's taken by the AvifDecoder.decode() function. The JNI wrapper itself does not do anything more to the bitmap other than to copy pixels. Depending on the configuration of the bitmap, the alpha plane should be handled correctly. For ARGB_8888 and RGBA_F16, the alpha plane is not premultiplied.

I have tried loading images with alpha plane several times before and it has worked as intended. Do you know which type of bitmaps you were testing? If you can share a sample image that fails to render properly, i can take a further look. :)

(as all Bitmaps must be in order to be drawn by Android draw calls), but in the guts of the JNI decode() function, we are just using the default value of avifRGBImage.alphaPremultiplied, which is AVIF_FALSE. This results in a Bitmap which contains the actual unpremultiplied pixels in them, yet the Bitmap itself is flagged as premultiplied. This causes incorrect blending.

If I'm right (please correct me if not), there are two solutions to this:

  • Legislate that all Bitmap data coming out of the Java AvifDecoder is actually premultiplied. Android wants this anyway, so it'd be as simple as adding: rgb_image.alphaPremultiplied = AVIF_TRUE;

... after the call to avifRGBImageSetDefaults().

I don't think this assumption is true, because in case of ARGB_8888 and RGBA_F16 bitmaps, the alpha channel is not pre-multiplied.

  • Expose the alphaPremultiplied property in some way to Java, so consumers of this library can set it to true. I don't see a ton of value here of it ever being false, but if we're going for completeness and someone simply wants to query pixel data after decode or do post-processing before handing it over to Android to draw, that would also be fine.

This is certainly possible (and quite easy to do). I have an updated API coming up for the JNI (changes in my local branch here: https://github.com/vigneshvg/libavif/tree/cl_animated_tree). That provides better access into the decoder (proper return codes, animated images, etc). We can add a function there to expose the premultiplied value if that will help.

But in general, Android apps using the JNI API via the Bitmap class do not care about that level of detail and only want the right pixels in the Bitmap so that it can be rendered. If they want access to the raw pixels (for post-processing or whatever), then the JNI API as it stands is not a good tool for that use-case. For that, we would have to expose other functions which return the raw pixel values rather than copying them into an android.graphics.Bitmap.

Thoughts?

joedrago commented 1 year ago

Can you elaborate a bit more on what exactly you mean by "Bitmaps are flagged as premultiplied"? Because the bitmap is an input parameter that's taken by the AvifDecoder.decode() function. The JNI wrapper itself does not do anything more to the bitmap other than to copy pixels. Depending on the configuration of the bitmap, the alpha plane should be handled correctly. For ARGB_8888 and RGBA_F16, the alpha plane is not premultiplied.

Sure, although this is quite secondhand for me (I am a C/C++ programmer, I very rarely dabble in Java). Here's my semi-limited understanding of the situation:

joedrago commented 1 year ago

g_magenta

Here is a very simple magenta AVIF, which offers an alpha gradient from 0-255 from left to right. If you hand this to an ImageView with a background color set (say blue), it will either blend incorrectly if Bitmap.isPremultiplied is true, or it will refuse to draw if it is set to false.

vigneshvg commented 1 year ago

g_magenta

Here is a very simple magenta AVIF, which offers an alpha gradient from 0-255 from left to right. If you hand this to an ImageView with a background color set (say blue), it will either blend incorrectly if Bitmap.isPremultiplied is true, or it will refuse to draw if it is set to false.

Thank you for the detailed explanation and the sample image. Let me take a look at this further and get back to you.

joedrago commented 1 year ago

g_magenta.zip

I just realized GitHub did not do what I expected with that image, so I've zipped the real test AVIF and attached it here.

As an aside:

But in general, Android apps using the JNI API via the Bitmap class do not care about that level of detail and only want the right pixels in the Bitmap so that it can be rendered.

I completely agree with this, and if it wasn't for the weird blending we discovered, this wouldn't have come up at all. I'm all for simple on this one, as long as it draws pretty.

Edit: Simply honoring the isPremultiplied value in the incoming Bitmap might be plenty, really. We can set avifRGBImage.alphaPremultiplied to that property's value, and everything should behave in all directions. No new APIs would be required.

vigneshvg commented 1 year ago

Edit: Simply honoring the isPremultiplied value in the incoming Bitmap might be plenty, really. We can set avifRGBImage.alphaPremultiplied to that property's value, and everything should behave in all directions. No new APIs would be required.

Yes, this is probably the way to go.

But i could've sworn i've seen images with alpha render with proper blending on Android before. So i just wanted to double check with your example that it is being blended incorrectly before making any changes.

Garaks commented 1 year ago

@vigneshvg

Thanks for the quick turnaround. When can this fix be released so that I can pull the fix for our builds?

https://mvnrepository.com/artifact/org.aomedia.avif.android/avif

vigneshvg commented 1 year ago

@vigneshvg

Thanks for the quick turnaround. When can this fix be released so that I can pull the fix for our builds?

I am waiting on one more PR [1] to be merged before i can make a maven release: https://github.com/AOMediaCodec/libavif/pull/1364.

I will wait until early next week to see if that's merged by then. Otherwise, i will go ahead and make a release for this fix. Would that work for you?

https://mvnrepository.com/artifact/org.aomedia.avif.android/avif

[1] https://github.com/AOMediaCodec/libavif/pull/1364

Garaks commented 1 year ago

Ok. I can wait for early next week.

vigneshvg commented 1 year ago

Ok. I can wait for early next week.

I pushed out a version to maven today and it contains the fix.

Gradle dependency: implementation 'org.aomedia.avif.android:avif:0.11.1.3c786d2'

I am closing this issue now. Please feel free to re-open (or make new ones) if you find any other issues. Thanks!