Implemented skia support for caching resizes of cropped images.
Created attachment 102727 [details] Patch
Comment on attachment 102727 [details] Patch Attachment 102727 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9287870
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?
Created attachment 102730 [details] Patch
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
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
(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.
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.
Created attachment 103948 [details] Patch
(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.
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
Created attachment 104247 [details] Patch
This may help themed gmail scrolling perf as well.
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)
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
Created attachment 104553 [details] Patch
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!
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?
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.
Created attachment 104611 [details] Patch
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.
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
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
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?
(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.
(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.
(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.
Created attachment 104711 [details] Patch
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
Created attachment 104732 [details] Patch
Comment on attachment 104732 [details] Patch just added a test_expectation change for a test that's needs rebaselining.
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.)
(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).
Comment on attachment 104732 [details] Patch Clearing flags on attachment: 104732 Committed r93580: <http://trac.webkit.org/changeset/93580>
All reviewed patches have been landed. Closing bug.