Bug 156766 - REGRESSION(198782): ImageSource::subsamplingLevelForScale() does not cache the MaximumSubsamplingLevel for this ImageSource
Summary: REGRESSION(198782): ImageSource::subsamplingLevelForScale() does not cache th...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords:
Depends on:
Blocks: 155322
  Show dependency treegraph
 
Reported: 2016-04-19 15:43 PDT by Said Abou-Hallawa
Modified: 2016-04-22 13:09 PDT (History)
2 users (show)

See Also:


Attachments
Patch (4.45 KB, patch)
2016-04-19 15:58 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (4.33 KB, patch)
2016-04-19 17:57 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 Said Abou-Hallawa 2016-04-19 15:43:32 PDT
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.+
Comment 1 Said Abou-Hallawa 2016-04-19 15:51:17 PDT
The image subsampling is on by default only for iOS. So this bug currently affects the iOS port.
Comment 2 Said Abou-Hallawa 2016-04-19 15:58:51 PDT
Created attachment 276768 [details]
Patch
Comment 3 Darin Adler 2016-04-19 16:11:14 PDT
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;
    }
Comment 4 Said Abou-Hallawa 2016-04-19 17:57:26 PDT
Created attachment 276785 [details]
Patch
Comment 5 Said Abou-Hallawa 2016-04-19 18:25:38 PDT
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.
Comment 6 Chris Dumez 2016-04-20 09:57:20 PDT
Did you mean to land this?
Comment 7 Said Abou-Hallawa 2016-04-20 11:18:09 PDT
(In reply to comment #6)
> Did you mean to land this?

Running tests to ensure the patch really fixes an iOS regression performance.
Comment 8 WebKit Commit Bot 2016-04-21 10:02:38 PDT
Comment on attachment 276785 [details]
Patch

Clearing flags on attachment: 276785

Committed r199821: <http://trac.webkit.org/changeset/199821>
Comment 9 WebKit Commit Bot 2016-04-21 10:02:42 PDT
All reviewed patches have been landed.  Closing bug.