Bug 168425 – REGRESSION(r205841): [GTK] Test fast/images/animated-png.html is failing since r205841

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

The colors of the PNGs are not correct. It started to fail in r205841

[Miguel Gomez]

The test is running several animations and comparing the final frame of each animation to a reference one. This is failing because the png animations are not running at all, and what it's comparing is the first frame of the animation instead of the last one.

I've debugged this and what happens is that BitmapImage::shouldAnimate() is false because PNGImageDecoder::repetitionCount() is returning 0, which means that it's not an animation.
The curious thing is that PNGImageDecoder reads 1 as the repetition value from the file, but it returns that value minus 1, making it 0. It has been that way since that code was added but I don't know why. Just removing that minus 1 and returning the gotten repetition value makes the animations work again.

This fixes the result in 9 of the 10 images used in the test. The failing one has some small differences in some pixels, probably due to small variations in the decoding result.

[Miguel Gomez]

> I've debugged this and what happens is that BitmapImage::shouldAnimate() is
> false because PNGImageDecoder::repetitionCount() is returning 0, which means
> that it's not an animation.
> The curious thing is that PNGImageDecoder reads 1 as the repetition value
> from the file, but it returns that value minus 1, making it 0. It has been
> that way since that code was added but I don't know why. Just removing that
> minus 1 and returning the gotten repetition value makes the animations work
> again.

Ah, in the APNG format, 0 for the repetition count means repeat indefinitely, but in the webkit code we use -1 for that. That's probably the explanation for the minus 1... but that's not valid as the code is now.

[Miguel Gomez]

Once I found the fix for the animation not running, I've been debugging why one of the images of the test fails. The problem seems to be how alpha premultiplied components are calculated.

Before r205841, premultiplied alpha components were calculated in PNGImageDecoder::setPixelPremultipliedRGBA as

fastDivideBy255(colorComponent * alpha);

But after r205841 this was moved to Color::premultipliedChannel, where it's calculated as

fastDivideBy255(colorComponent * alpha + 254);

which is not producing the same result as before the changes. This seems to be causing problems with animations that blend frames into the previous ones.

I'm not sure why 254 is added for the calculation. I've always seen it as just colorComponent * alpha, so I may be missing something here. Said, what do you think?

[Said Abou-Hallawa]

Yes, you are right regarding the PNGImageDecoder::repetitionCount(). This function should return RepetitionCountInfinite if the loopCunt is zero. Otherwise it should return the loopCount.

Regarding calculating the premultiplied components, if you look at https://bugs.webkit.org/attachment.cgi?id=288626&action=review you can see the old implementation of the following functions are using different methods for dividing by 255 and they handle the decimal part of the division differently.

premultipliedARGBFromColor()
ImageDecoder::setRGBA()
ImageDecoder::overRGBA()

premultipliedARGBFromColor() was getting the ceiling of the division and this is why there was 254 in the calculations. But ImageDecoder::setRGBA() and ImageDecoder::overRGBA() were just getting the floor of the division.

static_cast<int>((x + 254) / 255.0) will give the ceiling but static_cast<int>(x / 255.0) will get you the floor.

I was just trying to unify the code and remove the duplicates. But it is up to you, you can either change the expected results to match the new calculation. Or you can add an argument to premultipliedChannel(..., bool ceiling = true) and pass it all the way from ImageDecoder to this function.

[Miguel Gomez]

Created attachment 305954 [details]
Patch

[Miguel Gomez]

I'm splitting this into 2 bugs. This one will handle the problem with the premultiply change and 170499 will handle the repetition count issue.

[Miguel Gomez]

Created attachment 306377 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 306377 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=306377&action=review

> Source/WebCore/platform/graphics/Color.cpp:57
> +    if (ceiling)
> +        return fastDivideBy255(c * a + 254);
> +
> +    return fastDivideBy255(c * a);

Nit: This can still be written in one line:

return fastDivideBy255(ceiling ? c * a + 254 : c * a);

> Source/WebCore/platform/graphics/ImageBackingStore.h:131
> -    void setPixel(RGBA32* dest, unsigned r, unsigned g, unsigned b, unsigned a)
> +    void setPixel(RGBA32* dest, unsigned r, unsigned g, unsigned b, unsigned a, bool ceilingIfPremultiplied = true)

Since the ImageBackingStore is used only the non-CG image decoder, I think we can make all the callers of makePremultipliedRGBA() in this file pass false for the ceiling argument. An there is no need to add the argument ceilingIfPremultiplied for any function in this file.

> Source/WebCore/platform/image-decoders/png/PNGImageDecoder.cpp:527
> -            buffer.backingStore()->setPixel(address, pixel[0], pixel[1], pixel[2], alpha);
> +            buffer.backingStore()->setPixel(address, pixel[0], pixel[1], pixel[2], alpha, false);

If ceiling will be false always for the non-CG decoders, then there is no need for all the changes in this file.

[Miguel Gomez]

> Since the ImageBackingStore is used only the non-CG image decoder, I think
> we can make all the callers of makePremultipliedRGBA() in this file pass
> false for the ceiling argument. An there is no need to add the argument
> ceilingIfPremultiplied for any function in this file.

Ah, that's good to know. Indeed it makes the patch simpler. I'll fix it and resubmit. Thanks!!

[Miguel Gomez]

Created attachment 306498 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 306498 [details]
Patch

Clearing flags on attachment: 306498

Committed r215172: <http://trac.webkit.org/changeset/215172>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.