Closed Bug 135226 Opened 24 years ago Closed 24 years ago

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

Categories

(Core Graveyard :: GFX, defect, P1)

Product:
Core Graveyard
Component:
GFX
Platform:
x86
Windows 2000
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla1.0

People

(Reporter: kmcclusk, Assigned: saari)

References

Details

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

Attachments

(4 files, 3 obsolete files)

Updated patch for Mozilla 1.0 branch and the trunk...
12.38 KB, patch
blythe
: review+
waterson
: superreview+
Details | Diff | Splinter Review
The correct patch with the nsWindow.cpp change
798 bytes, patch
chak
: review+
waterson
: superreview+
jud
: approval+
Details | Diff | Splinter Review
The new look (bad)
19.86 KB, image/jpeg
Details
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.
3.19 KB, patch
jag+mozilla
: superreview+
Details | Diff | Splinter Review
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: 24 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.0 → adt1.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: 24 years ago → 24 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.0 → mozilla1.0.1
Whiteboard: [adt2] → [adt2 rtm]
Keywords: mozilla1.0.1 → fixed1.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

Creator:
Created:
Updated:
Size: