Apng fails to render with versions newer then 2.23.0

[connyduck]

New Issue Checklist

• I have searched for a similar issue in the project and found none

Issue Info

Info	Value
Device Info	probably all, tried with a Fairphone 4
System Version	probably all, tried with Android 12
APNG4Android Library Version	2.25.0
Repro rate	90%
Repro with our demo project	no, but we have quite a custom setup that loads the images into TextViews as Spans
Demo project link	relevant code is here

Issue Description and Steps

This apng fails to load on version 2.24.0 and 2.25.0. The console is spammed with FrameAnimationDrawable onRender:Buffer not large enough for pixels. When I tell Glide to load the image with smaller dimensions, it renders, but has completely wrong dimensions. Screenshots and additional info can be found in our issue: tuskyapp/Tusky#3631

When I downgrade to 2.23.0, it works just fine.

[jingpeng]

device-2023-07-07-120042.mp4

[jingpeng]

I use latest code to animate this image, it seems to work normally

[nikclayton]

Sorry, this isn't fixed in 2.26.0, I still see this same problem in Tusky.

It's tricky to track down because I can't easily build APNG4Android locally (I'll file a separate issue for that with more details), but I've tracked this down to a difference in the bitmap sizes in APNGFrame.onDraw().

In 2.23.0 (which works), if I debug onDraw() the dimensions of bitmap when loading this APNG are 64 x 64 x 32bpp = 16K. That matches the size of the buffer that FrameAnimationDrawable.onRender() uses, so there is no error.

In 2.24.0 and above (which doesn't work) the dimensions of bitmap are 128 x 128 x 32bpp = 64K. That's four times larger than the size of the FrameAnimationDrawable.onRender() buffer, so it fails.

The native dimensions of the source PNG file are 128 x 128 x 32bpp.

[nikclayton]

@jingpeng

e8f8fd3 is the problematic commit.

If I revert that then Tusky displays the animated PNG correctly.

That commit was to fix #165

Tusky's code to get and display the animated PNG is https://github.com/tuskyapp/Tusky/blob/develop/app/src/main/java/com/keylesspalace/tusky/util/CustomEmojiHelper.kt.

The image is being loaded in to a drawable that will be displayed as a Span. The Span (EmojiSpan in our code) contains code to adjust the size of the image so that it fits within the line height of the span and maintains its aspect ratio.

I'll keep looking...

[jingpeng]

Can you provide me a demo project with the related images?

[nikclayton]

I can, but I think I've found the bug. I think there's a race condition between FrameSeqDecoder.setDesiredSize() returning true (indicating that the sample size has changed), and a subsequent call to FrameSeqDecoder.getSampleSize() returning the new sample size.

This race happens in FrameAnimationDrawable.setBounds() (line numbers added for clarity):

 1    public void setBounds(int left, int top, int right, int bottom) {
 2        super.setBounds(left, top, right, bottom);
 3        boolean sampleSizeChanged = frameSeqDecoder.setDesiredSize(getBounds().width(), getBounds().height());
 4        matrix.setScale(
 5                1.0f * getBounds().width() * frameSeqDecoder.getSampleSize() / frameSeqDecoder.getBounds().width(),
 6                1.0f * getBounds().height() * frameSeqDecoder.getSampleSize() / frameSeqDecoder.getBounds().height());
 7
 8        if (sampleSizeChanged)
 9            this.bitmap = Bitmap.createBitmap(
10                    frameSeqDecoder.getBounds().width() / frameSeqDecoder.getSampleSize(),
11                    frameSeqDecoder.getBounds().height() / frameSeqDecoder.getSampleSize(),
12                    Bitmap.Config.ARGB_8888);
13    }

Line 3 checks to see if the sample size has changed by calling frameSeqDecoder.setDesiredSize()

If it has changed it calls frameSeqDecoder.getSampleSize() on lines 10 and 11 to get the new sample size.

This worked before e8f8fd3 because FrameSeqDecoder.sampleSize was modified before FrameSeqDecoder.setDesiredSize() returns true.

After that commit FameSeqDecoder.setDesiredSize() looks like this:

 1    public boolean setDesiredSize(int width, int height) {  
 2        boolean sampleSizeChanged = false;  
 3        final int sample = getDesiredSample(width, height);  
 4        if (sample != this.sampleSize) {  
 5            sampleSizeChanged = true;  
 6            final boolean tempRunning = isRunning();  
 7            workerHandler.removeCallbacks(renderTask);  
 8            workerHandler.post(new Runnable() {  
 9                @Override  
10                public void run() {  
11                    innerStop();  
12                    try {  
13                        sampleSize = sample;  
14                        initCanvasBounds(read(getReader(mLoader.obtain())));  
15                        if (tempRunning) {  
16                            innerStart();  
17                        }  
18                    } catch (IOException e) {  
19                        e.printStackTrace();  
20                    }  
21                }  
22            });  
23        }  
24        return sampleSizeChanged;  
25    }

sampleSize is changed (line 13) in the Runnable .

There's the race condition -- FrameSeqDecoder.setDesiredSize() can return true, but FrameSeqDecoder.getSampleSize() can return the old sample size, because the runnable that changes it hasn't run yet.

[jingpeng]

I think you should not mannually call setBounds or trigger it at any time, like this issue: #182 . Inappropriate call or invoke on it may cause issue

[nikclayton]

Yes, calling setBounds will cause a problem, because of this race condition.

But the race condition didn't exist in 2.23.0.

And expecting to be able to call setBounds on a drawable seems like a reasonable thing to do. Especially for emoji -- the emoji should match the text height, whatever the user might have set that to.

The problem this was supposed to fix was described in #165.

Unfortunately, Google Translate is not providing enough information that I can understand the original problem, and why changing sampleSize in a Runnable was the necessary fix.

If you could explain the original problem from #165 I might be able to suggest an alternative fix that doesn't have this race condition.

[connyduck]

This is still a problem for us @jingpeng @penfeizhou