Bug 163298 – [CG] Add the option to immediately decode an image frame and control its memory caching

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

[Said Abou-Hallawa]

This work is needed for asynchronous image decoding. By default an image frame is decoded only when it'e needed. Usually the image frame is needed when it is drawn. If the image decoding is done on the main thread, two things might happen. The main thread is blocked such that an activity, like scrolling, is not smooth. Secondly the image decoding is slow such that rendering a layout frame is done in more than 16ms, so we miss drawing a frame.

This bug should fix two things. It should add the option to immediately decode an image frame. This should happen on a different thread earlier than  the drawing time. The BitmapImage should also control when the image frame is deleted from memory. Currently CG can cache an image frame even if WebKit does not have a reference to it. The only way to guarantee an image frame is removed from memory is to delete the reference to the CGImageSourceRef.

[Said Abou-Hallawa]

Created attachment 291298 [details]
Patch

[Said Abou-Hallawa]

Created attachment 291300 [details]
Patch

[Tim Horton]

Comment on attachment 291300 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [CG] Add the option to immediacy decode an image frame and control its memory caching

s/immediacy/immediately/

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:80
> +    void append(const void *key, const void *value)
> +    {
> +        ASSERT(numOptions < basicOptions + extraOptions);
> +        keys[numOptions] = key;
> +        values[numOptions++] = value;
> +    }
> +
> +    static const CFIndex basicOptions = 3;
> +    static const CFIndex extraOptions = 3;
> +
> +    const void* keys[basicOptions + extraOptions];
> +    const void* values[basicOptions + extraOptions];
> +    CFIndex numOptions { 0 };

This is all very sad, maybe we should just use CFMutableDictionary?

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:367
> +        image = adoptCF(CGImageSourceCreateImageAtIndex(m_nativeDecoder.get(), index, imageSourceOptions(subsamplingLevel, decodingMode).get()));

Maybe factor out creation of imageSourceOptions for cleanliness' sake?

[Said Abou-Hallawa]

Created attachment 291310 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 291310 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:351
> +    if (decodingMode == DecodingMode::OnDemand)
> +        image = adoptCF(CGImageSourceCreateImageAtIndex(m_nativeDecoder.get(), index, options));
> +    else
> +        image = adoptCF(CGImageSourceCreateThumbnailAtIndex(m_nativeDecoder.get(), index, options));

Do we need both the call to CGImageSourceCreateThumbnailAtIndex(), AND the kCGImageSourceCreateThumbnailFromImageAlways = kCFBooleanTrue in the options?

[Said Abou-Hallawa]

Comment on attachment 291310 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:351
>> +        image = adoptCF(CGImageSourceCreateThumbnailAtIndex(m_nativeDecoder.get(), index, options));
> 
> Do we need both the call to CGImageSourceCreateThumbnailAtIndex(), AND the kCGImageSourceCreateThumbnailFromImageAlways = kCFBooleanTrue in the options?

YES, we do. Deleting one of them does not force immediate image decoding. The returned CGImageRef does not hold the decoded data. So we end up with synchronous image decoding which happens only at drawing time.

[Said Abou-Hallawa]

Created attachment 291322 [details]
image-decode-CGImageSourceCreateThumbnailAtIndex-only.trace

This is the Instruments trace when calling CGImageSourceCreateThumbnailAtIndex() with passing the flag kCGImageSourceCreateThumbnailFromImageAlways.

GIFReadPlugin::decodeGIFFrame() is called from CG::DrawOp() in the drawing thread.

[Said Abou-Hallawa]

(In reply to comment #7)
> Created attachment 291322 [details]
> image-decode-CGImageSourceCreateThumbnailAtIndex-only.trace
> 
> This is the Instruments trace when calling
> CGImageSourceCreateThumbnailAtIndex() with passing the flag
> kCGImageSourceCreateThumbnailFromImageAlways.

I meant "without passing ...."

> 
> GIFReadPlugin::decodeGIFFrame() is called from CG::DrawOp() in the drawing
> thread.

[Said Abou-Hallawa]

Created attachment 291323 [details]
image-decode-kCGImageSourceCreateThumbnailFromImageAlways-only.trace

This is the Instruments trace when calling CGImageSourceCreateImageAtIndex() with passing the flag kCGImageSourceCreateThumbnailFromImageAlways.

GIFReadPlugin::decodeGIFFrame() is called from CG::DrawOp() in the drawing thread

[Simon Fraser (smfr)]

Comment on attachment 291310 [details]
Patch

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

>>> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:351
>>> +        image = adoptCF(CGImageSourceCreateThumbnailAtIndex(m_nativeDecoder.get(), index, options));
>> 
>> Do we need both the call to CGImageSourceCreateThumbnailAtIndex(), AND the kCGImageSourceCreateThumbnailFromImageAlways = kCFBooleanTrue in the options?
> 
> YES, we do. Deleting one of them does not force immediate image decoding. The returned CGImageRef does not hold the decoded data. So we end up with synchronous image decoding which happens only at drawing time.

I see. kCGImageSourceCreateThumbnailFromImageAlways says to always create the thumbnail from the full image (not any thumbnail embedded in the image file).

[WebKit Commit Bot]

Comment on attachment 291310 [details]
Patch

Clearing flags on attachment: 291310

Committed r207182: <http://trac.webkit.org/changeset/207182>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #11)
> Comment on attachment 291310 [details]
> Patch
> 
> Clearing flags on attachment: 291310
> 
> Committed r207182: <http://trac.webkit.org/changeset/207182>

It broke the Apple Windows build, see build.webkit.org for details.

[Said Abou-Hallawa]

(In reply to comment #13)
> (In reply to comment #11)
> > Comment on attachment 291310 [details]
> > Patch
> > 
> > Clearing flags on attachment: 291310
> > 
> > Committed r207182: <http://trac.webkit.org/changeset/207182>
> 
> It broke the Apple Windows build, see build.webkit.org for details.

Yes. This is true. The error is:

C:\cygwin\home\buildbot\slave\win-release\build\Source\WebCore\platform\graphics\cg\ImageDecoderCG.cpp(77): error C2065: 'kCGImageSourceShouldCacheImmediately': undeclared identifier [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]

Working on a fix.

[Said Abou-Hallawa]

Fix the Windows build: Committed r207216: <http://trac.webkit.org/changeset/207216>