Bug 216104 - [CG] Cache the last status of the image encoded data
Summary: [CG] Cache the last status of the image encoded data
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: InRadar
Depends on:
Blocks:
 
Reported: 2020-09-02 15:48 PDT by Said Abou-Hallawa
Modified: 2020-09-14 09:22 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.40 KB, patch)
2020-09-02 16:21 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (4.41 KB, patch)
2020-09-02 17:28 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (4.65 KB, patch)
2020-09-10 13:02 PDT, Said Abou-Hallawa
youennf: review+
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (4.68 KB, patch)
2020-09-14 02:41 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 2020-09-02 15:48:54 PDT
In one of the WebKit traces, opening a Wikipedia page shows that CGImageSourceCopyPropertiesAtIndex() was called over 500 times. ImageDecoderCG::encodedDataStatus() keeps calling this function as long as the encoded data is not complete. This may happen if the network is slow. This case can be optimized by caching the last status of the image encoded data.
Comment 1 Said Abou-Hallawa 2020-09-02 16:21:12 PDT
Created attachment 407830 [details]
Patch
Comment 2 Peng Liu 2020-09-02 16:46:27 PDT
Comment on attachment 407830 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        can be made with calling system functions.

s/with/without/
Sounds like?
Comment 3 Said Abou-Hallawa 2020-09-02 17:28:03 PDT
Created attachment 407838 [details]
Patch
Comment 4 Radar WebKit Bug Importer 2020-09-09 15:49:15 PDT
<rdar://problem/68601184>
Comment 5 Tim Horton 2020-09-10 11:39:04 PDT
Comment on attachment 407838 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:-239
> -        // Ragnaros yells: TOO SOON! You have awakened me TOO SOON, Executus!

This critical historical documentation must remain!
Comment 6 Said Abou-Hallawa 2020-09-10 13:02:11 PDT
Created attachment 408470 [details]
Patch
Comment 7 Said Abou-Hallawa 2020-09-10 13:03:06 PDT
Comment on attachment 407838 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:-239
>> -        // Ragnaros yells: TOO SOON! You have awakened me TOO SOON, Executus!
> 
> This critical historical documentation must remain!

I put it back :)
Comment 8 youenn fablet 2020-09-11 00:07:48 PDT
Comment on attachment 408470 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:253
> +            m_encodedDataStatus = EncodedDataStatus::Error;

Could be a one liner:
m_encodedDataStatus = !m_isAllDataReceived ? EncodedDataStatus::Unknown : EncodedDataStatus::Error;

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:260
>          RetainPtr<CFDictionaryRef> image0Properties = adoptCF(CGImageSourceCopyPropertiesAtIndex(m_nativeDecoder.get(), 0, imageSourceOptions().get()));

auto

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:266
> +        if (!CFDictionaryContainsKey(image0Properties.get(), kCGImagePropertyPixelWidth) || !CFDictionaryContainsKey(image0Properties.get(), kCGImagePropertyPixelHeight)) {

You could group both this if and previous if in a single statement.
Comment 9 Said Abou-Hallawa 2020-09-14 02:41:43 PDT
Created attachment 408694 [details]
Patch
Comment 10 EWS 2020-09-14 09:22:35 PDT
Committed r267016: <https://trac.webkit.org/changeset/267016>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408694 [details].