Bug 99784 - [chromium] Implement full-featured image cache
Summary: [chromium] Implement full-featured image cache
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hin-Chung Lam
URL:
Keywords:
: 99785 (view as bug list)
Depends on: 97347 97481 98098 99785 103354
Blocks: 99790
  Show dependency treegraph
 
Reported: 2012-10-18 18:04 PDT by Hin-Chung Lam
Modified: 2012-11-27 13:23 PST (History)
9 users (show)

See Also:


Attachments
Patch (48.28 KB, patch)
2012-11-25 22:44 PST, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch (48.29 KB, patch)
2012-11-25 22:51 PST, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch (48.25 KB, patch)
2012-11-26 13:21 PST, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch (48.42 KB, patch)
2012-11-26 15:18 PST, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch (48.39 KB, patch)
2012-11-26 21:30 PST, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch (48.60 KB, patch)
2012-11-27 12:27 PST, Hin-Chung Lam
no flags Details | Formatted Diff | Diff
Patch for landing (48.59 KB, patch)
2012-11-27 12:40 PST, Hin-Chung Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hin-Chung Lam 2012-10-18 18:04:52 PDT
This is a high level task to improve image cache in Chromium based on https://bugs.webkit.org/show_bug.cgi?id=94240.
Comment 1 Hin-Chung Lam 2012-11-25 22:01:54 PST
*** Bug 99785 has been marked as a duplicate of this bug. ***
Comment 2 Hin-Chung Lam 2012-11-25 22:44:41 PST
Created attachment 175926 [details]
Patch
Comment 3 WebKit Review Bot 2012-11-25 22:46:54 PST
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.
Comment 4 Hin-Chung Lam 2012-11-25 22:51:17 PST
Created attachment 175927 [details]
Patch
Comment 5 Hin-Chung Lam 2012-11-25 22:52:16 PST
There will be one style problem in SkSizeHash.h because SkTypes.h needs to included before SkSize.h, otherwise visual studio will fail compile.
Comment 6 WebKit Review Bot 2012-11-25 22:55:21 PST
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 7 James Robinson 2012-11-25 23:50:19 PST
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() ?
Comment 8 James Robinson 2012-11-25 23:52:07 PST
(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)
Comment 9 Hin-Chung Lam 2012-11-26 13:21:16 PST
Created attachment 176052 [details]
Patch
Comment 10 Hin-Chung Lam 2012-11-26 13:21:28 PST
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.
Comment 11 Hin-Chung Lam 2012-11-26 15:18:35 PST
Created attachment 176085 [details]
Patch
Comment 12 Hin-Chung Lam 2012-11-26 15:20:04 PST
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 13 James Robinson 2012-11-26 16:10:32 PST
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 14 Nat Duca 2012-11-26 16:44:47 PST
Comment on attachment 176085 [details]
Patch

Looks good
Comment 15 WebKit Review Bot 2012-11-26 18:07:18 PST
Comment on attachment 176085 [details]
Patch

Clearing flags on attachment: 176085

Committed r135798: <http://trac.webkit.org/changeset/135798>
Comment 16 WebKit Review Bot 2012-11-26 18:07:24 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 noel gordon 2012-11-26 20:28:39 PST
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?
Comment 18 noel gordon 2012-11-26 20:29:57 PST
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.
Comment 19 WebKit Review Bot 2012-11-26 20:58:39 PST
Re-opened since this is blocked by bug 103354
Comment 20 Hin-Chung Lam 2012-11-26 21:30:10 PST
Created attachment 176165 [details]
Patch
Comment 21 Hin-Chung Lam 2012-11-26 21:31:19 PST
Added a virtual destructor in the last upload. This should appease mac buildbot.
Comment 22 Hin-Chung Lam 2012-11-27 11:46:44 PST
jamesr: Mind take a quick look? This is a re-land with build fix.
Comment 23 Stephen White 2012-11-27 12:03:50 PST
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.
Comment 24 Hin-Chung Lam 2012-11-27 12:27:30 PST
Created attachment 176325 [details]
Patch
Comment 25 Hin-Chung Lam 2012-11-27 12:27:46 PST
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 26 Stephen White 2012-11-27 12:34:46 PST
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.
Comment 27 Hin-Chung Lam 2012-11-27 12:37:43 PST
> > 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 28 Stephen White 2012-11-27 12:38:07 PST
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.
Comment 29 Hin-Chung Lam 2012-11-27 12:40:14 PST
Created attachment 176326 [details]
Patch for landing
Comment 30 WebKit Review Bot 2012-11-27 13:23:13 PST
Comment on attachment 176326 [details]
Patch for landing

Clearing flags on attachment: 176326

Committed r135911: <http://trac.webkit.org/changeset/135911>
Comment 31 WebKit Review Bot 2012-11-27 13:23:18 PST
All reviewed patches have been landed.  Closing bug.