Bug 157500 – REGRESSION (r199821): Large animated GIFs with slow network do not animate till the last frame

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Said Abou-Hallawa]

In r199821, some of the image metadata were cached in the ImageSource object. The purpose of this change was to save time which we may encounter when calculating the maximumSubsamplingLevel on iOS. Having the image metadata and the image frames cached in the ImageSource is part of the asynchronous image decoding feature https://bugs.webkit.org/show_bug.cgi?id=155322. But because a flag is needed to indicate whether the image metadata has already cached or not, I used the cached frameCount for this purpose. The assumption was the image frameCount can be retrieved once the image size is available and it is not going to change. It turned out this assumption is wrong.

A bug may happen with large GIFs where the frame count gets changed every time a new data block is decoded. Therefore the cached frameCount can be stale data and needs to be updated.

A possible way to fix this bug is to invalidate the image metadata cache every time new data block is decoded. This will achieve the following goals:
1. We get the correct image metadata regardless they are final when decoding the first data block or they are going to change later.
2. We save time by calculating maximumSubsamplingLevel only once.
3. We do some work towards the asynchronous image decoding feature instead of rolling out r199821.

[Said Abou-Hallawa]

Created attachment 278474 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 278474 [details]
Patch

What was the behavior before your change? Does this change cause more decoding of metadata than before the regression point? Can we only do this for animated images?

[Said Abou-Hallawa]

(In reply to comment #2)
> Comment on attachment 278474 [details]
> Patch
> 
> What was the behavior before your change? Does this change cause more
> decoding of metadata than before the regression point? Can we only do this
> for animated images?

Before I started the asynchronous image decoding work, BitmapImage::m_frameCount was invalidated every time BitmapImage::dataChanged() is called. This was happening by setting BitmapImage::m_haveFrameCount to false. The next time BitmapImage::frameCount() is called, ImageSource::frameCount() was called and BitmapImage::m_haveFrameCount to true. Because ImageSource for CG was actually the image decoder, ImageSource::frameCount() was calling CGImageSourceGetCount().

In r199821, I partially moved getting the frameCount from BitmapImage to ImageSource but I assumed the frameCount is constant and will not change once the image size is available. This was enforcing calling CGImageSourceGetCount() only once for any image including large animated GIFs where the frameCount can change every time a data block is decoded.

With this patch, we are back to the same behavior before r199821. Setting BitmapImage::m_haveFrameCount is left as is even though BitmapImage::m_haveFrameCount and BitmapImage::m_frameCount are not really needed anymore. ImageSource::frameCount()  can be used directly to get the frameCount. With some other clean-up they can both deleted. ImageSource::m_needsUpdateMetadata is initialized with true expect for the case when BitmapImage is created for a single NativeImagePtr. Every time BitmapImage::dataChanged() is called, BitmapImage::m_haveFrameCount is set to false and ImageSource::m_needsUpdateMetadata is set to true. When BitmapImage::frameCount() is called and finds BitmapImage::m_haveFrameCount is false it calls ImageSource::frameCount() which finds also ImageSource::m_needsUpdateMetadata is true. In this case, it reevaluates ImageSource::m_frameCount by calling CGImageSourceGetCount().

Regarding reevaluating the frameCount only for the animated images, as far as I know the only way to know whether the image is animated or not is through its frameCount. If the frameCount == 1 and we received a new block of data, we can't know if calling CGImageSourceGetCount() after decoding this block is going to get 1 or more than 1.

We can also roll out r199821 for now.

[Simon Fraser (smfr)]

Comment on attachment 278474 [details]
Patch

OK, I think this is fine. Is it testable?

[Darin Adler]

Comment on attachment 278474 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageSource.cpp:113
> +    if (!(needsUpdateMetadata() && isSizeAvailable()))

Seems that this should look at the data member directly, not call the needsUpdateMetdata() function.

> Source/WebCore/platform/graphics/ImageSource.cpp:120
> +    setNeedsUpdateMetadata(false);

Seems that this should set the data member directly, not call a function.

> Source/WebCore/platform/graphics/ImageSource.h:113
> +    bool needsUpdateMetadata() const { return m_needsUpdateMetadata; }

I am not sure we should expose this function.

> Source/WebCore/platform/graphics/ImageSource.h:114
> +    void setNeedsUpdateMetadata(bool needsUpdateMetadata) { m_needsUpdateMetadata = needsUpdateMetadata; }

I don’t think this function needs an argument. Should just set to true.

[Said Abou-Hallawa]

Created attachment 278673 [details]
Patch

[Said Abou-Hallawa]

Created attachment 278674 [details]
Patch

[Said Abou-Hallawa]

Created attachment 278739 [details]
Patch

[Build Bot]

Comment on attachment 278739 [details]
Patch

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

New failing tests:
http/tests/misc/slow-loading-animated-image.html

[Build Bot]

Created attachment 278743 [details]
Archive of layout-test-results from ews101 for mac-yosemite

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

[Said Abou-Hallawa]

Created attachment 278745 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 278739 [details]
Patch

I really think it's worth making an http test with a slow-serving script to test this.

[Simon Fraser (smfr)]

...or figure out why http/tests/misc/slow-loading-animated-image.html didn't spot the original regression.

[Build Bot]

Comment on attachment 278745 [details]
Patch

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

New failing tests:
http/tests/misc/slow-loading-animated-image.html

[Build Bot]

Created attachment 278751 [details]
Archive of layout-test-results from ews117 for mac-yosemite

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

[Build Bot]

Comment on attachment 278745 [details]
Patch

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

New failing tests:
http/tests/misc/slow-loading-animated-image.html

[Build Bot]

Created attachment 278752 [details]
Archive of layout-test-results from ews101 for mac-yosemite

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

[Said Abou-Hallawa]

Created attachment 278753 [details]
Patch

[Build Bot]

Comment on attachment 278753 [details]
Patch

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

New failing tests:
http/tests/misc/slow-loading-animated-image.html

[Build Bot]

Created attachment 278758 [details]
Archive of layout-test-results from ews115 for mac-yosemite

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

[Build Bot]

Comment on attachment 278753 [details]
Patch

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

New failing tests:
http/tests/misc/slow-loading-animated-image.html

[Build Bot]

Created attachment 278763 [details]
Archive of layout-test-results from ews101 for mac-yosemite

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

[Said Abou-Hallawa]

Created attachment 278766 [details]
Patch

[Build Bot]

Comment on attachment 278766 [details]
Patch

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

New failing tests:
http/tests/misc/slow-loading-animated-image.html

[Build Bot]

Created attachment 278775 [details]
Archive of layout-test-results from ews112 for mac-yosemite

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

[Said Abou-Hallawa]

These failures happen in WK1 which also fails locally.

[Build Bot]

Comment on attachment 278766 [details]
Patch

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

New failing tests:
http/tests/misc/slow-loading-animated-image.html

[Build Bot]

Created attachment 278785 [details]
Archive of layout-test-results from ews101 for mac-yosemite

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

[Said Abou-Hallawa]

Created attachment 278881 [details]
Patch

[Build Bot]

Comment on attachment 278881 [details]
Patch

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

New failing tests:
http/tests/misc/slow-loading-animated-image.html

[Build Bot]

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

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

[Said Abou-Hallawa]

Created attachment 278888 [details]
Patch

[Said Abou-Hallawa]

The ios-simulator bot is slow. Animating 33 frames took 1000 ms. Add 500ms extra waiting time to be sure the last frame is displayed.

[Said Abou-Hallawa]

Created attachment 278899 [details]
Patch

[Said Abou-Hallawa]

(In reply to comment #4)
> Comment on attachment 278474 [details]
> Patch
> 
> OK, I think this is fine. Is it testable?

A test was added but it takes 2.5 seconds to finish.

[Said Abou-Hallawa]

Comment on attachment 278474 [details]
Patch

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

>> Source/WebCore/platform/graphics/ImageSource.cpp:113
>> +    if (!(needsUpdateMetadata() && isSizeAvailable()))
> 
> Seems that this should look at the data member directly, not call the needsUpdateMetdata() function.

Done.

>> Source/WebCore/platform/graphics/ImageSource.cpp:120
>> +    setNeedsUpdateMetadata(false);
> 
> Seems that this should set the data member directly, not call a function.

Done.

>> Source/WebCore/platform/graphics/ImageSource.h:113
>> +    bool needsUpdateMetadata() const { return m_needsUpdateMetadata; }
> 
> I am not sure we should expose this function.

This functions was deleted.

>> Source/WebCore/platform/graphics/ImageSource.h:114
>> +    void setNeedsUpdateMetadata(bool needsUpdateMetadata) { m_needsUpdateMetadata = needsUpdateMetadata; }
> 
> I don’t think this function needs an argument. Should just set to true.

The parameter was removed.

[Said Abou-Hallawa]

Created attachment 278907 [details]
Patch

[Darin Adler]

Comment on attachment 278907 [details]
Patch

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

> Source/WebCore/platform/graphics/BitmapImage.h:86
> +    static IntSize sizeOfImage(const NativeImagePtr&);
> +    static bool alphaOfImage(const NativeImagePtr&);

Why do these functions have "OfImage" in their names? They take an argument, so clearly they are the size of the thing that is passed in. Is there perhaps a conflict between these functions and others with the same names that prevents us from just calling them size and hasAlpha?

It’s unclear what a function named “alpha” that returns a boolean means. It would be better to have a more precise name. Maybe hasAlpha is precise enough.

Why doe these functions need to be members of BitmapImage rather than free functions?

I think it’s good to have functions names size and hasAlpha that can be called on NativeImagePtr, but I don’t think they should necessarily be static function members of BitmapImage.

> Source/WebCore/platform/graphics/ImageSource.cpp:48
> +    , m_maximumSubsamplingLevel(-1)

Can we use Optional to indicate we don’t have a level, rather than a magic number of -1?

[Darin Adler]

Comment on attachment 278907 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/BitmapImageCG.cpp:61
> +bool FrameData::alphaOfImage(const RetainPtr<CGImageRef>&)
> +{
> +    return true;
> +}

Given this implementation, this function needs to be named mayHaveAlpha, I think, since it doesn’t really correctly answer the question.

[Said Abou-Hallawa]

Created attachment 279003 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 279003 [details]
Patch

Clearing flags on attachment: 279003

Committed r200939: <http://trac.webkit.org/changeset/200939>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Said Abou-Hallawa]

<rdar://problem/26180740>

[Said Abou-Hallawa]

Comment on attachment 278907 [details]
Patch

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

>> Source/WebCore/platform/graphics/BitmapImage.h:86
>> +    static bool alphaOfImage(const NativeImagePtr&);
> 
> Why do these functions have "OfImage" in their names? They take an argument, so clearly they are the size of the thing that is passed in. Is there perhaps a conflict between these functions and others with the same names that prevents us from just calling them size and hasAlpha?
> 
> It’s unclear what a function named “alpha” that returns a boolean means. It would be better to have a more precise name. Maybe hasAlpha is precise enough.
> 
> Why doe these functions need to be members of BitmapImage rather than free functions?
> 
> I think it’s good to have functions names size and hasAlpha that can be called on NativeImagePtr, but I don’t think they should necessarily be static function members of BitmapImage.

Names were changed and the functions were moved out of the FrameData. To keep names as suggested, I put them in a namespace called "NativeImage"

>> Source/WebCore/platform/graphics/ImageSource.cpp:48
>> +    , m_maximumSubsamplingLevel(-1)
> 
> Can we use Optional to indicate we don’t have a level, rather than a magic number of -1?

Done.

>> Source/WebCore/platform/graphics/cg/BitmapImageCG.cpp:61
>> +}
> 
> Given this implementation, this function needs to be named mayHaveAlpha, I think, since it doesn’t really correctly answer the question.

I kept the name hasAlpha() but I added a FIXME comment because I realized that hasAlpha information can be calculated for CG by calling CGImageGetAlphaInfo().

[Alexey Proskuryakov]

The new test is very flaky on iOS, filed bug 157916.