| Summary: | [chromium] Implement full-featured image cache | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Hin-Chung Lam <hclam> | ||||||||||||||||
| Component: | Images | Assignee: | Hin-Chung Lam <hclam> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | cc-bugs, jamesr, levin+threading, nduca, noel.gordon, qinmin, senorblanco, vangelis, webkit.review.bot | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Bug Depends on: | 97347, 97481, 98098, 99785, 103354 | ||||||||||||||||||
| Bug Blocks: | 99790 | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Hin-Chung Lam
2012-10-18 18:04:52 PDT
*** Bug 99785 has been marked as a duplicate of this bug. *** Created attachment 175926 [details]
Patch
Attachment 175926 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/chromium/SkSizeHash.h:29: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 1 in 19 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 175927 [details]
Patch
There will be one style problem in SkSizeHash.h because SkTypes.h needs to included before SkSize.h, otherwise visual studio will fail compile. Attachment 175927 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/chromium/SkSizeHash.h:29: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 1 in 19 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 175927 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175927&action=review > Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.h:85 > + static bool m_enabled; static members in WebKit have an s_ prefix, not m_ > Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:82 > + CacheEntry* cacheEntry = iter->value; > + if (!cacheEntry->cachedImage->isComplete()) > + return 0; > + cacheEntry->cachedImage->bitmap().lockPixels(); > + ++cacheEntry->useCount; > + return cacheEntry->cachedImage.get(); do you need to hold the mutex for all of these operations? > Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:127 > + OwnPtr<CacheEntry> newCacheEntry = CacheEntry::createAndUse(image); does this have to happen with the lock held? it doesn't appear to mutate any shared data in general, you always want to hold mutexes for as short as possible. for instance, try to avoid unnecessary allocations > Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:131 > + CacheMap::iterator iter = m_cacheMap.find(key); > + ASSERT_UNUSED(iter, iter == m_cacheMap.end()); this does a map lookup always just to do an ASSERT, which is a bummer since we'll do a totally unnecessary map lookup in production builds. can you put the lookup itself in the ASSERT()? you can use bool HashMap::contains(const KeyType&) > Source/WebCore/platform/graphics/chromium/ImageDecodingStore.h:54 > + // These methods are thread-safe and protected by a mutex. what are the expected lock/unlock semantics here? it's a bit confusing to mention the mutex here when talking about lock/unlock - naively i would expect these operations to have to do with thread synchronization as well, but from reading the code it appears that's not what they are for > Source/WebCore/platform/graphics/chromium/ImageDecodingStore.h:82 > + int useCount; what's the intended use of this? for prune() ? > Source/WebCore/platform/graphics/chromium/ImageDecodingStore.h:93 > + Mutex m_mutex; document what members this mutex protects. for instance are the entries in the m_cacheMap / m_cacheEntries protected, or just the containers themselves? > Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:64 > + // Prevents concurrent decode or scale operations on the same image data. hmmmm - when dealing with a large scale i would expect the image to span multiple tiles. this lock means that the raster operations for all tiles referencing the same image to block on the first decode. is that the desired behavior? is that something that should be decided in this code? > Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:101 > + skia::ImageOperations::RESIZE_LANCZOS3, do we always want LANCZOS? the 'normal' resizing code has logic to do linear filtering when possible, which is pretty important for performance > Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:136 > + OwnPtr<ScaledImageFragment> fullSizeImage = ScaledImageFragment::create(m_fullSize, bitmap, isComplete); can the rest of this function share code with tryToScale() ? (In reply to comment #5) > There will be one style problem in SkSizeHash.h because SkTypes.h needs to included before SkSize.h, otherwise visual studio will fail compile. Can you fix this in skia, for instance by moving the #include SkTypes.h up higher in SkSize.h ? (it looks like it's currently picked up transitively by the #include "SkScalar.h" line halfway down the file) Created attachment 176052 [details]
Patch
Comment on attachment 175927 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175927&action=review >> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.h:85 >> + static bool m_enabled; > > static members in WebKit have an s_ prefix, not m_ Done. >> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:82 >> + return cacheEntry->cachedImage.get(); > > do you need to hold the mutex for all of these operations? Updated code to only include cache lookup and use count increment. >> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:127 >> + OwnPtr<CacheEntry> newCacheEntry = CacheEntry::createAndUse(image); > > does this have to happen with the lock held? it doesn't appear to mutate any shared data > > in general, you always want to hold mutexes for as short as possible. for instance, try to avoid unnecessary allocations Done. >> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:131 >> + ASSERT_UNUSED(iter, iter == m_cacheMap.end()); > > this does a map lookup always just to do an ASSERT, which is a bummer since we'll do a totally unnecessary map lookup in production builds. can you put the lookup itself in the ASSERT()? you can use bool HashMap::contains(const KeyType&) Done. >> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.h:54 >> + // These methods are thread-safe and protected by a mutex. > > what are the expected lock/unlock semantics here? it's a bit confusing to mention the mutex here when talking about lock/unlock - naively i would expect these operations to have to do with thread synchronization as well, but from reading the code it appears that's not what they are for Mutex is just for protecting concurrent access to shared data. Removed the comments here and mention it next to m_mutex. >> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.h:82 >> + int useCount; > > what's the intended use of this? for prune() ? Yes for prune(). To protect an entry from being evicted while being used. >> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.h:93 >> + Mutex m_mutex; > > document what members this mutex protects. for instance are the entries in the m_cacheMap / m_cacheEntries protected, or just the containers themselves? Done. >> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:64 >> + // Prevents concurrent decode or scale operations on the same image data. > > hmmmm - when dealing with a large scale i would expect the image to span multiple tiles. this lock means that the raster operations for all tiles referencing the same image to block on the first decode. is that the desired behavior? is that something that should be decided in this code? It should be decided by the task scheduler for rasterization. The task scheduler will lock SkPixelRef once and then kick off the raster tasks. >> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:101 >> + skia::ImageOperations::RESIZE_LANCZOS3, > > do we always want LANCZOS? the 'normal' resizing code has logic to do linear filtering when possible, which is pretty important for performance Current non deferred-decoded case uses LANCZOS3 so this code stays consistent to pass layout tests. I took it out as a separate method so we can adjust this parameter later. >> Source/WebCore/platform/graphics/chromium/ImageFrameGenerator.cpp:136 >> + OwnPtr<ScaledImageFragment> fullSizeImage = ScaledImageFragment::create(m_fullSize, bitmap, isComplete); > > can the rest of this function share code with tryToScale() ? Done and merged code with tryToScale(). >> Source/WebCore/platform/graphics/chromium/SkSizeHash.h:29 >> +#include "SkTypes.h" > > Alphabetical sorting problem. [build/include_order] [4] Used SkScalar.h instead. Fix is landed in Skia will roll later. Created attachment 176085 [details]
Patch
The latest upload uses const ScaledImageFragment for return values of all cache operations. This is to indicate that the returned object cannot be mutated by user. Comment on attachment 176085 [details]
Patch
Looks pretty good as far as I can tell. Stephen / Nat - either of you want to take a look at this before landing?
Comment on attachment 176085 [details]
Patch
Looks good
Comment on attachment 176085 [details] Patch Clearing flags on attachment: 176085 Committed r135798: <http://trac.webkit.org/changeset/135798> All reviewed patches have been landed. Closing bug. Seems to have broken the mac chrome build: http://build.webkit.org/builders/Chromium%20Mac%20Release/builds/50287/steps/compile-webkit/logs/stdio Can you investigate please? Build funk for reference.
In file included from /Volumes/data/b/WebKit-BuildSlave/chromium-mac-release/build/Source/WebCore/WebCore.gyp/../platform/graphics/chromium/ImageDecodingStore.cpp:27:
In file included from ../platform/graphics/chromium/ImageDecodingStore.h:29:
In file included from ../platform/graphics/chromium/ScaledImageFragment.h:33:
In file included from ../../WTF/wtf/PassOwnPtr.h:31:
../../WTF/wtf/OwnPtrCommon.h:60:13: error: delete called on 'WebCore::ImageDecoderFactory' that is abstract but has non-virtual destructor [-Werror,-Wdelete-non-virtual-dtor]
delete ptr;
^
../../WTF/wtf/OwnPtr.h:141:9: note: in instantiation of function template specialization 'WTF::deleteOwnedPtr<WebCore::ImageDecoderFactory>' requested here
deleteOwnedPtr(ptr);
^
../platform/graphics/chromium/ImageFrameGenerator.h:64:108: note: in instantiation of member function 'WTF::OwnPtr<WebCore::ImageDecoderFactory>::operator=' requested here
void setImageDecoderFactoryForTesting(PassOwnPtr<ImageDecoderFactory> factory) { m_imageDecoderFactory = factory; }
^
1 error generated.
Re-opened since this is blocked by bug 103354 Created attachment 176165 [details]
Patch
Added a virtual destructor in the last upload. This should appease mac buildbot. jamesr: Mind take a quick look? This is a re-land with build fix. Comment on attachment 176165 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176165&action=review > Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:86 > + // FIXME: This code has the potential problem that multiple > + // LazyDecodingPixelRefs are created even though they share the same > + // scaled size and ImageFrameGenerator. Isn't this what the cache is supposed to handle? Its key includes the scaled size, no? (Pardon me if I'm being stupid.. I'm still a little jet-lagged.) > Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:136 > + m_cacheMap.add(key, newCacheEntry.get()); > + m_cacheEntries.append(newCacheEntry.release()); An explanation of why you need both of these would be helpful. I.e., why m_cacheMap can't simply own the cache entries. Created attachment 176325 [details]
Patch
Comment on attachment 176165 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176165&action=review >> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:86 >> + // scaled size and ImageFrameGenerator. > > Isn't this what the cache is supposed to handle? Its key includes the scaled size, no? > > (Pardon me if I'm being stupid.. I'm still a little jet-lagged.) Yes the cache will handle this by saving only one copy of scaled image, since the image is indexed by ImageFrameGenerator* which is shared by these SkPixelRefs. The potential problem is that Skia cannot be smart in this case by uploading one version of bitmap to GPU texture, because they have different generationID, even though the backing content is the same. >> Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:136 >> + m_cacheEntries.append(newCacheEntry.release()); > > An explanation of why you need both of these would be helpful. I.e., why m_cacheMap can't simply own the cache entries. Done. Please see the comments. Comment on attachment 176165 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176165&action=review >>> Source/WebCore/platform/graphics/chromium/DeferredImageDecoder.cpp:86 >>> + // scaled size and ImageFrameGenerator. >> >> Isn't this what the cache is supposed to handle? Its key includes the scaled size, no? >> >> (Pardon me if I'm being stupid.. I'm still a little jet-lagged.) > > Yes the cache will handle this by saving only one copy of scaled image, since the image is indexed by ImageFrameGenerator* which is shared by these SkPixelRefs. The potential problem is that Skia cannot be smart in this case by uploading one version of bitmap to GPU texture, because they have different generationID, even though the backing content is the same. Oh, hmm. That is unfortunate. > > Yes the cache will handle this by saving only one copy of scaled image, since the image is indexed by ImageFrameGenerator* which is shared by these SkPixelRefs. The potential problem is that Skia cannot be smart in this case by uploading one version of bitmap to GPU texture, because they have different generationID, even though the backing content is the same.
>
> Oh, hmm. That is unfortunate.
We can fix this easily but let's do it separately.
The fix is DeferredImageDecoder can "cache" SkPixelRef for a particular scale, the tradeoff is we can't easily do a scale-and-clip but we don't have that today anyway.
Comment on attachment 176325 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176325&action=review OK. r=me > Source/WebCore/platform/graphics/chromium/ImageDecodingStore.cpp:138 > + // entries quickly, it also owns the cached images. Grammar nit: comma splice. (Both sides of the comma are complete sentences. Fix is to use a semicolon, or split into two sentences.) Much too small to r-, though. Created attachment 176326 [details]
Patch for landing
Comment on attachment 176326 [details] Patch for landing Clearing flags on attachment: 176326 Committed r135911: <http://trac.webkit.org/changeset/135911> All reviewed patches have been landed. Closing bug. |