Re: [webkit-dev] Canvas performance and memory usage
Resent from the proper address: On Tue, Aug 3, 2010 at 6:00 PM, Martin Robinson <martin.james.robinson@gmail.com> wrote:
I notice that Qt added imageForRendering() and felt they could not use image() for some reason. I'd be curious if a Qt expert could weigh in on that, since maybe with a redesign a separate call would not be needed.
I'm not a Qt expert, but just based on a quick look, it seems that imageForRendering avoids the full QPixmap copy. Christophe, when you open a bug for this issue, please CC me, as I have a small patch in my tree which has the same imageForRendering pecialization, but for Cairo. Martin
On 08/04/2010 03:55 AM, ext Martin Robinson wrote:
Resent from the proper address:
On Tue, Aug 3, 2010 at 6:00 PM, Martin Robinson <martin.james.robinson@gmail.com> wrote:
I notice that Qt added imageForRendering() and felt they could not use image() for some reason. I'd be curious if a Qt expert could weigh in on that, since maybe with a redesign a separate call would not be needed.
I'm not a Qt expert, but just based on a quick look, it seems that imageForRendering avoids the full QPixmap copy.
Indeed. I added it because while QPixmap is copy-on-write it will always do a deep copy when painting is active on the pixmap. Since we have no way to retain painter state between QPainter::end() and QPainter::begin(), we leave the painter active at all times which leads us to this situation. -Kling
I'm confused why a special method is needed though. Can't image() just avoid the full copy? Given how we use image() in WebKit, I don't think there's any reason to be concerned if image() continues to reflect the contents of the ImageBuffer. I think we should just switch to that model for the CG port also anyway, since I'm unconvinced we're truly avoiding a copy, and return an image with a custom data provider that feeds the current contents of the ImageBuffer to the image. dave (hyatt@apple.com) On Aug 3, 2010, at 8:55 PM, Martin Robinson wrote:
Resent from the proper address:
On Tue, Aug 3, 2010 at 6:00 PM, Martin Robinson <martin.james.robinson@gmail.com> wrote:
I notice that Qt added imageForRendering() and felt they could not use image() for some reason. I'd be curious if a Qt expert could weigh in on that, since maybe with a redesign a separate call would not be needed.
I'm not a Qt expert, but just based on a quick look, it seems that imageForRendering avoids the full QPixmap copy. Christophe, when you open a bug for this issue, please CC me, as I have a small patch in my tree which has the same imageForRendering pecialization, but for Cairo.
Martin _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Hi, Actually, I was questioning also the necessity of this extra buffer. I'll just give an update and some numbers. We have a very large canvas (few MB) and our updates are very frequent but very small (clip area is for example 3x3 pixels), it takes about 25ms for our system to handle the expose. Once the double buffer is removed, the expose takes less than 1ms. We applied our change to image(). When the BitmapImage is created, we now pass the m_data.m_surface after increasing its reference count through cairo_surface_reference and removed the copy. Our tests didn't detect any issue after this change. Christophe On Wed, Aug 4, 2010 at 11:07 AM, David Hyatt <hyatt@apple.com> wrote:
I'm confused why a special method is needed though. Can't image() just avoid the full copy? Given how we use image() in WebKit, I don't think there's any reason to be concerned if image() continues to reflect the contents of the ImageBuffer. I think we should just switch to that model for the CG port also anyway, since I'm unconvinced we're truly avoiding a copy, and return an image with a custom data provider that feeds the current contents of the ImageBuffer to the image.
dave (hyatt@apple.com)
On Aug 3, 2010, at 8:55 PM, Martin Robinson wrote:
Resent from the proper address:
On Tue, Aug 3, 2010 at 6:00 PM, Martin Robinson <martin.james.robinson@gmail.com> wrote:
I notice that Qt added imageForRendering() and felt they could not use image() for some reason. I'd be curious if a Qt expert could weigh in on that, since maybe with a redesign a separate call would not be needed.
I'm not a Qt expert, but just based on a quick look, it seems that imageForRendering avoids the full QPixmap copy. Christophe, when you open a bug for this issue, please CC me, as I have a small patch in my tree which has the same imageForRendering pecialization, but for Cairo.
Martin _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
The reason ImageBuffer::image() makes a copy (be it a deep copy, or CoW) is almost exclusively for the purpose of ensuring correct behaviour in the case where a canvas is drawn onto itself, eg. context = myCanvas.getContext("2d"); context.drawImage(myCanvas, 0, 0); Off hand I can think of no other case where this would be necessary, so it seems like the best solution would be to make the default behaviour for image() be to return a reference to a potentially mutable image, and to give the canvas a distinct method for getting a copied image in the case where it's actually necessary. --Oliver On Aug 4, 2010, at 2:45 PM, Christophe Public wrote:
Hi,
Actually, I was questioning also the necessity of this extra buffer. I'll just give an update and some numbers. We have a very large canvas (few MB) and our updates are very frequent but very small (clip area is for example 3x3 pixels), it takes about 25ms for our system to handle the expose. Once the double buffer is removed, the expose takes less than 1ms.
We applied our change to image(). When the BitmapImage is created, we now pass the m_data.m_surface after increasing its reference count through cairo_surface_reference and removed the copy.
Our tests didn't detect any issue after this change.
Christophe
On Wed, Aug 4, 2010 at 11:07 AM, David Hyatt
<hyatt@apple.com> wrote: I'm confused why a special method is needed though. Can't image() just avoid the full copy? Given how we use image() in WebKit, I don't think there's any reason to be concerned if image() continues to reflect the contents of the ImageBuffer. I think we should just switch to that model for the CG port also anyway, since I'm unconvinced we're truly avoiding a copy, and return an image with a custom data provider that feeds the current contents of the ImageBuffer to the image.
dave (hyatt@apple.com)
On Aug 3, 2010, at 8:55 PM, Martin Robinson wrote:
Resent from the proper address:
On Tue, Aug 3, 2010 at 6:00 PM, Martin Robinson <martin.james.robinson@gmail.com> wrote:
I notice that Qt added imageForRendering() and felt they could not use image() for some reason. I'd be curious if a Qt expert could weigh in on that, since maybe with a redesign a separate call would not be needed.
I'm not a Qt expert, but just based on a quick look, it seems that imageForRendering avoids the full QPixmap copy. Christophe, when you open a bug for this issue, please CC me, as I have a small patch in my tree which has the same imageForRendering pecialization, but for Cairo.
Martin _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Thanks for the clarification. If this is the main reason, then it seems a valuable optimization to do. Christophe On Wed, Aug 4, 2010 at 2:52 PM, Oliver Hunt <oliver@apple.com> wrote:
The reason ImageBuffer::image() makes a copy (be it a deep copy, or CoW) is almost exclusively for the purpose of ensuring correct behaviour in the case where a canvas is drawn onto itself, eg.
context = myCanvas.getContext("2d"); context.drawImage(myCanvas, 0, 0);
Off hand I can think of no other case where this would be necessary, so it seems like the best solution would be to make the default behaviour for image() be to return a reference to a potentially mutable image, and to give the canvas a distinct method for getting a copied image in the case where it's actually necessary.
--Oliver
On Aug 4, 2010, at 2:45 PM, Christophe Public wrote:
Hi,
Actually, I was questioning also the necessity of this extra buffer. I'll just give an update and some numbers. We have a very large canvas (few MB) and our updates are very frequent but very small (clip area is for example 3x3 pixels), it takes about 25ms for our system to handle the expose. Once the double buffer is removed, the expose takes less than 1ms.
We applied our change to image(). When the BitmapImage is created, we now pass the m_data.m_surface after increasing its reference count through cairo_surface_reference and removed the copy.
Our tests didn't detect any issue after this change.
Christophe
On Wed, Aug 4, 2010 at 11:07 AM, David Hyatt
<hyatt@apple.com> wrote:
I'm confused why a special method is needed though. Can't image() just avoid the full copy? Given how we use image() in WebKit, I don't think there's any reason to be concerned if image() continues to reflect the contents of the ImageBuffer. I think we should just switch to that model for the CG port also anyway, since I'm unconvinced we're truly avoiding a copy, and return an image with a custom data provider that feeds the current contents of the ImageBuffer to the image.
dave (hyatt@apple.com)
On Aug 3, 2010, at 8:55 PM, Martin Robinson wrote:
Resent from the proper address:
On Tue, Aug 3, 2010 at 6:00 PM, Martin Robinson <martin.james.robinson@gmail.com> wrote:
I notice that Qt added imageForRendering() and felt they could not use image() for some reason. I'd be curious if a Qt expert could weigh in on that, since maybe with a redesign a separate call would not be needed.
I'm not a Qt expert, but just based on a quick look, it seems that imageForRendering avoids the full QPixmap copy. Christophe, when you open a bug for this issue, please CC me, as I have a small patch in my tree which has the same imageForRendering pecialization, but for Cairo.
Martin _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
On Wed, Aug 4, 2010 at 3:23 PM, Christophe Public <christophe.public@gmail.com> wrote:
Thanks for the clarification. If this is the main reason, then it seems a valuable optimization to do.
Here's the bug for this issue: https://bugs.webkit.org/show_bug.cgi?id=43507
There are other cases as well where you want a copy. Patterns are another example. For example you can create a pattern from another canvas, and I don't think it's supposed to be live if that other canvas later changes. There are examples in SVG as well where patterns are used and I am pretty sure a copy is the desired behavior. I don't think it's as simple as saying "image()" should never copy. I spent some time thinking about this from a CG perspective, and I couldn't really come up with a good solution. We use a bitmap context for efficient pixel access for get/putImageData. Any platform that wants these functions to be efficient has to keep the pixels easily obtainable. This means, for example, that we can't produce a live image using CGLayers. Looking at Qt, the only current platform to return a "live" image to canvas for rendering, it looks like they made this tradeoff, i.e., it looks to me like get/putImageData are slower than other platforms. (The get is having to call toImage() on a pixmap and then has to convert to obtain a good representation of the data for a QImage). The put has to render the changes into the pixmap. I tried renaming the two methods (image() and imageForRendering()) to copiedImage and liveImage, and ended up with about an 80/20 split at the call sites between the two. It looked really ugly to me, since in nearly every case the "liveImage" was just being used to paint the current contents of the ImageBuffer, i.e., it was just being passed to drawImage. The copiedImage was typically passed to somebody who needed to retain it for later rendering, e.g., a pattern. So to sum up, the call sites want two things: (1) To render the current contents of the image buffer efficiently. (2) To obtain a copied image that is independent of the image buffer that is typically retained by someone else. And the current implementations of image()/imageForRendering() do three things: (1) Make a copy of the image buffer into an Image (e.g., Skia). (2) Make an Image that is a live representation of the Image buffer contents (e.g., Qt using imageForRendering). (3) Make a copy-on-write image wrapper (e.g., CG). Assuming the copy-on-write stuff in CG is actually working (and I suspect that it is), then it's actually a pretty good solution. You don't copy to do throwaway drawing. Pixel access is efficient using get/PutImageData. There's no need to distinguish between copied images vs. live, with the caveat that the cached image has to be cleared (only an issue for canvas, since nobody else does continual drawing to ImageBuffers). Anyway I'm open to suggestions here. :) As for removing the ImageBuffer completely, I don't think you can get away with that. The canvas buffer's size does not necessarily match the size into which it is drawn. You need to be able to get to the image pixels of a possibly larger (or smaller) buffer. You also have to be able to draw the contents of one canvas into another canvas, so you have to be able to treat the canvas as an image. Again that means being able to get to the contents of the canvas itself as a separate buffer/image. dave (hyatt@apple.com)
Anyway I'm open to suggestions here. :)
I came to very similar conclusions that you did and posted a WIP patch here: https://bugs.webkit.org/show_bug.cgi?id=43507 . I'm not sure if it is ready or good, but in particular removing the caching behavior for ::image() led to a lot of unexpected changes. Martin
On Mon, Aug 9, 2010 at 6:17 PM, David Hyatt <hyatt@apple.com> wrote:
There are other cases as well where you want a copy. Patterns are another example. For example you can create a pattern from another canvas, and I don't think it's supposed to be live if that other canvas later changes. There are examples in SVG as well where patterns are used and I am pretty sure a copy is the desired behavior.
I don't think it's as simple as saying "image()" should never copy.
I spent some time thinking about this from a CG perspective, and I couldn't really come up with a good solution. We use a bitmap context for efficient pixel access for get/putImageData. Any platform that wants these functions to be efficient has to keep the pixels easily obtainable. This means, for example, that we can't produce a live image using CGLayers. Looking at Qt, the only current platform to return a "live" image to canvas for rendering, it looks like they made this tradeoff, i.e., it looks to me like get/putImageData are slower than other platforms. (The get is having to call toImage() on a pixmap and then has to convert to obtain a good representation of the data for a QImage). The put has to render the changes into the pixmap.
I tried renaming the two methods (image() and imageForRendering()) to copiedImage and liveImage, and ended up with about an 80/20 split at the call sites between the two. It looked really ugly to me, since in nearly every case the "liveImage" was just being used to paint the current contents of the ImageBuffer, i.e., it was just being passed to drawImage. The copiedImage was typically passed to somebody who needed to retain it for later rendering, e.g., a pattern.
So to sum up, the call sites want two things:
(1) To render the current contents of the image buffer efficiently.
Would it make sense to add a GraphicsContext::drawImageBuffer(ImageBuffer*, ...) call for this use case? Then the details of how the image is actually extracted (or not) could be kept down in the platform-specific layers. I don't know if this entirely helps the problem under discussion, but I ran into it in another context and it seemed like it would be useful. Stephen (2) To obtain a copied image that is independent of the image buffer that is
typically retained by someone else.
And the current implementations of image()/imageForRendering() do three things:
(1) Make a copy of the image buffer into an Image (e.g., Skia). (2) Make an Image that is a live representation of the Image buffer contents (e.g., Qt using imageForRendering). (3) Make a copy-on-write image wrapper (e.g., CG).
Assuming the copy-on-write stuff in CG is actually working (and I suspect that it is), then it's actually a pretty good solution. You don't copy to do throwaway drawing. Pixel access is efficient using get/PutImageData. There's no need to distinguish between copied images vs. live, with the caveat that the cached image has to be cleared (only an issue for canvas, since nobody else does continual drawing to ImageBuffers).
Anyway I'm open to suggestions here. :)
As for removing the ImageBuffer completely, I don't think you can get away with that. The canvas buffer's size does not necessarily match the size into which it is drawn. You need to be able to get to the image pixels of a possibly larger (or smaller) buffer. You also have to be able to draw the contents of one canvas into another canvas, so you have to be able to treat the canvas as an image. Again that means being able to get to the contents of the canvas itself as a separate buffer/image.
dave (hyatt@apple.com)
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
On Aug 10, 2010, at 2:38 PM, Stephen White wrote:
On Mon, Aug 9, 2010 at 6:17 PM, David Hyatt <hyatt@apple.com> wrote: There are other cases as well where you want a copy. Patterns are another example. For example you can create a pattern from another canvas, and I don't think it's supposed to be live if that other canvas later changes. There are examples in SVG as well where patterns are used and I am pretty sure a copy is the desired behavior.
I don't think it's as simple as saying "image()" should never copy.
I spent some time thinking about this from a CG perspective, and I couldn't really come up with a good solution. We use a bitmap context for efficient pixel access for get/putImageData. Any platform that wants these functions to be efficient has to keep the pixels easily obtainable. This means, for example, that we can't produce a live image using CGLayers. Looking at Qt, the only current platform to return a "live" image to canvas for rendering, it looks like they made this tradeoff, i.e., it looks to me like get/putImageData are slower than other platforms. (The get is having to call toImage() on a pixmap and then has to convert to obtain a good representation of the data for a QImage). The put has to render the changes into the pixmap.
I tried renaming the two methods (image() and imageForRendering()) to copiedImage and liveImage, and ended up with about an 80/20 split at the call sites between the two. It looked really ugly to me, since in nearly every case the "liveImage" was just being used to paint the current contents of the ImageBuffer, i.e., it was just being passed to drawImage. The copiedImage was typically passed to somebody who needed to retain it for later rendering, e.g., a pattern.
So to sum up, the call sites want two things:
(1) To render the current contents of the image buffer efficiently.
Would it make sense to add a GraphicsContext::drawImageBuffer(ImageBuffer*, ...) call for this use case? Then the details of how the image is actually extracted (or not) could be kept down in the platform-specific layers. I don't know if this entirely helps the problem under discussion, but I ran into it in another context and it seemed like it would be useful.
Yeah, I think an even better way of abstracting it might be to make ImageBuffer:drawIntoContext(GraphicsContext*, ...). I think that would be simpler for people implementing something special. If we did that, then the image() accessor on ImageBuffer could probably just always be a deep copy (or copy-on-write). Getting rid of the graphicsContext->drawImage(imageBuffer->image()....) pattern would definitely be good though. I'm just really curious about the performance of canvas and whether it's better to have slower get/PutImageData or faster rendering otherwise. It all comes down to how people are using canvas. I suspect that get/PutImageData are really really popular. dave (hyatt@apple.com)
On Aug 10, 2010, at 2:49 PM, David Hyatt wrote:
Yeah, I think an even better way of abstracting it might be to make ImageBuffer:drawIntoContext(GraphicsContext*, ...). I think that would be simpler for people implementing something special. If we did that, then the image() accessor on ImageBuffer could probably just always be a deep copy (or copy-on-write).
Getting rid of the graphicsContext->drawImage(imageBuffer->image()....) pattern would definitely be good though.
I'm just really curious about the performance of canvas and whether it's better to have slower get/PutImageData or faster rendering otherwise. It all comes down to how people are using canvas. I suspect that get/PutImageData are really really popular.
I implemented ImageBuffer::drawIntoContext on Mac, and I switched the implementation of ImageBuffer to dynamically swap to a CGLayer (throwing away the bitmap context) if it can do so. Preliminary tests look very very good. This benchmark for example: http://themaninblue.com/experiment/AnimationBenchmark/canvas/ Jumped from 37fps to 85fps. I'll need to see what happens with intensive get/PutImageData examples though before I declare victory, but it definitely looks like slowing down get/PutImageData is worth it if we can get performance gains like this! dave (hyatt@apple.com)
http://themaninblue.com/experiment/AnimationBenchmark/canvas/
Jumped from 37fps to 85fps.
Do you post the patch somewhere? Would be lovely to try this on QtWebKit... -- Ariya Hidayat http://www.linkedin.com/in/ariyahidayat
Yeah I'll get it posted soon in a bug. I need to make get/PutImageData work first. :) dave On Aug 11, 2010, at 12:35 PM, Ariya Hidayat wrote:
http://themaninblue.com/experiment/AnimationBenchmark/canvas/
Jumped from 37fps to 85fps.
Do you post the patch somewhere? Would be lovely to try this on QtWebKit...
-- Ariya Hidayat http://www.linkedin.com/in/ariyahidayat _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
The changes to ImageBuffer have landed. Here is what port authors need to know: Image* image() on ImageBuffer is gone. It has been replaced with: PassRefPtr<Image> copyImage() This function should always simply copy the image. It is used in any place where you want to get a snapshot of the ImageBuffer and not be broken if the ImageBuffer subsequently changes. For drawing, callers should now use drawImageBuffer on GraphicsContext instead of drawImage. drawImageBuffer internally just turns around and calls a draw function on ImageBuffer. clipToImageBuffer now also turns around and calls clip on the ImageBuffer. void clip(GraphicsContext*, const FloatRect&) const; // The draw method draws the contents of the buffer without copying it. void draw(GraphicsContext*, ColorSpace styleColorSpace, const FloatRect& destRect, const FloatRect& srcRect = FloatRect(0, 0, -1, -1), CompositeOperator = CompositeSourceOver, bool useLowQualityScale = false); void drawPattern(GraphicsContext*, const FloatRect& srcRect, const AffineTransform& patternTransform, const FloatPoint& phase, ColorSpace styleColorSpace, CompositeOperator, const FloatRect& destRect); I've made sure the above functions are implemented in the various ports, although the Skia and Cairo implementations currently draw by copying the image and then just calling drawImage. I've left it up to those ports to fix this issue (the current behavior is the same on those platforms as it was before my patch). There is a temporary method implemented by all ports called: bool drawsUsingCopy() const; // If the image buffer has to render using a copied image, it will return true. This method only exists right now because some of the ports are still copying. Once everyone has switched over to drawing the image buffer without making copies, then I can remove this method from the interface. HTMLCanvasElement is using it in order to cache a copy of the image for repeated drawing of a static canvas, so that the "copying" platforms don't suffer a performance degradation from my changes. Feel free to ping me on IRC or in email if you have any questions about this change. Thanks, Dave (hyatt@apple.com)
On August 16, 2010, David Hyatt wrote:
There is a temporary method implemented by all ports called:
bool drawsUsingCopy() const; // If the image buffer has to render using a copied image, it will return true.
This method only exists right now because some of the ports are still copying. Once everyone has switched over to drawing the image buffer without making copies, then I can remove this method from the interface. HTMLCanvasElement is using it in order to cache a copy of the image for repeated drawing of a static canvas, so that the "copying" platforms don't suffer a performance degradation from my changes.
The OpenVG backend (should I ever get around to spare some more time reorganizing my commits and upstreaming more of it) will always need a copying fallback, because the size of the ImageBuffer might exceed the maximum OpenVG image size. So there's a fallback code path that uses plain EGL pbuffer surfaces instead, for which there is no formal maximum size, unless it's an implementation-specific one. Cheers, Jakob
participants (8)
-
Andreas Kling
-
Ariya Hidayat
-
Christophe Public
-
David Hyatt
-
Jakob Petsovits
-
Martin Robinson
-
Oliver Hunt
-
Stephen White