61410 - Poor performance dealing with fully opaque rgb+alpha images

Status: VERIFIED FIXED

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

Description

[Tim Allen]

Whenever I access a page with a background that is a .png image, Mozilla
crashes, or is extremely slow to load (taking 10-30 minutes). This is in the
latest mozilla nightly build, and the problem disappears when the background
image is changed to a .gif.

Comment 1

[Asa Dotzler [:asa]]

Tim Allen, how about a testcase or URL where we can see this problem.
http://www.mozilla.org/quality/bug-writing-guidelines.html

Comment 2

[Tim Allen]

you can see a test URL at http://www.treehouse.org.za/without-png.html
http://www.treehouse.org.za/with-png.html is identical, except that the
background image is a PNG rather than a GIF

Thanks!

Comment 3

[Doron Rosenberg (IBM)]

what build are you using?

Comment 4

[uamjet602]

Confirmed with: Linux 2000112708

Loading the page hangs mozilla and gives a very high load to my X server

Setting severity to major.

Comment 5

[Doron Rosenberg (IBM)]

over to imagelib, as people are seeing this.

Comment 6

[Asa Dotzler [:asa]]

updating component and owner.  I see some slowness in laying out the page bug no
crash.

Comment 7

[Tim Allen]

sorry, misworded there.. I meant hang not crash. Sorry for the mistake.

Comment 8

[tenthumbs]

It turns out that bg.gif is a 5x5 image with notransparency while bg.png is a
1x3 RGBA image with a fully opaque alpha channel. It's not really fair to
compare the two.

If you convert the GIF to a 5x5 RGB PNG, then they load in equivalent times. If
you add an alpha channel to the 5x5 PNG then page loading is at least 6 times
slower. If you use the original bg.png then loading is at least 25/8 times
slower than that.

The problem seems to be in alpha processing.

Comment 9

[tor]

Two things are working against performance with small tiled RGBA images:

  * drawing composited requires readback from the framebuffer

  * we don't coalesce the tiles for alpha compositing as we do for
      opaque or binary images, so mozilla ends up roundtripping the
      server a lot.

Comment 10

[pnunn]

and that is why it makes sense to put alpha channel handling 
in the compositor.

Comment 11

[tor]

No, that's why it makes sense to use the platforms graphics capablities more
effectively.  The first problem can be solved by using the Render extension
where available, and the second was just bryner and I running out of time
before RTM.

Comment 12

[pnunn]

If you want to support and maintain more than one platform,
putting alpha in the compositor and then using the platform
goodies to speed rendering in gfx seem a better design.

Comment 13

[pnunn]

but if you want to run with it, I have cache issues as
a higher task priority. If you'd like the bug, have at it.
Be sure to touch base with kmcclusky & dcone.
-p

Comment 14

[tor]

I was planning on doing this in gfx2 for X11.  Any news regarding the status
of gfx2 (greenlighted?)?

Comment 15

[pnunn]

I've not heard any news from pav.
-p

Comment 16

[pnunn]

over to tor

Comment 17

[tor]

File: bg.png (154 bytes)
  chunk IHDR at offset 0x0000c, length 13
    1 x 3 image, 32-bit RGB+alpha, non-interlaced
  chunk gAMA at offset 0x00025, length 4: 0.45455
  chunk bKGD at offset 0x00035, length 6: red = 0 green = 0 blue = 0
  chunk pHYs at offset 0x00047, length 9: 2833x2833 pixels/meter (72 dpi)
  chunk tIME at offset 0x0005c, length 7:  2 Nov 2000 18:00:25 GMT
  chunk IDAT at offset 0x0006f, length 23
    zlib:  deflated, 32K window, default compression
  chunk IEND at offset 0x00092, length 0

Comment 18

[tor]

Attachment: optimize the fully opaque 8-bit alpha case

Comment 19

[Christopher Blizzard (:blizzard)]

r=blizzard

Comment 20

[Brendan Eich [:brendan]]

+  if ((mAlphaDepth==8) && (!mAlphaValid)) {

Dude, overparenthesized code makes me whine (also, this line makes my eyes hurt:

+      unsigned char *alpha=mAlphaBits+mAlphaRowBytes*y+mDecodedX1;

Spaces around at least the lower-precedence operator + occurrences would be more
readable IMHO).

Ok, a non-nit:

+  PRBool     mAlphaValid;
+

Use PRPackedBool and pack with other *int8 and PRPackedBool typed members such
as mFlags, mAlphaDepth, mNumBytesPixel, and mIsTopToBottom (assuming that member
is PRPackedBool, not PRBool).  Access time should not suffer on common
CPU/ISA/memory architectures, but you'll save at least two words per instance.

/be

Comment 21

[tor]

Attachment: fix nits and cleanse/shuffle nsImageGTK for fun and profit

Comment 22

[Brendan Eich [:brendan]]

Couldn't mDepth be PRUint8 and packed into the last (currently-three-byte) group
of PRPackedBool/*int8 members?

Final nit whine:
+  if ((partial) || ((mAlphaDepth == 8) && mAlphaValid)) {
has nicely unparenthesized mAlphaValid but overparenthesized partial; similar
paren-style inconsistencies appear elsewhere; and of course, mAlphaDepth == 8
need not be parenthesized against &&.  (/me flees the foolish consistency little
mind hobgoblin...)

/be

Comment 23

[tor]

Attachment: more tweaks

Comment 24

[tor]

Shuffled nsImageGTK a bit more and removed some extraneous parantheses.
If you want more removed, do it yourself.

Comment 25

[Brendan Eich [:brendan]]

Beauty, thanks -- sr=[e-mail].

/be

Comment 26

[tor]

Checked in.

Comment 27

[tor]

Fix wasn't quite right, as ImageUpdated is called before SetDecodedRect.
This means we weren't checking the right region (and in the case of an
image that only made one call to ImageUpdated, weren't checking at all).
The attached patch moves the mAlphaValid check down to SetDecodedRect.

Comment 28

[tor]

Attachment: patch fixup

Comment 29

[Christopher Blizzard (:blizzard)]

r/sr=blizzard

Comment 30

[Stuart Parmenter]

r=pavlov

Comment 31

[tor]

Checked in.

Comment 32

[tor]

Let's try actually closing the bug...

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That patch caused bustage on nebiros, for good reason.  The |return| that you
moved with the code is bad in a function that returns a value.  I'm guessing it
should just be taken out, since there's a |return NS_OK| just below, right?

Comment 34

[Terri Preston]

David Baron, should this bug still be closed or re-opened?

Comment 35

[Terri Preston]

Verified fixed linux build 2002012909