Bug 170333 - REGRESSION(r213764): Async decoding of animated images is disabled for ImageDocument
Summary: REGRESSION(r213764): Async decoding of animated images is disabled for ImageD...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords:
Depends on:
Blocks: 165039
  Show dependency treegraph
 
Reported: 2017-03-31 02:16 PDT by Miguel Gomez
Modified: 2017-04-27 15:57 PDT (History)
13 users (show)

See Also:


Attachments
Patch (1.88 KB, patch)
2017-04-06 13:30 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (1.01 MB, application/zip)
2017-04-06 14:28 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-elcapitan (1.83 MB, application/zip)
2017-04-06 15:00 PDT, Build Bot
no flags Details
Patch (17.93 KB, patch)
2017-04-27 12:26 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (1.05 MB, application/zip)
2017-04-27 13:32 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.09 MB, application/zip)
2017-04-27 13:37 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-elcapitan (1.86 MB, application/zip)
2017-04-27 13:42 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (1.10 MB, application/zip)
2017-04-27 14:09 PDT, Build Bot
no flags Details
Patch (19.08 KB, patch)
2017-04-27 14:24 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Miguel Gomez 2017-03-31 02:16:13 PDT
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.
Comment 1 Carlos Alberto Lopez Perez 2017-04-01 08:47:44 PDT
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.
Comment 2 Said Abou-Hallawa 2017-04-03 10:55:50 PDT
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; }
Comment 3 Carlos Alberto Lopez Perez 2017-04-03 12:36:43 PDT
(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.
Comment 4 Said Abou-Hallawa 2017-04-03 15:40:18 PDT
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.
Comment 5 Carlos Alberto Lopez Perez 2017-04-03 16:18:55 PDT
(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)
Comment 6 Carlos Alberto Lopez Perez 2017-04-03 17:26:58 PDT
(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
Comment 7 Said Abou-Hallawa 2017-04-03 17:47:16 PDT
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.
Comment 8 Miguel Gomez 2017-04-06 07:08:24 PDT
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.
Comment 9 Said Abou-Hallawa 2017-04-06 13:30:44 PDT
Created attachment 306421 [details]
Patch
Comment 10 Build Bot 2017-04-06 14:27:59 PDT
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
Comment 11 Build Bot 2017-04-06 14:28:00 PDT
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
Comment 12 Build Bot 2017-04-06 15:00:05 PDT
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
Comment 13 Build Bot 2017-04-06 15:00:06 PDT
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
Comment 14 Michael Catanzaro 2017-04-06 15:48:38 PDT
(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?
Comment 15 Miguel Gomez 2017-04-07 02:18:54 PDT
The deactivation of the async decoding allowed to discover that sync sync decoding is nor working properly. I'll handle that on bug 170591.
Comment 16 Miguel Gomez 2017-04-12 02:48:02 PDT
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?
Comment 17 Said Abou-Hallawa 2017-04-12 10:03:02 PDT
(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.
Comment 18 Said Abou-Hallawa 2017-04-27 12:26:02 PDT
Created attachment 308426 [details]
Patch
Comment 19 Simon Fraser (smfr) 2017-04-27 12:44:22 PDT
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.
Comment 20 Build Bot 2017-04-27 13:32:00 PDT
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
Comment 21 Build Bot 2017-04-27 13:32:01 PDT
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
Comment 22 Build Bot 2017-04-27 13:37:49 PDT
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
Comment 23 Build Bot 2017-04-27 13:37:51 PDT
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
Comment 24 Build Bot 2017-04-27 13:42:45 PDT
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
Comment 25 Build Bot 2017-04-27 13:42:47 PDT
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
Comment 26 Build Bot 2017-04-27 14:09:05 PDT
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
Comment 27 Build Bot 2017-04-27 14:09:07 PDT
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
Comment 28 Said Abou-Hallawa 2017-04-27 14:24:26 PDT
Created attachment 308446 [details]
Patch
Comment 29 WebKit Commit Bot 2017-04-27 15:57:35 PDT
Comment on attachment 308446 [details]
Patch

Clearing flags on attachment: 308446

Committed r215900: <http://trac.webkit.org/changeset/215900>
Comment 30 WebKit Commit Bot 2017-04-27 15:57:38 PDT
All reviewed patches have been landed.  Closing bug.