1334749 - putImageData allocates twice as much memory as it ought to

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

And worse yet, deallocates it; I'm seeing the madvise from the large deallocations show up quite prominently in a profile I just did.  In fact, the time under putImageData breaks down about like so in this profile:

13% deallocating a cairo surface
16% deallocating a skia surface
16% copying data from a cairo surface to a skia surface
17% actually drawing the skia surface to the canvas
38% getting the data all set up (premultiplying, etc)

What it looks like we do in this code is:

1)  Create a cairo surface (a gfxImageSurface) with the size we want.
2)  Put our desired pixel data into its Data() buffer.
3)  Create Skia surface with that Data() and the cairo surface's Stride().
4)  Draw the Skia surface.

So really, the only thing we use the cairo surface for is to compute its Stride(), which we could just as easily do ourselves: it's 4*width in this case.  And creating the Skia surface copies all the data into it.

It would be much better if we did one allocation, then wrote to it.  I don't see an obvious way to just init a SourceSurfaceSkia with a given size, but maybe we can init an SkImage of a given size and construct from that, to avoid these copies or something?

Milan, I'm not quite sure who owns this code nowadays and hence whom to needinfo.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Avoid creating a temporary cairo surface, just to copy its data to a skia surface, in putImageData

Comment 2

[Lee Salzman [:lsalzman]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #1)
> Created attachment 8831404 [details] [diff] [review]
> Avoid creating a temporary cairo surface, just to copy its data to a skia
> surface, in putImageData

Almost... One extra snag: the data source surface has an alignment that is a multiple of 16 bytes, instead of just the 4 that gfxImageSurface created, so it is no longer safe to assume the next row starts where the previous row ended. To work around this, query sourceSurface->Stride() and use that to increment the destination row pointer, so that it is alignment agnostic.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: With the stride issue fixed

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c3de6069e8e
Avoid creating a temporary cairo surface, just to copy its data to a skia surface, in putImageData.  r=lsalzman

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0c3de6069e8e