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)
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)
12.38 KB,
patch
|
blythe
:
review+
waterson
:
superreview+
|
Details | Diff | Splinter Review |
798 bytes,
patch
|
chak
:
review+
waterson
:
superreview+
jud
:
approval+
|
Details | Diff | Splinter Review |
19.86 KB,
image/jpeg
|
Details | |
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.
Reporter | ||
Comment 1•22 years ago
|
||
Reporter | ||
Updated•22 years ago
|
Assignee | ||
Comment 3•22 years ago
|
||
Comment 4•22 years ago
|
||
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+
Updated•22 years ago
|
Keywords: adt1.0.0,
mozilla1.0
Comment 5•22 years ago
|
||
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
Comment 7•22 years ago
|
||
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.
Comment 8•22 years ago
|
||
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.
Updated•22 years ago
|
Attachment #77501 -
Flags: review+
Comment 9•22 years ago
|
||
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 10•22 years ago
|
||
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+
Assignee | ||
Comment 11•22 years ago
|
||
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 12•22 years ago
|
||
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).
Comment 13•22 years ago
|
||
Need an SR....
Comment 14•22 years ago
|
||
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
Comment 15•22 years ago
|
||
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.
Assignee | ||
Comment 16•22 years ago
|
||
Yes, we want that change in nsWindow too
Comment 17•22 years ago
|
||
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 18•22 years ago
|
||
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 19•22 years ago
|
||
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.
Comment 20•22 years ago
|
||
Comment 21•22 years ago
|
||
Attachment #79469 -
Attachment is obsolete: true
Comment 22•22 years ago
|
||
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 23•22 years ago
|
||
Comment on attachment 79472 [details] [diff] [review] The correct patch with the nsWindow.cpp change sr=waterson
Attachment #79472 -
Flags: superreview+
Comment 24•22 years ago
|
||
Comment on attachment 79288 [details] [diff] [review] Updated patch for Mozilla 1.0 branch and the trunk... sr=waterson
Attachment #79288 -
Flags: superreview+
Comment 26•22 years ago
|
||
Please land this on the trunk and after it's been tested, update this bug with the results.
Updated•22 years ago
|
Attachment #79472 -
Flags: approval+
Comment 27•22 years ago
|
||
Patches have landed on trunk per ADT approval. mdunn : Can someone in your team test this on the next available trunk build...thanks
Comment 28•22 years ago
|
||
saari/anyone : Can you please post a test case for QA (or me) to use when testing this patch..thanks
Assignee | ||
Comment 29•22 years ago
|
||
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.
Comment 30•22 years ago
|
||
Marking FIXED (on the trunk)
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 31•22 years ago
|
||
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
Comment 32•22 years ago
|
||
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.
Comment 33•22 years ago
|
||
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 → ---
Comment 34•22 years ago
|
||
This is a screen cap of it now (todays trunk build)
Comment 35•22 years ago
|
||
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.
Assignee | ||
Comment 36•22 years ago
|
||
I'm on this
Comment 37•22 years ago
|
||
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
Comment 38•22 years ago
|
||
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.
Comment 39•22 years ago
|
||
We need to resolve this, what is the next step.... ? Saari please comment as appropriate.
Assignee | ||
Comment 40•22 years ago
|
||
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.
Assignee | ||
Comment 41•22 years ago
|
||
Comment 42•22 years ago
|
||
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+
Assignee | ||
Comment 43•22 years ago
|
||
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 ;-)
Comment 44•22 years ago
|
||
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.
Comment 45•22 years ago
|
||
So this patch was checked in without r= or a= and it caused performance regressions? Sounds like it needs to be backed out.
Comment 46•22 years ago
|
||
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.
Comment 47•22 years ago
|
||
Should this be fixed1.0.0, since the right stuff seems to be on the 1.0 branch?
Whiteboard: [adt2]
Comment 48•22 years ago
|
||
Tsting see failures again on Win98 and ME w/saari patch
Assignee | ||
Comment 49•22 years ago
|
||
Anyone with a 98/ME box they want to lend me to test on, speak up
Comment 50•22 years ago
|
||
saari --- We have a lab setup w/ME, 98, Win2K. Get w/Brent or Jen.
Comment 51•22 years ago
|
||
Sarri- I have a Windows Me machine to verify on too. Is this checked into today's branch build ?
Assignee | ||
Comment 52•22 years ago
|
||
no not checked in yet, verifying first. I've installed 98 on a spare machine to test with
Assignee | ||
Comment 53•22 years ago
|
||
fixed under another bug
Status: REOPENED → RESOLVED
Closed: 22 years ago → 22 years ago
Resolution: --- → FIXED
Comment 54•22 years ago
|
||
If this was fixed on the branch by another bug, could you add the fixed1.0.0 keyword and mention the other bug?
Comment 55•22 years ago
|
||
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.
Updated•22 years ago
|
Keywords: mozilla1.0 → mozilla1.0.1
Whiteboard: [adt2] → [adt2 rtm]
Assignee | ||
Updated•22 years ago
|
Keywords: mozilla1.0.1 → fixed1.0.1
Comment 56•22 years ago
|
||
The status of this bug is confusing. Is it fixed on branch? Was it fixed by another checkin?
Assignee | ||
Comment 57•22 years ago
|
||
Should be fixed everywhere
Comment 58•22 years ago
|
||
Verified in 2002-07-16-08 1.0.0 branch under Windows ME.
Keywords: verified1.0.1
Comment 59•22 years ago
|
||
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.
Updated•15 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•