Bug 65587 – Implemented skia support for caching resizes of cropped images.

• Component: New Bugs
• Status: RESOLVED FIXED

[John Bates]

Implemented skia support for caching resizes of cropped images.

[John Bates]

Created attachment 102727 [details]
Patch

[WebKit Review Bot]

Comment on attachment 102727 [details]
Patch

Attachment 102727 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9287870

[John Bates]

Comment on attachment 102727 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:-167
> -    // First get the subset we need. This is efficient and does not copy pixels.
> -    SkBitmap subset;
> -    bitmap.extractSubset(&subset, srcIRect);

Moved to NativeImageSkia::resizedBitmap that is specialized to handle subset (cropped) image resizes.

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:-195
> -    if (srcIsFull && bitmap.hasResizedBitmap(destRectTransformedRounded.width(), destRectTransformedRounded.height())) {
> -        // Yay, this bitmap frame already has a resized version.
> -        SkBitmap resampled = bitmap.resizedBitmap(destRectTransformedRounded.width(), destRectTransformedRounded.height());
> -        canvas.drawBitmapRect(resampled, 0, destRect, &paint);
> -        return;
> -    }

Unless destRectTransformedRounded == resizedImageRect, this code was never triggered, because hasResizedBitmap was keyed on the dimensions passed to shouldCacheResampling below. Since that used to be resizedImageRect, and this code is asking for destRectTransformedRounded, it would always be false. Actually, it looks like this code would ping-pong m_lastRequestSize between resizedImageRect and destRectTransformedRounded, causing the count-to-4-caching logic to never get triggered unless destRectTransformedRounded == resizedImageRect.

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:-203
> -    // visible portion for use below.
> -    SkRect destBitmapSubsetSk;
> -    ClipRectToCanvas(canvas, destRect, &destBitmapSubsetSk);
> -    SkRect destBitmapSubsetTransformed;
> -    canvas.getTotalMatrix().mapRect(&destBitmapSubsetTransformed, destBitmapSubsetSk);

Moved inside the else below where it was used.

> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:-73
> -    if (m_lastRequestSize.width() == w && m_lastRequestSize.height() == h)
> -        m_resizeRequests++;
> -    else {
> -        m_lastRequestSize = IntSize(w, h);
> -        m_resizeRequests = 0;
> -    }

This logic was moved to shouldCacheResampling so that there aren't multiple methods dealing with m_resizeRequests.

> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:-78
> -// FIXME: don't cache when image is in-progress.

Fixed?

[John Bates]

Created attachment 102730 [details]
Patch

[WebKit Review Bot]

Comment on attachment 102730 [details]
Patch

Attachment 102730 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9285917

New failing tests:
svg/custom/image-small-width-height.svg

[Mike Reed]

1. Do you have metrics for how this has improved/affected our decisions for caching?

2. Do we need to store the dstRect in the cache, or just the dst-width and dst-height?

3. Going forward, I think this notion (fancy filtering and caching it) should be moved lower-down, so we can
- age such caches more globally
- not perform filter if it can be done differently on a gpu
- not perform the filter (or perform it differently) when we're printing

#3 is just a note, not a comment on this CL per-se

[John Bates]

(In reply to comment #6)
> 1. Do you have metrics for how this has improved/affected our decisions for caching?

I'm working on this. There is a clear performance issue that this has fixed (try new tab page or new youtube interface), but some of the code paths still need testing.
 
> 2. Do we need to store the dstRect in the cache, or just the dst-width and dst-height?

Not sure - the code uses all stored information AFAIK. Let me know if you have a way to optimize out some of the data.

> 3. Going forward, I think this notion (fancy filtering and caching it) should be moved lower-down, so we can
> - age such caches more globally
> - not perform filter if it can be done differently on a gpu
> - not perform the filter (or perform it differently) when we're printing
> 
> #3 is just a note, not a comment on this CL per-se

Yes, agreed. Would be very cool to have a shared cache for better memory utilization and control.

[Mike Reed]

I believe we're using all of dstRect, but I think that is overconstrained. Two destinations of the same size, but different positions on screen should be able to use the same cached image.

[John Bates]

Created attachment 103948 [details]
Patch

[John Bates]

(In reply to comment #6)
> 1. Do you have metrics for how this has improved/affected our decisions for caching?
> 

There seem to be common use cases for cropping and resizing images. Even for things like page backgrounds. In these cases, when there are animations the image gets resized every time it's drawn. Two examples I know of that trigger this are the new new tab page and the new youtube page.

> 2. Do we need to store the dstRect in the cache, or just the dst-width and dst-height?

The documentation for skia::ImageOperations::Resize seems to indicate that it uses the full dstRect. I'm not sure how to optimize anything out.

[WebKit Review Bot]

Comment on attachment 103948 [details]
Patch

Attachment 103948 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9359867

New failing tests:
svg/custom/image-small-width-height.svg

[John Bates]

Created attachment 104247 [details]
Patch

[John Bates]

This may help themed gmail scrolling perf as well.

[James Robinson]

Comment on attachment 104247 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/NativeImageSkia.h:82
> +    SkBitmap resizedBitmap(int width, int height, const SkIRect& srcSubset, const SkIRect& destSubset, bool allowCaching) const;

style note: we tend to prefer enums for parameters like this rather that bools when the callsites are likely to pass a literal.  That way callsites look like:

resizedBitmap(..., AllowCaching)

which is a lot easier to understand than

resizedBitmap(...., true)

[Darin Fisher (:fishd, Google)]

Comment on attachment 104247 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:64
> +NativeImageSkia::CachedImageInfo::CachedImageInfo()

nit: The CachedImageInfo definition should probably move to either the top
or the bottom of the file so as not to interrupt the flow of NativeImageSkia
method implementations.

> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:110
> +

it seems like you should rewrite this function to avoid duplicating the call
to ImageOperations::Resize.

since SkBitmap is internally reference counted, this should be cheap to do:

  SkBitmap result = skia::ImageOperations::Resize(*this, skia::ImageOperations::RESIZE_LANCZOS3, w, h);
  if (m_isDataComplete && allowCaching)
      m_resizedImage = result;
  return result;

> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:126
> +        return skia::ImageOperations::Resize(subset, skia::ImageOperations::RESIZE_LANCZOS3, w, h, destSubset);

same nit

> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:140
> +    return shouldCacheResamplingInternal(destWidth, destHeight, destSubsetWidth, destSubsetHeight, srcSubset, destSubset);

it seems like this function could also be implemented in terms of a call
to shouldCacheResampling(destWidth, destHeight, srcSubset, destSubset).
my assumption is that we could compute destSubset given destSubsetWidth
and destSubsetHeight.  am i missing something?

> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:148
> +    return shouldCacheResamplingInternal(destWidth, destHeight, destWidth, destHeight, srcSubset, destSubset);

are you sure you are computing the destSubsetWidth and destSubsetHeight parameters
properly?  shouldn't they be taken from destSubset?

> Source/WebCore/platform/graphics/skia/NativeImageSkia.h:112
> +

nit: nuke this new line

[John Bates]

Created attachment 104553 [details]
Patch

[John Bates]

Comment on attachment 104247 [details]
Patch

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

This latest patch considerably cleans up drawResampledBitmap and addresses feedback.

>> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:64
>> +NativeImageSkia::CachedImageInfo::CachedImageInfo()
> 
> nit: The CachedImageInfo definition should probably move to either the top
> or the bottom of the file so as not to interrupt the flow of NativeImageSkia
> method implementations.

Done.

>> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:110
>> +
> 
> it seems like you should rewrite this function to avoid duplicating the call
> to ImageOperations::Resize.
> 
> since SkBitmap is internally reference counted, this should be cheap to do:
> 
>   SkBitmap result = skia::ImageOperations::Resize(*this, skia::ImageOperations::RESIZE_LANCZOS3, w, h);
>   if (m_isDataComplete && allowCaching)
>       m_resizedImage = result;
>   return result;

Done. (I was opting for avoiding the copy, but the resize is a lot more expensive so not a big deal.)

>> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:140
>> +    return shouldCacheResamplingInternal(destWidth, destHeight, destSubsetWidth, destSubsetHeight, srcSubset, destSubset);
> 
> it seems like this function could also be implemented in terms of a call
> to shouldCacheResampling(destWidth, destHeight, srcSubset, destSubset).
> my assumption is that we could compute destSubset given destSubsetWidth
> and destSubsetHeight.  am i missing something?

I added more comments to the method definitions to help explain what they are for. The skia APIs use all this data as-is. In the cropped case, destSubsetWidth/destSubsetHeight is not used. Instead, the full destSubset rect is used to crop the *resized* version of the image. (Of course, I assume the implementation only resizes the used bits.)

>> Source/WebCore/platform/graphics/skia/NativeImageSkia.h:82
>> +    SkBitmap resizedBitmap(int width, int height, const SkIRect& srcSubset, const SkIRect& destSubset, bool allowCaching) const;
> 
> style note: we tend to prefer enums for parameters like this rather that bools when the callsites are likely to pass a literal.  That way callsites look like:
> 
> resizedBitmap(..., AllowCaching)
> 
> which is a lot easier to understand than
> 
> resizedBitmap(...., true)

I just named the variables where they are passed as parameters. Otherwise the non-literal callsite needs to do specify "allowCaching? NativeImageSkia::AllowCaching : NativeImageSkia::DisallowCaching" instead of just allowCaching, which doesn't seem like a good tradeoff.

>> Source/WebCore/platform/graphics/skia/NativeImageSkia.h:112
>> +
> 
> nit: nuke this new line

Gladly. I found some structs elsewhere in webkit that used this style. The style bots don't have an opinion, so.. nuked!

[Darin Fisher (:fishd, Google)]

Comment on attachment 104553 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:150
> +    return shouldCacheResamplingInternal(destWidth, destHeight, destWidth, destHeight, srcSubset, destSubset);

I still have the same question:

"are you sure you are computing the destSubsetWidth and destSubsetHeight parameters
properly?  shouldn't they be taken from destSubset?"

> Source/WebCore/platform/graphics/skia/NativeImageSkia.h:45
> +    // CachedImageInfo is used to uniquely identify cached or requested image

Sorry, I explained poorly.  I thought where you placed the struct in this header file
was fine.  I was pointing out that the method implementations in the .cpp file appear
in the middle of the NativeImageSkia methods.  It is helpful when things in the .cpp
file are listed in the same order as they appear in the .h file.

I would move this back down to where you had it, and then I would just move the
methods in the .cpp file to be at the bottom of the .cpp file.

> Source/WebCore/platform/graphics/skia/NativeImageSkia.h:94
> +    SkBitmap resizedBitmap(int width, int height, bool allowCaching) const;

what about jamesr's suggestion to switch allowCaching over to an enum?

[John Bates]

Comment on attachment 104553 [details]
Patch

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

>> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:150
>> +    return shouldCacheResamplingInternal(destWidth, destHeight, destWidth, destHeight, srcSubset, destSubset);
> 
> I still have the same question:
> 
> "are you sure you are computing the destSubsetWidth and destSubsetHeight parameters
> properly?  shouldn't they be taken from destSubset?"

Sorry, I misunderstood your original question. I was trying to reuse the common heuristic code, but the way I did it is confusing. I've fixed it and will upload soon.

To explain (tl;dr)
In the uncropped resize case, the whole image is cached, so the last heuristic checks if more than 1/4 of the image is used (and caches if so). This avoids caching large images when only a small portion is displayed.

In the cropped caching case, only the visible subset of the image is resized. We are displaying 100% of what we resample. So in this case I was specifying destWidth as destSubsetWidth to ensure that the 1/4 heuristic was passed.

>> Source/WebCore/platform/graphics/skia/NativeImageSkia.h:45
>> +    // CachedImageInfo is used to uniquely identify cached or requested image
> 
> Sorry, I explained poorly.  I thought where you placed the struct in this header file
> was fine.  I was pointing out that the method implementations in the .cpp file appear
> in the middle of the NativeImageSkia methods.  It is helpful when things in the .cpp
> file are listed in the same order as they appear in the .h file.
> 
> I would move this back down to where you had it, and then I would just move the
> methods in the .cpp file to be at the bottom of the .cpp file.

Ahh, I should have looked at your comment in context! Done.

>> Source/WebCore/platform/graphics/skia/NativeImageSkia.h:94
>> +    SkBitmap resizedBitmap(int width, int height, bool allowCaching) const;
> 
> what about jamesr's suggestion to switch allowCaching over to an enum?

It gets a bit messy for the non-literal caller cases. (I explained in my earlier response to jamesr's comment.) I'm OK with either, but the current code solves the problem of self-documented literals with less code.

[John Bates]

Created attachment 104611 [details]
Patch

[John Bates]

Darin and I suspected some over-complication with this code and discovered a nice simplification that just required NativeImageSkia::resizedBitmap to return the visible subset of the image in all cases. Then all the "shouldCache" logic can be internalized into NativeImageSkia. And the caller knows it can just render the returned image into the clipped destRect.

Definitely looks a lot simpler and cleaner all around.

[WebKit Review Bot]

Comment on attachment 104611 [details]
Patch

Attachment 104611 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9437623

New failing tests:
svg/custom/image-small-width-height.svg

[Darin Fisher (:fishd, Google)]

Comment on attachment 104611 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:89
> +    m_resizedImage.extractSubset(&visibleBitmap, destVisibleSubset);

i take it that extractSubset does no work if the desired subset is everything?

this is so much clearer.  thanks!

R=me

[John Bates]

Comment on attachment 104611 [details]
Patch

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

>> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:89
>> +    m_resizedImage.extractSubset(&visibleBitmap, destVisibleSubset);
> 
> i take it that extractSubset does no work if the desired subset is everything?
> 
> this is so much clearer.  thanks!
> 
> R=me

surprisingly no, extractSubset currently does not check for the subset==everything case. I was going to write the code here (just copy+ref if subset==everything), but the extractSubset implementation looks like a better place for it (and then it might speed up other uses of extractSubset). what do you think?

[Darin Fisher (:fishd, Google)]

(In reply to comment #24)
> (From update of attachment 104611 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104611&action=review
> 
> >> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:89
> >> +    m_resizedImage.extractSubset(&visibleBitmap, destVisibleSubset);
> > 
> > i take it that extractSubset does no work if the desired subset is everything?
> > 
> > this is so much clearer.  thanks!
> > 
> > R=me
> 
> surprisingly no, extractSubset currently does not check for the subset==everything case. I was going to write the code here (just copy+ref if subset==everything), but the extractSubset implementation looks like a better place for it (and then it might speed up other uses of extractSubset). what do you think?

Does extractSubset do much work?  Does it just return a view onto the existing bitmap, or does it copy and create a new bitmap of the subset size?  If the latter, then optimizing seems well worth it.

[John Bates]

(In reply to comment #25)
> (In reply to comment #24)
> > (From update of attachment 104611 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=104611&action=review
> > 
> > >> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:89
> > >> +    m_resizedImage.extractSubset(&visibleBitmap, destVisibleSubset);
> > > 
> > > i take it that extractSubset does no work if the desired subset is everything?
> > > 
> > > this is so much clearer.  thanks!
> > > 
> > > R=me
> > 
> > surprisingly no, extractSubset currently does not check for the subset==everything case. I was going to write the code here (just copy+ref if subset==everything), but the extractSubset implementation looks like a better place for it (and then it might speed up other uses of extractSubset). what do you think?
> 
> Does extractSubset do much work?  Does it just return a view onto the existing bitmap, or does it copy and create a new bitmap of the subset size?  If the latter, then optimizing seems well worth it.

The source pixels are referenced rather than copied, but it appears to loop through each row of the image so it may not be completely cheap. I think it's fine for now though since the original code did an extractSubset every time as well. It's an easy fix in skia if I'm reading the code correctly, so let's see what Mike thinks.

[John Bates]

(In reply to comment #26)
> (In reply to comment #25)
> > (In reply to comment #24)
> > > (From update of attachment 104611 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=104611&action=review
> > > 
> > > >> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:89
> > > >> +    m_resizedImage.extractSubset(&visibleBitmap, destVisibleSubset);
> > > > 
> > > > i take it that extractSubset does no work if the desired subset is everything?
> > > > 
> > > > this is so much clearer.  thanks!
> > > > 
> > > > R=me
> > > 
> > > surprisingly no, extractSubset currently does not check for the subset==everything case. I was going to write the code here (just copy+ref if subset==everything), but the extractSubset implementation looks like a better place for it (and then it might speed up other uses of extractSubset). what do you think?
> > 
> > Does extractSubset do much work?  Does it just return a view onto the existing bitmap, or does it copy and create a new bitmap of the subset size?  If the latter, then optimizing seems well worth it.
> 
> The source pixels are referenced rather than copied, but it appears to loop through each row of the image so it may not be completely cheap. I think it's fine for now though since the original code did an extractSubset every time as well. It's an easy fix in skia if I'm reading the code correctly, so let's see what Mike thinks.

Mike says extractSubset only loops through rows in a special compressed case the chrome doesn't use. Otherwise it's trivial, so I think we're good to go.

[John Bates]

Created attachment 104711 [details]
Patch

[WebKit Review Bot]

Comment on attachment 104711 [details]
Patch

Attachment 104711 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9466875

New failing tests:
svg/custom/image-small-width-height.svg

[John Bates]

Created attachment 104732 [details]
Patch

[John Bates]

Comment on attachment 104732 [details]
Patch

just added a test_expectation change for a test that's needs rebaselining.

[Brett Wilson (Google)]

This looks good as far as I can tell. It doesn't break the animation detection, right? (This is where it does a lower quality paint and then comes in later with the nicer one.)

[John Bates]

(In reply to comment #32)
> This looks good as far as I can tell. It doesn't break the animation detection, right? (This is where it does a lower quality paint and then comes in later with the nicer one.)

It works in my tests (it doesn't do the resize until the end of a CSS animation).

[WebKit Review Bot]

Comment on attachment 104732 [details]
Patch

Clearing flags on attachment: 104732

Committed r93580: <http://trac.webkit.org/changeset/93580>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.