Bug 103796

Created attachment 177684 [details] Patch

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.

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

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.

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?

Created attachment 178292 [details] Patch

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.

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.

(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.

Created attachment 178317 [details] Patch for landing

Comment on attachment 178317 [details] Patch for landing Clearing flags on attachment: 178317 Committed r137008: <http://trac.webkit.org/changeset/137008>

All reviewed patches have been landed. Closing bug.

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