putImageData allocates twice as much memory as it ought to

RESOLVED FIXED in Firefox 54

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

({perf})

Trunk
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment, 1 obsolete attachment)

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.
Flags: needinfo?(milan)
Keywords: perf
Priority: -- → P3
Whiteboard: [gfx-noted]
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Flags: needinfo?(milan)
(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.
Flags: needinfo?(bzbarsky)
Attachment #8831410 - Flags: review?(lsalzman)
Attachment #8831404 - Attachment is obsolete: true
Attachment #8831404 - Flags: review?(lsalzman)
Flags: needinfo?(bzbarsky)
Attachment #8831410 - Flags: review?(lsalzman) → review+

Comment 4

2 years ago
Pushed by bzbarsky@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0c3de6069e8e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Updated

2 years ago
Depends on: 1347147
You need to log in before you can comment on or make changes to this bug.