633467 - 2 Premultiply tables both gfxUtil and nsCanvasRenderingContext2D

Status: NEW

(Core :: Graphics: Color Management, defect)

Description

[Makoto Kato [:m_kato]]

See the following source.  We allocate and create same table in 2 area.  We should merge this.
 
http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/nsCanvasRenderingContext2D.cpp?mark=469-478#470 and http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxUtils.cpp?mark=48-49#48

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Part 1 use const table instead of dyanmic allocation

Comment 2

[Makoto Kato [:m_kato]]

Attachment: Part 2 use gfxUtils in canvas

Comment 3

[Jeff Muizelaar [:jrmuizel]]

It seems like this could have a negative impact on startup. I'm not convinced that having a static lookup table is a good idea.

Comment 4

[Alfred Kayser]

One make it so that these matrices are only initialized on demand, but in gfxUtils the space is already 'static const' 'allocated' at startup.
48 static PRUint8 sUnpremultiplyTable[256*256];
49 static PRUint8 sPremultiplyTable[256*256];
50 static PRBool sTablesInitialized = PR_FALSE;

So making canvas reuse those is better than having canvas allocate its own set.
The only thing missing in the patch is the check that the matrices are initialized by calling:
118     if (!sTablesInitialized)
119         CalculateTables();

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: alternate patch

Here's an alternate approach that dynamically allocates the tables in gfxUtils.  I was motivated to do this when I saw that sPremultiplyTable and sUnpremultiplyTable are two of the largest statically allocated objects in libxul.

Hiding the tables themselves behind getters means that we don't have to do checks every access.

There's no good place to free the tables, so they will show up as a memory "leak".  I'm honestly not sure this is worth fixing; the OS will deallocate them when there are no more users (i.e. the process is exiting).  We can't do any better than that.

Future cleanups could take the (un)premultiplication loops and stick them in gfxUtils.  That would eliminate more duplication and any optimizations could take place in gfxUtils directly.

I'm not a graphics person, so the comments in gfxUtils may be dodgy.  Please advise on better ones if necessary.

Comment 6

[Justin Lebar (not reading bugmail)]

There are a number of ways you can deal with the "leak":

One is to hand out a refcounted {Un,}PremutiplyTable object, created on-demand.  You could hold onto a weak reference to this table, so that you'd re-calculate the table whenever it's needed, after it's deleted.

Or you could keep around a strong reference to this object, and call ClearOnShutdown() on that nsRefPtr.

Or you could register a shutdown observer and delete the pointers.  See the code in ClearOnShutdown.h for an example of this.

Comment 7

[Justin Lebar (not reading bugmail)]

> I was motivated to do this when I saw that sPremultiplyTable and sUnpremultiplyTable are two of the 
> largest statically allocated objects in libxul.

Not that these changes aren't virtuous in their own right, but does this have any practical effects (on startup time, or something)?  The tables have value zero initially, so I didn't think they'd matter much.

Comment 8

[Kelsey Gilbert [:jgilbert]]

The tables should only amount to 128k, though, so I wouldn't think size would be much of an issue. Static allocation also gives you the memory management for free.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 587787 [details] [diff] [review]
alternate patch

I would rather we consolidate the methods for (un)premultiplying and not expose the tables at all. Is that feasible?

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Yes please. Maybe even somewhere in mozilla::gfx?

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to Justin Lebar [:jlebar] from comment #7)
> > I was motivated to do this when I saw that sPremultiplyTable and sUnpremultiplyTable are two of the 
> > largest statically allocated objects in libxul.
> 
> Not that these changes aren't virtuous in their own right, but does this
> have any practical effects (on startup time, or something)?  The tables have
> value zero initially, so I didn't think they'd matter much.

I haven't measured.  I suppose they are (should be) allocated in .bss, so the only effect is touching pages and not any extra I/O.

(In reply to Jeff Muizelaar [:jrmuizel] from comment #9)
> I would rather we consolidate the methods for (un)premultiplying and not
> expose the tables at all. Is that feasible?

Sure, I can take a whack at that.

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #9)
> Comment on attachment 587787 [details] [diff] [review]
> alternate patch
> 
> I would rather we consolidate the methods for (un)premultiplying and not
> expose the tables at all. Is that feasible?

Some issues:

1) The canvas methods include extra increments for source and destination data as they might not be tightly packed--which the gfxUtils method assumes;
2) The canvas methods write data quite differently: e.g. their premultiplication methods always write out in big-endian RGBA format (regardless of host endianness), whereas the gfxUtils method writes host-endian ARGB;
3) It looks like source and dest don't have to be identically sized in the canvas methods, whereas they do in the gfxUtils method.

One can, of course, add parameters to deal with all this, but then some of the usefulness of consolidation goes away.  My feeling is that the consolidation isn't worth it with the added complexity of getting everything right.  WDYT?

Comment 13

[Jeff Muizelaar [:jrmuizel]]

(In reply to Nathan Froyd (:froydnj) from comment #12)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #9)
> > Comment on attachment 587787 [details] [diff] [review]
> > alternate patch
> > 
> > I would rather we consolidate the methods for (un)premultiplying and not
> > expose the tables at all. Is that feasible?
> 
> Some issues:
> 
> 1) The canvas methods include extra increments for source and destination
> data as they might not be tightly packed--which the gfxUtils method assumes;
> 2) The canvas methods write data quite differently: e.g. their
> premultiplication methods always write out in big-endian RGBA format
> (regardless of host endianness), whereas the gfxUtils method writes
> host-endian ARGB;
> 3) It looks like source and dest don't have to be identically sized in the
> canvas methods, whereas they do in the gfxUtils method.
> 
> One can, of course, add parameters to deal with all this, but then some of
> the usefulness of consolidation goes away.  My feeling is that the
> consolidation isn't worth it with the added complexity of getting everything
> right.  WDYT?

I'd suggest just moving the canvas code to gfxUtils and having (Un)PremultiplyForCanvas methods in addition to the existing ones. This way we still get these similar things near each other and we don't have to worry about having a single more complex one-size-fits-all method.

Comment 14

[Kelsey Gilbert [:jgilbert]]

For some experimental work on pixel format conversions, I am using the following two files for premultiplication:

header: http://pastebin.mozilla.org/1477450
source: http://pastebin.mozilla.org/1477451

The interface is really simple, and for same-pixel-format premultiplication, it gets ~350 megapixels per second on my machine. (For reference, ABGR->RGBA conversion is 630MP/s)

I also tested using a LUT vs. doing the math, functions using bytes vs. dwords, and even using a dword-based LUT.
Results in MP/s:
with LUT:
336 with Byte
351 with DWord

without LUT:
196 with Byte
231 with DWord

with DWord LUT:
247 with Byte
253 with DWord