194261 - [Image Tiling] Windows Platform Cleanup

Status: RESOLVED FIXED

(Core Graveyard :: Image: Painting, enhancement)

Description

[ArronM (:paper)]

Smarter Image Tiling:

- Don't create temporary tiling DC if not needed

- Don't draw full image on areas not in tile region. Presently, if we have a div
of 50x50, with a dotted border of 1 (making the total 52x52), and a image of
50x50 tiled over the div, we make an offscreen DC and blit the image 9 times..
topleft (1x1px seen), top (50x1px seen), topright (1x1px seen), left (1x50px
seen), middle (50x50px), right (1x50px), bottomleft, bottom, and bottomright.

- Stop progressively doubleblitting when 1/2 the tile area is blitted, and do
two draws to the screen.  This is always faster than p. doubleblitting the whole
image and drawing to screen once.  We used to do it the fast way before I mucked
it up in another patch.  Oops

Testcases:

Perf tests: 
http://mozilla.animecity.nu/IMGTest/TileTest01.html

Verification that tiling still works properly:
http://mozilla.animecity.nu/IMGTest/TileBorder.html

Comment 1

[ArronM (:paper)]

Attachment: Patch

ProgressiveDoubleBlit is pretty much rewritten.  For reviewers/SRs, it's
probably easier to apply the patch and review the function instead of trying to
read the diff.

Comment 2

[ArronM (:paper)]

Comment on attachment 115041 [details] [diff] [review]
Patch

ere, since you r'd one of my other tiling patches, could you r this one?

Comment #1 & #2 has all the good info on the patch.  I went through the code
and added extra comments as well, so hopefully that will make things easier.

Comment 3

[Ere Maijala (slow)]

I've yet to go through the actual functionality, but here are the notes from the
first pass:

- ((nsDrawingSurfaceWin *)aSurface)->ReleaseDC() missing from most early returns
- oldMaskBits not selected back, maskDC and maskBits not destroyed at 978
- oldBits and oldMaskBits at 1004 should be renamed to avoid (my) confusion
- oldBits and oldMaskBits not selected back, maskDC, maskBits and mTmpHBitmap
not destroyed at 1029 and 1063

And the mandatory nits: 

- in .h: "Progressivly" -> Progressively
- in .cpp: "drawn a tall" :)

With these fixed it's easier to check if there was even more :)
Now to functional check..

Comment 4

[ArronM (:paper)]

Attachment: mwhaha!! do break while(0)!

bah, too many exit points with lots of unreleased resources.
I put a "do { stuff; if (fail) break; stuff; } while (0);"  loop in there and
break instead of return.  After the "loop", cleanup takes place.

I checked every exit point twice, but that doesn't mean I haven't missed
something again, so have a go at it. :)

Also changed oldBits and oldMaskBits to oldImgBits, oldDstBits, oldImgMaskBits,
and oldDstMaskBits.

And fixed the typos.

Comment 5

[Ere Maijala (slow)]

The patch looks good to me. I'll give it r= as soon as I can test it (as soon as
I get my tree working).

Comment 6

[ArronM (:paper)]

Attachment: Patch, with Win95/98/ME 0xFF0000 limit check

Forgot about the 0xFF0000 DC memory limit for Win95/98/ME.  That's there now. 
Also made the logic better for choosing whether to just draw "up to 4 image
parts" or p. double blitting.

Comment 7

[ArronM (:paper)]

Attachment: Changes between last 2 patches

This should make it easier for Ere to review.  It contains only the changes
between the last patch and the new one.

Comment 8

[Ere Maijala (slow)]

Comment on attachment 115304 [details] [diff] [review]
Patch, with Win95/98/ME 0xFF0000 limit check

Otherwise it's good, but I stumbled on something suspicious. I run it under
CodeWatch to check for any leaks I didn't notice (there wasn't anything, btw)
but crashed at CreateDIBSection(). It was fixed by changing the ppvBits
parameter to non-NULL. This could be caused by CodeWatch instrumentation, but
MSDN doesn't state whether it can be NULL or not, so to be safe I'd change all
of them to something like this (I know it was NULL before your patch, but
still):

void *maskBitValues;
maskBits  = ::CreateDIBSection(theHDC, (LPBITMAPINFO)&bmi,
	DIB_RGB_COLORS, &maskBitValues, NULL, 0);

Comment 9

[ArronM (:paper)]

Attachment: Patch

Never had a problem with it, but there's no perf difference, so better safe
than sorry :)

There were 3 CreateDIBSection in ProgressiveDoubleBlit(..), so I did them all.

Comment 10

[Ere Maijala (slow)]

Comment on attachment 115330 [details] [diff] [review]
Patch

r=[e-mail]

Comment 11

[ArronM (:paper)]

Comment on attachment 115330 [details] [diff] [review]
Patch

roc, is this something you're willing/able to SR?


perf gain is somewhere around 30% for JPG and GIF tiling, and about 5% gain for
PNG tiling.

Comment 12

[ArronM (:paper)]

checked in

Comment 13

[ArronM (:paper)]

Perf Results:

% are in comparison to 0224 (+ means 0224 was faster, - means the other browser
was faster)

Test -->    PNG    JPG     GIF  RealPNG NoTiling
20030224   9163   7090   12738   9493   6950

20030223  10044   9012   17205  10004   6990
            10%    27%     35%     5%     1%
            
IE6        6950*   7010  72276   7001*  8061
           -24%*   -1%    467%   -26%*   16%
           
Op7       12197   6739   10175  11907   6400
            33%    -5%    -20%    25%    -8%

* = Did not draw correctly

Overall, a big win for GIF tiling from today and yesterday, a moderate win for
JPG tiling, and a small win for PNG tiling.  The only place that really needs
improving is GIF tiling, since OP7 does it much faster.

Comment 14

[(not reading, please use [e-mail] instead)]

hey Arron, I'm not asserting on every message I load in mailnews.
(see bug #194791).

can you jump in there and comment?  

I have a feeling it isn't your fault, but some bad xul or css in mailnews.