186103 - Transparent PNG background image is much darker than usual in today's build

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Pascal Chevrel:pascalc (PTO until April 26)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3b) Gecko/20021218
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3b) Gecko/20021218

build 2002218008 WinXP
on the page http://pascal.chevrel.free.fr the PNG image used as a background for
the menu links ( http://pascal.chevrel.free.fr/images/transNB2.png ) is
displayed much darker than it should. You can compare its color with how NS7.01
or Phoenix 0.5 render it.

I think that this is a very recent regression


Reproducible: Always

Steps to Reproduce:
1. go to the page
2. look at the background color
3. compare it with another Mozilla build

Actual Results:  
color is incorrect

Comment 1

[ArronM (:paper)]

Yes, we re-enabled GDI alpablending yesterday.  I'll dig up a PNG that goes from
0 to 100% opacity (I saw it somewhere) and compare the RGBs in Mozilla to a
paint program.. but they should be the same now

Comment 2

[ArronM (:paper)]

I know the cause of this one, and it's due to nsImageWin::DrawTile not handling
optimized 8 bit alpha images correctly.

Taking, but I may not have a fix ready until next year (gone for xmas)

Comment 3

[ArronM (:paper)]

Attachment: Fix (diff -uw)

Comment 4

[ArronM (:paper)]

Attachment: Fix (diff -u)

Comment 5

[ArronM (:paper)]

IMO, the -uw patch is much easier to read (review).

I added 8 bit alpha support to the ProgressiveDoubleBlit() function, and cleaned
up the function.

- ProgressiveDoubleBlit now checks for the 16M DDB limit that Win 95/98/ME has.
  The function used to always create a new DDB at the size of the whole area to
be tiled.  If > 16M and Win98/95/ME, it now makes a smaller DDB (as close to 16M
as possible using multiples of the imagesize) and draws it multiple times.

- Added spaces after commas, split lines > 80 cols, etc.

- Rewrote BuildTile so it doesn't call itself.  (it uses while loops now)  Moved
it next to ProgressiveDoubleBlit so it's easier to find when debugging.


This patch also has the side effect of increasing PNG tiling performance by 20%
- 40% on WinME, Win2K, WinXP, and maybe even WinNT. :)

Comment 6

[Ere Maijala (slow)]

In addition to the things mentioned in #mozilla, could you change BuildTile()
description comment and the comments inside the while loops? The rest looks ok
to me, assuming gAlphaBlend() does its job.

Comment 7

[ArronM (:paper)]

Attachment: Fix -uw

Changes discussed on IRC:

- Comment before the call to ProgressiveDoubleBlit (in DrawTile) was confusing.


- "offDCBytesPerPix = ::GetDeviceCaps(theHDC, BITSPIXEL)" is just wrong

- theHDC in the above line should really be offDC (not that it matters, since
offDC is a copy of theHDC, but logically it looks better)


I've also changed the BuildTile comments

Comment 8

[ArronM (:paper)]

Attachment: Fix -u

Comment 9

[Ere Maijala (slow)]

Comment on attachment 110690 [details] [diff] [review]
Fix -u

Two nits: Please update or remove the @update comment of BuildTile(). One thing
I missed before is that parameter TheHDC is aTheHDC in the comment. I know you
didn't do it, but consider changing the parameter name to be consistent with
the others. 

Otherwise it looks good to me. With the changes mentioned above, r=[e-mail].

Comment 10

[ArronM (:paper)]

Attachment: Fix -uw

- removed @update comments
- change TheHDC to aTheHDC in BuildTile

Also, some unrequested changes:
- BuildTile declaration in .h said "HDC SrcDestDC" instead of "HDC aTheHDC"
- "PRBool result" was no longer being used in DrawTile. Removed.

Comment 11

[ArronM (:paper)]

Attachment: Fix -u

Comment 12

[Ere Maijala (slow)]

Comment on attachment 110726 [details] [diff] [review]
Fix -u

One more nit now that you fixed the header too :) There's a comment about
@param aDS, which doesn't exist. I'd move the documentation comments from .cpp
to .h altogether. 
Anyway, with the comment fixed in .h, r=[e-mail].

Comment 13

[ArronM (:paper)]

Attachment: Fix -uw

Comment 14

[ArronM (:paper)]

Attachment: Fix -u

Comment 15

[ArronM (:paper)]

Comment on attachment 111114 [details] [diff] [review]
Fix -u

Updated to work in non 32 bit color displays.

The only changes from the last patch is changing:
// Create a bitmap of 1 plane, 32 bit color depth
tileBits = ::CreateBitmap(offDCWidth, offDCHeight, 1, 32, NULL);

to:
    // Can't use ::CreateBitmap, since the resulting HBITMAP will only be able
    // to be selected into a compatible HDC (ie. User is @ 24 bit, you can't
    // select the 32 HBITMAP into the HDC.)
    ALPHA32BITMAPINFO bmi(offDCWidth, offDCHeight);
    tileBits = ::CreateDIBSection(theHDC, (LPBITMAPINFO)&bmi, DIB_RGB_COLORS,
				  NULL, NULL, 0);


Hopefully the comment should be self explanatory

Comment 16

[Ere Maijala (slow)]

Comment on attachment 111114 [details] [diff] [review]
Fix -u

Good catch. r=[e-mail]

Comment 17

[tor]

Comment on attachment 111114 [details] [diff] [review]
Fix -u

sr=tor

Comment 18

[ArronM (:paper)]

Checked-in. Thanks ere, tor.

Comment 19

[Pascal Chevrel:pascalc (PTO until April 26)]

Hello, thanks for the patch !

As a side effect, there is a *huge* performance boost (IMO, much more than the
20% to 40% indicated in comment #5) when scrolling pages with transparent PNG
images used as tiled backgrounds for elements. I had abandonned the use of png
backgrounds on my sites because scrolling was almost impossible with them. I
think that if it is safe, this patch should be checked into the 1.0.x branch so
as to allow the next Netscape version to benefit from this.

With an Athlon1700XP/WinXP/256MB ram, mouse scrolling the page uses about 40 to
60 % CPU while phoenix0.5 and NS7.01 which do not benefit from this patch have
choppy scrolling and above 90% CPU use.

test page : http://pascal.chevrel.free.fr/faqmoz2.html

Comment 20

[ArronM (:paper)]

Glad to hear of the speed improvement, Pascal. :)
The speed improvement is totally dependant upon the video card, so everyone will
probably have different results.

Bug 187822 should also provide more speed improvements.  Using
http://mozilla.animecity.nu/IMGTest/TileTest01.html  (PNG Tiles mode), I get the
following

20030115    : 13951ms
20030116    : 10575ms -> 1.319 or ~32% speed increase
w/Bug 187822:  8012ms -> another 1.3198 or 32% speed increase from 20030116, or
                         a total of 74% speed increase from 20030116

Anyway, resolving this bug to fixed :)

Comment 21

[ArronM (:paper)]

that should read "a total of 74% speed increase from 20030115"

Comment 22

[David Tenser [:djst]]

Attachment: Problems with the recent fix

As you can clearly see in this screenshot, there are problems with the fix for
this bug. There are other problems too (some transparent png's are not rendered
it seems).

Comment 23

[ArronM (:paper)]

Attachment: Regression Fix

Normally, I'd suggest filing a new bug on this, but since I have a fix, we'll
let it slide this time.

r/srs: Patch fixes problem by not drawing beyond the area intended.
This regression can be tested by going to http://www.texturizer.net/phoenix/
and swishing a small window over top of the "English Dutch" table row until it
doesn't look right.  Windows Only.

Comment 24

[tor]

Comment on attachment 111856 [details] [diff] [review]
Regression Fix

sr=tor

Comment 25

[Ere Maijala (slow)]

Comment on attachment 111856 [details] [diff] [review]
Regression Fix

r=[e-mail]

Comment 26

[ArronM (:paper)]

regression fix checked-in.

Comment 27

[Mark Perris]

Attachment: Problems with latest fix

Moving a window over the area and out now works, however rollovers and moving
the mozilla window off screen still seem to have problems, as shown in the
testcase.

Using build 17th Jan 17:34 (2003011713)

Comment 28

[ArronM (:paper)]

2003011713 is actually the 1:00pm build.  I can't reproduce this.. try the
20030118xx build.

Comment 29

[Mark Perris]

yes, it works fine on the latest, thanks.