REGRESSION(198782): ImageSource::subsamplingLevelForScale() does not cache the MaximumSubsamplingLevel for this ImageSource

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

Calling ImageSource::calculateMaximumSubsamplingLevel() every time ImageSource::subsamplingLevelForScale() is called is expensive because it requires fetching the first frame for up to three subsampling level. Prior to r198782, ImageSource::calculateMaximumSubsamplingLevel() (which was named BitmapImage::determineMinimumSubsamplingLevel()) was called only once from BitmapImage::updateSize(). The fix should be to cache the maximumSubsamplingLevel when the image size is available.+

[Said Abou-Hallawa]

The image subsampling is on by default only for iOS. So this bug currently affects the iOS port.

[Said Abou-Hallawa]

Created attachment 276768 [details]
Patch

[Darin Adler]

Comment on attachment 276768 [details]
Patch

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

> Source/WebCore/platform/graphics/ImageSource.cpp:132
> +    if (cacheMetadata(), !m_maximumSubsamplingLevel)
>          return 0;

This use of the comma expression is not idiomatic for WebKit. The way we would write this in WebKit coding style is:

    cacheMetadata();
    if (!m_maximumSubsamplingLevel)

> Source/WebCore/platform/graphics/ImageSource.h:122
> +    size_t frameCount() { return cacheMetadata(), m_frameCount; }

Ditto. Please don't format it like this. Instead I suggest writing it after the class definition like this:

    inline size_t ImageSource::frameCount()
    {
        cacheMetadata();
        return m_frameCount;
    }

[Said Abou-Hallawa]

Created attachment 276785 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 276768 [details]
Patch

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

>> Source/WebCore/platform/graphics/ImageSource.cpp:132
>>          return 0;
> 
> This use of the comma expression is not idiomatic for WebKit. The way we would write this in WebKit coding style is:
> 
>     cacheMetadata();
>     if (!m_maximumSubsamplingLevel)

Done. The comma expression was split into two statements.

>> Source/WebCore/platform/graphics/ImageSource.h:122
>> +    size_t frameCount() { return cacheMetadata(), m_frameCount; }
> 
> Ditto. Please don't format it like this. Instead I suggest writing it after the class definition like this:
> 
>     inline size_t ImageSource::frameCount()
>     {
>         cacheMetadata();
>         return m_frameCount;
>     }

Done. The comma expression was split into two statements.

[Chris Dumez]

Did you mean to land this?

[Said Abou-Hallawa]

(In reply to comment #6)
> Did you mean to land this?

Running tests to ensure the patch really fixes an iOS regression performance.

[WebKit Commit Bot]

Comment on attachment 276785 [details]
Patch

Clearing flags on attachment: 276785

Committed r199821: <http://trac.webkit.org/changeset/199821>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.