Closed Bug 135226 Opened 22 years ago Closed 22 years ago

256 color palettes should be selected as background palettes not as foreground palettes

Categories

(Core Graveyard :: GFX, defect, P1)

x86
Windows 2000
defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.0

People

(Reporter: kmcclusk, Assigned: saari)

References

Details

(Keywords: hang, topembed+, Whiteboard: [adt2 rtm])

Attachments

(4 files, 3 obsolete files)

taken from http://bugscape.mcom.com/show_bug.cgi?id=12932

"Gecko" currently creates a custom palette and selects it as the foreground
palette. This causes two problems:

1. If an embedding application has a foreground palette it will get into a race
condition with "Gecko" where they are both trying to select the foreground
palette resulting if flickering and hanging the application.

2. When a separate application with a foreground palette is activated the colors
in the "Gecko" browser window will be "messed up" because its palette has been
swapped out.
topembed+
Keywords: topembed+
Target Milestone: --- → mozilla1.0
Severity: normal → critical
Keywords: hang
Priority: -- → P1
Comment on attachment 77609 [details] [diff] [review]
Feel the love! The love that is in 0.9.4 and now 0.9.9, and soon 1.0 so I don't have to keep giving the love!

carrying over an sr=waterson per aim conversation w/ saari.
Attachment #77609 - Flags: superreview+
Keywords: adt1.0.0, mozilla1.0
This needs a review before it can adt1.0.0. Pls add adt1.0.0, once the r= is
added. Also, would you pls characterize this one as a ADT1 or ADT2?
Keywords: adt1.0.0
rock on, Chris!
The only file for which the patch failed was for nsImageWin.cpp - so i'd like
to get an r= for that part...just to make sure i've got it right.
Chris's comments from an email msg. regarding the orig patch:

The last patch is the one you want, that is what is in 0.9.4 tried true 
and tested.

What it is doing is ripping out our custom palette on win32, and using 
the os to create a halftone palette. That OS palette happens to be the 
same as the web safe palette that we had, but the colors are in a 
different order, and GDI has set it up such that the win32 blit calls 
can properly dither the colors on 256 color systems provided we set the 
blitting mode to halftone.

In summary, it is getting rid of broken custom palette code and 
defferring to the OS. I know IE also works this way based on some 
digging in MSDN.

Attachment #77501 - Flags: review+
Comment on attachment 77609 [details] [diff] [review]
Feel the love! The love that is in 0.9.4 and now 0.9.9, and soon 1.0 so I don't have to keep giving the love!

Looks good overall.  One concern:  My reading of SetStretchBltMode indicates a
SetBrushOrgEx is required afterwards when doing HALFTONE when using brushes. 
Possible opt:  You could use the return value of SetStretchBltMode and avoid
the calls to GetStretchBltMode in nsImageWin.cpp
Attachment #77609 - Flags: needs-work+
Comment on attachment 79288 [details] [diff] [review]
Updated patch for Mozilla 1.0 branch and the trunk...

Looks good overall.  One concern:  My reading of SetStretchBltMode indicates a
SetBrushOrgEx is required afterwards when doing HALFTONE when using brushes
(hmm, but we aren't).  Possible opt:  You could use the return value of
SetStretchBltMode and avoid the calls to GetStretchBltMode in nsImageWin.cpp
Attachment #79288 - Flags: needs-work+
Not sure what SetBrushOrgEx will buy us, beyond setting the brush orgin (duh).
Given that the MSDN samples all use it, I'm inclined to say we should too,
although I cannot point to an exact benefit.
Comment on attachment 77501 [details] [diff] [review]
Select the 256 color palette in the background instead of the foreground

r=blythe (forgot to append this comment when reviewing this particular patch
earlier).
Need an SR....
Comment on attachment 77609 [details] [diff] [review]
Feel the love! The love that is in 0.9.4 and now 0.9.9, and soon 1.0 so I don't have to keep giving the love!

obsoleting this since we now have the Mozilla 1.0 patch
Attachment #77609 - Attachment is obsolete: true
Chris : I'm trying to "obsolete" some of the patches which are no longer valid
per Jud's request.

The changes to nsWindow.cpp in the patch
http://bugzilla.mozilla.org/attachment.cgi?id=77501&action=view

are not in http://bugzilla.mozilla.org/attachment.cgi?id=79288&action=view
(which was based on attachment #77609 [details] [diff] [review])

1. Is the change to nsWindow.cpp needed?
2. If yes, i'll make that change to nsWindow.cpp and resubmit a new patch
3. If not, http://bugzilla.mozilla.org/attachment.cgi?id=79288&action=view is
our patch and i'll obsolete the other patches.
Yes, we want that change in nsWindow too
Comment on attachment 79288 [details] [diff] [review]
Updated patch for Mozilla 1.0 branch and the trunk...

r=blythe

reversal of previous "needs-work" as can not find explaination of why
SetBrushOrgEx needs to be invoked.  One could still reduce the number of win32
api calls by using the return value of SetStretchBltMode and not call
GetStretchBltMode.
Attachment #79288 - Flags: needs-work+ → review+
Comment on attachment 77501 [details] [diff] [review]
Select the 256 color palette in the background instead of the foreground

Obsoleting this patch - since the changes we need from this are covered in the
current Mozilla 1.0 branch patch and the nsWindow.cpp change will be submitted
as a separate patch.
Attachment #77501 - Attachment is obsolete: true
Comment on attachment 77501 [details] [diff] [review]
Select the 256 color palette in the background instead of the foreground

Obsoleting this patch - since the changes we need from this are covered in the
current Mozilla 1.0 branch patch and the nsWindow.cpp change will be submitted
as a separate patch.
Attachment #79469 - Attachment is obsolete: true
Comment on attachment 79472 [details] [diff] [review]
The correct patch with  the  nsWindow.cpp change

Carrying over Garrett's earlier review

r=garrett

So, the correct patches to sr= are:

http://bugzilla.mozilla.org/attachment.cgi?id=79288&action=view
http://bugzilla.mozilla.org/attachment.cgi?id=79472&action=view

Can i get an sr= pls?
Attachment #79472 - Flags: review+
Comment on attachment 79472 [details] [diff] [review]
The correct patch with  the  nsWindow.cpp change

sr=waterson
Attachment #79472 - Flags: superreview+
Comment on attachment 79288 [details] [diff] [review]
Updated patch for Mozilla 1.0 branch and the trunk...

sr=waterson
Attachment #79288 - Flags: superreview+
Adding adt1.0.0 kwd
Keywords: adt1.0.0
Please land this on the trunk and after it's been tested, update this bug with
the results.
Attachment #79472 - Flags: approval+
Patches have landed on trunk per ADT approval.

mdunn : Can someone in your team test this on the next available trunk
build...thanks
saari/anyone : Can you please post a test case for QA (or me) to use when
testing this patch..thanks
just run in 256 colors, if the images look dithered and not horribly quantized,
you're good. Also, there should not be flashing when running in an embedded app,
or with plugins.
Marking FIXED (on the trunk)
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Tested per saari's directions at
http://bugzilla.mozilla.org/show_bug.cgi?id=135226#c29

Images look dithered and not quantized.

mdunn : Can someone in QA take a look and note their findings in the bug..we
still need to get this into the 1.0 branch..thanks
adding adt1.0.0+ on behalf of the adt.  Please check this into the branch as
soon as possible and add the fixed1.0.0 keyword.
Keywords: adt1.0.0adt1.0.0+
Reopening. I believe this is the reason why Moz now looks even worse than before
when running under Win2K Terminal Services. Possibly also the cause of bug
138139. I'll attach an image so you'll see what I mean. I've gotten used to Moz
looking bad in ts, but this is just too horrible. In bug 88560 is an attachment
that shows how it was before.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached image The new look (bad)
This is a screen cap of it now (todays trunk build)
With the April 18th trunk build (2002-04-18-03), I noticing additional problems
with netscape page in 256 colors. Working with Saari on how to verify this further.
I'm on this
Line 448 of nsRenderingContextWin.cpp was changed from
"::SetStretchBltMode(aNewDC, COLORONCOLOR);" 
to
"::SetStretchBltMode(aNewDC, HALFTONE);"

I'm not exactly sure what this change does, but one of the bad effects it has 
is that it doubles the CPU required to show .gifs.  Take a look at 
http://dormcam.mine.nu:8080/orbitz.gif
Before the patch, CPU = 20% (Build 2002041603)
After the patch, CPU = 40% (Build 2002041710)
System: Win2k PIII 933
Bug 138404 appears to be the result of this fix as well.

After changing that same line of code int Comment #37 back, the colors are 
correct.
Blocks: 138404
We need to resolve this, what is the next step.... ? 
Saari please comment as appropriate.
I finally found the change that is giving my patches indigestion this morning at
3am. It is the image optimization stuff that Pavlov added in the 0.9.9 cycle. I
can fix it for 256 colors only with a pretty simple hack, but I'd at least try
to fix it the right way as it is a memory footprint win that Pavlov was going
after. 

Basically our DDBs are not playing nice in 256 color depth.
Comment on attachment 80686 [details] [diff] [review]
workaround patch for the DDB issue. It works, but it isn't ideal. Pav isn't of help, trying to contact dcone for any better ideas.

sr=jag.
Attachment #80686 - Flags: superreview+
worked around on 1.0 branch so should be good for pull tomorrow morning. A
similar workaround should go in trunk until/unless a better solution can be
found. That would correct the nasty 256 color regression on trunk.

I know I didn't follow the process. Time limitations, the desire to not stay
until 3am again and being certain of what I'm doing prompted an adapation ;-)
Confirming that Saari's checked-in patch (which happens to not be any of the
patches listed in this bug) does not cause CPU-doubling of animated .gif's
problem, as well as images are not slightly off color as in Bug 138404.  
Build ID: 2002042406 Mozilla 1.0 Branch.

Looking forward to seeing a similar patch checked-in for the trunk.  Even if
it's temporary, it will at least prevent some dup bugs being filed.
So this patch was checked in without r= or a= and it caused performance
regressions?  Sounds like it needs to be backed out.
No, Attachment 79228 [details] [diff] ("Updated patch for Mozilla 1.0 branch and the trunk...")
and Attachment 74972 [details] [diff] (nsWindow change), with reviews and approval were checked
into the trunk, and they caused performance regressions.

The patch checked into the branch is one that is not listed here, but has none
of the problems exibited with the previous patches in the trunk.   What we
really need now is the trunk updated to what the 1.0 branch is.
Should this be fixed1.0.0, since the right stuff seems to be on the 1.0 branch?
Whiteboard: [adt2]
Tsting see failures again on Win98 and ME w/saari patch
Anyone with a 98/ME box they want to lend me to test on, speak up
saari --- We have a lab setup w/ME, 98, Win2K. Get w/Brent or Jen.
Sarri- I have a Windows Me machine to verify on too. Is this checked into
today's branch build ?
no not checked in yet, verifying first. I've installed 98 on a spare machine to
test with
fixed under another bug
Status: REOPENED → RESOLVED
Closed: 22 years ago22 years ago
Resolution: --- → FIXED
If this was fixed on the branch by another bug, could you add the fixed1.0.0
keyword and mention the other bug?
changing to adt1.0.1+ for checkin to the 1.0 branch for the Mozilla1.0.1
milestone.  Please get drivers approval before checking in.
Keywords: adt1.0.0+adt1.0.1+
Keywords: mozilla1.0mozilla1.0.1
Whiteboard: [adt2] → [adt2 rtm]
Keywords: mozilla1.0.1fixed1.0.1
The status of this bug is confusing.  Is it fixed on branch?  Was it fixed by
another checkin?
Should be fixed everywhere
Verified in 2002-07-16-08 1.0.0 branch under Windows ME.
Keywords: verified1.0.1
I'm getting different results in the trunk build (2002-07-17-08) when in 256
colors. Toolbar icons and scrollbar arrow buttons appear a black background with
the trunk. I'm not able to reproduce this in the branch build.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: