Bug 103796 – [chromium] ImageDecodingStore should cache partially decoded images

• Component: Images
• Status: RESOLVED FIXED

[Hin-Chung Lam]

Current implementation does not cache partially decoded images. The result is that when an image is partially decoded, the bitmap and corresponding ImageDecoder is deleted. The means any subsequent decoding will have to start from scratch.

Redoing image decoding many times is very expensive and should be avoided. The solution to this is to cache partially decoded bitmap together with the ImageDecoder.

[Hin-Chung Lam]

Created attachment 177684 [details]
Patch

[Hin-Chung Lam]

Comment on attachment 177684 [details]
Patch

The seems big but if you focus on ImageDecodingStore and ImageFrameGenerator the others are just test and changes to mock modules.

ImageDecodingStore

Added logic to handle incomplete cache entries.

ImageFrameGenerator

Added logic to resume decoding using a cached decoder. The change has some code move involved to share code with decoding starting from scratch.

[Hin-Chung Lam]

Woohoo! Tested this with Chrome and it works with large JPEG files too, which means this incomplete image caching is working.

[Hin-Chung Lam]

nduca, senorblanco: have time to have a look? As we have discussed we'll change the resampling algorithm later so it's now in it's own function and easy to swap out.

[Stephen White]

Comment on attachment 177684 [details]
Patch

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

Please fix the anonymous bool.  The rest are nits and suggestions, which you can heed or ignore at your whim.

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:80
> +bool ImageDecodingStore::lockCache(const ImageFrameGenerator* generator, const SkISize& scaledSize, bool cacheMustBeComplete, const ScaledImageFragment** cachedImage, ImageDecoder** decoder)

For cacheMustBeComplete:  "Prefer enums to bools on function parameters if callers are likely to be passing constants, since named constants are easier to read at the call site."

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:82
>      CacheEntry* cacheEntry = 0;

Maybe ASSERT(cachedImage) here?  Seems to be non-optional outparam.  (I'd actually keep it as the actual return value, since only decoder seems to be optional, but that's just personal style.)

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:103
> +    if (decoder)
> +        *decoder = cacheEntry->cachedDecoder();

(Suggestion)  Would it make sense to have this function return a cacheEntry instead, so the caller could just get the decoder from it?  Or would that expose too much implementation to the caller?

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:152
> +    OwnPtr<ImageDecoder> trash;

Does this really need to live to end of function scope?  (I'm guessing yes, just making sure.)

> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:168
> +        cacheEntry->overwriteCachedImage(newImage);
> +        newCachedImage = cacheEntry->cachedImage();
> +
> +        // Image is complete then decoder is not needed.
> +        if (cacheEntry->cachedImage()->isComplete())
> +            trash = cacheEntry->releaseCachedDecoder();

(Suggestion) It seems like the only place you releaseCachedDecoder() is right after calling overwriteCachedImage().  If that's always going to be true, perhaps you should just have overwriteCachedImage() release the decoder itself if isComplete() is true?

[Hin-Chung Lam]

Created attachment 178292 [details]
Patch

[Hin-Chung Lam]

Comment on attachment 177684 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:80
>> +bool ImageDecodingStore::lockCache(const ImageFrameGenerator* generator, const SkISize& scaledSize, bool cacheMustBeComplete, const ScaledImageFragment** cachedImage, ImageDecoder** decoder)
> 
> For cacheMustBeComplete:  "Prefer enums to bools on function parameters if callers are likely to be passing constants, since named constants are easier to read at the call site."

Okay.

>> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:82
>>      CacheEntry* cacheEntry = 0;
> 
> Maybe ASSERT(cachedImage) here?  Seems to be non-optional outparam.  (I'd actually keep it as the actual return value, since only decoder seems to be optional, but that's just personal style.)

Done.

>> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:103
>> +        *decoder = cacheEntry->cachedDecoder();
> 
> (Suggestion)  Would it make sense to have this function return a cacheEntry instead, so the caller could just get the decoder from it?  Or would that expose too much implementation to the caller?

I want to keep CacheEntry internal to ImageDecodingStore.

>> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:152
>> +    OwnPtr<ImageDecoder> trash;
> 
> Does this really need to live to end of function scope?  (I'm guessing yes, just making sure.)

Yes so that deleting the object doesn't live inside the lock.

>> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:168
>> +            trash = cacheEntry->releaseCachedDecoder();
> 
> (Suggestion) It seems like the only place you releaseCachedDecoder() is right after calling overwriteCachedImage().  If that's always going to be true, perhaps you should just have overwriteCachedImage() release the decoder itself if isComplete() is true?

Good idea, will do.

[Stephen White]

Comment on attachment 178292 [details]
Patch

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

OK.  The logic still seems a bit complex, but perhaps that can be cleaned up later.  r=me

> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:191
> +            *decoder = m_imageDecoderFactory->create().leakPtr();

I wonder if this function (and others) should take an OwnPtr<ImageDecoder>* instead?  Then you wouldn't have to call leakPtr() here.

[Hin-Chung Lam]

(In reply to comment #8)
> (From update of attachment 178292 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178292&action=review
> 
> OK.  The logic still seems a bit complex, but perhaps that can be cleaned up later.  r=me
> 
> > Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:191
> > +            *decoder = m_imageDecoderFactory->create().leakPtr();
> 

ImageDecodingStore is pretty close to complete and concise. There will be some more work in ImageFrameGenerator later.

> I wonder if this function (and others) should take an OwnPtr<ImageDecoder>* instead?  Then you wouldn't have to call leakPtr() here.

One caller doesn't transfer ownership hence it's a ** parameter.

[Hin-Chung Lam]

Created attachment 178317 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 178317 [details]
Patch for landing

Clearing flags on attachment: 178317

Committed r137008: <http://trac.webkit.org/changeset/137008>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Min Qin]

*** Bug 104311 has been marked as a duplicate of this bug. ***