Bug 170333 – REGRESSION(r213764): Async decoding of animated images is disabled for ImageDocument

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Miguel Gomez]

The problem can be reproduced with http://apng.onevcat.com/assets/elephant.png

Seems that after a couple of cycles the frames are not rendered in the proper time or with the proper order.

[Carlos Alberto Lopez Perez]

This is another regression of r213764 <https://trac.webkit.org/r213764> (async image decoding).

I double-checked it:

 * Built WebKitGTK+ on r213763 and the animation plays fine.
 * Built WebKitGTK+ on r213764 and the image don't plays fine at all.

[Said Abou-Hallawa]

The animation issue happens because in PNGImageDecoder.h, the implementation of repetitionCount() is the following:

RepetitionCount repetitionCount() const override { return m_playCount-1; }

Since I changed the definition of RepetitionCount enum values, this function should like this:

RepetitionCount repetitionCount() const override { return m_playCount ? m_playCount : RepetitionCountInfinite; }

[Carlos Alberto Lopez Perez]

(In reply to Said Abou-Hallawa from comment #2)
> The animation issue happens because in PNGImageDecoder.h, the implementation
> of repetitionCount() is the following:
> 
> RepetitionCount repetitionCount() const override { return m_playCount-1; }
> 
> Since I changed the definition of RepetitionCount enum values, this function
> should like this:
> 
> RepetitionCount repetitionCount() const override { return m_playCount ?
> m_playCount : RepetitionCountInfinite; }

I tested this, and it is not fixing the problem.
The playback of the image is still weird.

I think this is better explained with a video than with words.

This is what I see (on the WebKitGTK+ MiniBrowser with a build from trunk at r214787) :

video [mp4]  : https://people.igalia.com/clopez/wkbug/170333/elephant_bug.mp4
video [webm] : https://people.igalia.com/clopez/wkbug/170333/elephant_bug.webm


An I see the same after the suggested fix.

[Said Abou-Hallawa]

Does the bug happen with animated GIF images also? Or it only happens with the animated PNG?

I just want to know it is specific to the PNG image or to the related to animated images in general.

[Carlos Alberto Lopez Perez]

(In reply to Said Abou-Hallawa from comment #4)
> Does the bug happen with animated GIF images also? Or it only happens with
> the animated PNG?
> 
> I just want to know it is specific to the PNG image or to the related to
> animated images in general.

I have converted that elephant.png image to gif with apng2gif and uploaded the result here:

https://people.igalia.com/clopez/wkbug/170333/elephant.gif

And I see the same behaviour than with the original png image [*].

So the bug seems related to animated images in general.

Can you reproduce this behaviour with the gif image on the Mac port?


[*] It seems there is something else also wrong, because I only see the gif image if I go to this index https://people.igalia.com/clopez/wkbug/170333/ and then click on the gif image. If I try to load the gif image directly or hit re-load once its loaded I don't see it (only a small square). But this seems an unrelated bug, perhaps related to the cache?. 
When the gif image loads it behaves like the apng one (like on the video)

[Carlos Alberto Lopez Perez]

(In reply to Carlos Alberto Lopez Perez from comment #5)
> [*] It seems there is something else also wrong, because I only see the gif
> image if I go to this index https://people.igalia.com/clopez/wkbug/170333/
> and then click on the gif image. If I try to load the gif image directly or
> hit re-load once its loaded I don't see it (only a small square). But this
> seems an unrelated bug, perhaps related to the cache?. 

Reported this on bug 170432

[Said Abou-Hallawa]

The same image animates fine on mac. It also animates fine on GTK the first loop only. This image has 34 frame and the frame duration is almost 42ms. So a whole loop should take 34 * 42 = 1428ms = 1.428sec. In the attached video, the image starts displaying after the the second 2. The display is fine just before the second 4. After that the animation flashes and stutters.

[Miguel Gomez]

This started to happen because of the change in the initialization of m_allowAnimatedImageAsyncDecoding in CachedImage.h. It was changed from true to false, which disables the async decoding of animations.

Theoretically m_allowAnimatedImageAsyncDecoding should be set to the animatedImageAsyncDecodingEnabled setting value afterwards, which is true, but this doesn't happen because the image doesn't have a loader, thus it keeps the false value.

[Said Abou-Hallawa]

Created attachment 306421 [details]
Patch

[Build Bot]

Comment on attachment 306421 [details]
Patch

Attachment 306421 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3486296

New failing tests:
svg/custom/anchor-on-use.svg

[Build Bot]

Created attachment 306427 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 306421 [details]
Patch

Attachment 306421 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3486374

New failing tests:
svg/custom/anchor-on-use.svg

[Build Bot]

Created attachment 306429 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Michael Catanzaro]

(In reply to Build Bot from comment #10)
> Comment on attachment 306421 [details]
> Patch
> 
> Attachment 306421 [details] did not pass mac-ews (mac):
> Output: http://webkit-queues.webkit.org/results/3486296
> 
> New failing tests:
> svg/custom/anchor-on-use.svg

Said, you'll investigate this?

[Miguel Gomez]

The deactivation of the async decoding allowed to discover that sync sync decoding is nor working properly. I'll handle that on bug 170591.

[Miguel Gomez]

Any idea about what to do with this Said? Will you investigate the error in the mac bots or should we use platform guards in the patch?

[Said Abou-Hallawa]

(In reply to Miguel Gomez from comment #16)
> Any idea about what to do with this Said? Will you investigate the error in
> the mac bots or should we use platform guards in the patch?

The reason for the Mac layout test failures is in the case of ImageDocument, there is no loader associated with the CachedImage. Therefore the settings cannot be read from the document and the async decoded is enabled. However ImageDocument::createDocumentStructure() can pass the settings to the CachedImage when the imageElement is created. The settings should have the async decoded disabled since DRT and WTR disable it in their initialization. I will have a fix soon.

[Said Abou-Hallawa]

Created attachment 308426 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 308426 [details]
Patch

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

> Source/WebCore/platform/graphics/BitmapImage.cpp:67
> +void BitmapImage::setDrawingSettings(const Settings& settings)

I would call this updateFromSettings() or something.

> Source/WebCore/platform/graphics/BitmapImage.h:217
>      bool m_animationFinished { false };
>  
> +    // The default value of m_allowSubsampling should be the same as defaultImageSubsamplingEnabled in Settings.cpp
> +#if PLATFORM(IOS)
> +    bool m_allowSubsampling { true };
> +#else
> +    bool m_allowSubsampling { false };
> +#endif
> +    bool m_allowLargeImageAsyncDecoding { false };
> +    bool m_allowAnimatedImageAsyncDecoding { false };
> +    bool m_showDebugBackground { false };
> +

Please move the bools down after m_clearDecoderAfterAsyncFrameRequestForTesting to optimize padding.

[Build Bot]

Comment on attachment 308426 [details]
Patch

Attachment 308426 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3621744

New failing tests:
fast/images/slower-decoding-than-animation-image.html

[Build Bot]

Created attachment 308436 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308426 [details]
Patch

Attachment 308426 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3621762

New failing tests:
fast/images/slower-decoding-than-animation-image.html

[Build Bot]

Created attachment 308437 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308426 [details]
Patch

Attachment 308426 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3621742

New failing tests:
fast/images/slower-decoding-than-animation-image.html

[Build Bot]

Created attachment 308439 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 308426 [details]
Patch

Attachment 308426 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3621827

New failing tests:
fast/images/slower-decoding-than-animation-image.html

[Build Bot]

Created attachment 308442 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Said Abou-Hallawa]

Created attachment 308446 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 308446 [details]
Patch

Clearing flags on attachment: 308446

Committed r215900: <http://trac.webkit.org/changeset/215900>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.