6061 - Changing bit depth/# of colors busts chrome, images

Status: VERIFIED FIXED

(Core Graveyard :: GFX, defect, P3)

Description

[Eli Goldberg]

* TITLE/SUMMARY
Changing bit depth/# of colors busts chrome, images

* STEPS TO REPRODUCE
0) Launch Apprunner. Ideally, go to an image-intensive page.
1) Change the system bit depth (I changed from 16-bit to 8-bit on Mac OS, and 32-
bit to 16-bit on Win32).

* RESULT
 - What happened

Chrome background turns black on Mac OS, and blue toolbar items turn grey on
Win32. Page being viewed isn't dithered (or re-dithered) for the new resolution.

 - What was expected

Like Communicator 4.5, Seamonkey should be aware of system bit depth changes. I
assume this is slated for implementation, but I don't know for sure, don't see a
bug to this effect, and am thus writing this bug report up. ;)

Please note related bug #2639.

* REGRESSION

 - Occurs On
        Mac OS Apprunner (M5 optimized build)
        Win32 Apprunner (M5 optimized build [NT 4, Service Pack 3])
        (Didn't check Linux)

 - Doesn't Occur On
        Communicator 4.6 RTM for Mac OS (99118 build)


* CONFIGURATIONS TESTED

- [Mac] Power Mac 8500/120 (233 Mhz 604e), 64 MB RAM (VM on; 1 MB of VM used),
1024x768 (Thousands of Colors), Mac OS 8.6

- [Win32] Vectra VL (233 Mhz P2), 96 MB RAM, 800x600 (True Color), NT 4.0 SP3.

- [Linux] Vectra VL (266 Mhz P2), 96 MB RAM.

Comment 1

[Eli Goldberg]

[Courtesy notice: Bug is still present in 8.16.99 AM builds; checked on Windows
NT 4.0 SP & Mac OS 8.6]

Comment 2

[Kevin McCluskey (gone)]

Movin to M11

Comment 3

[Kevin McCluskey (gone)]

Moving to M12

Comment 4

[Eli Goldberg]

As a courtesy note, on 9.23.99 AM M11 candidates:
	Mac OS: Chrome is busted upon resolution change; images redither properly.
	Windows: Chrome is not busted upon resolution change, but images are busted
during True color ---> 256 color resolution change.

Comment 5

[Kevin McCluskey (gone)]

I have added a new GUI event: NS_DISPLAYCHANGE. It is generated when the display
depth is changed. It is implemented on WIN32 as of Oct 6, 1999 3:06PM build.

The next step is to have the view system reload all images.

Patrick, reassigning to you.

Linux and Mac still need to generate the NS_DISPLAYCHANGE event.

Comment 6

[Kevin McCluskey (gone)]

*** Bug 2639 has been marked as a duplicate of this bug. ***

Comment 7

[Kevin McCluskey (gone)]

*** Bug 8545 has been marked as a duplicate of this bug. ***

Comment 8

[Simon Fraser [no longer active]]

*** Bug 29774 has been marked as a duplicate of this bug. ***

Comment 9

[Patrick C. Beard]

Reassigning all compositor bugs to kevin.

Comment 10

[Eli Goldberg]

QA Assigning back to self, unless petersen feels strongly.

Comment 11

[Kevin McCluskey (gone)]

Moving to M16

Comment 12

[Kevin McCluskey (gone)]

Moving to M17

Comment 13

[Simon Fraser [no longer active]]

See also 36470.

Comment 14

[Kevin McCluskey (gone)]

The problem on WIN32 is the nsIImages are converted to DDB's. When the depth is 
changed, we need to dump all of the existing DDB's and convert them to new DDB's 
with the current depth.

As a test I modified nsImageWin.cpp's nsImageWin :: Optimize(nsIDeviceContext* 
aContext) to set  mCanOptimize = PR_FALSE; instead of PR_TRUE.

I also added code to cause the offscreen to be reallocated to nsViewManager2.cpp

NS_IMETHODIMP nsViewManager2::DispatchEvent(nsGUIEvent *aEvent, nsEventStatus 
*aStatus)
{
	*aStatus = nsEventStatus_eIgnore;

	switch(aEvent->message)
		{

              case NS_DISPLAYCHANGED:
                  // Reset the offscreens width and height
                  // so it will be reallocated the next time it needs to
                  // draw. It needs to be reallocated because it's depth
                  // has changed.
                 *aStatus = nsEventStatus_eConsumeDoDefault;
                 mDSBounds.width = 0;
                 mDSBounds.height = 0;
      break;

This fixes the problem for the most part. I still have to go to edit preferences 
in mozilla and press OK, then expose the window to get it to refresh the Images 
properly. Editing the preferences causes a style change reflow.

This is not a long term solution since it requires storing all images as DIB's 
which probably more inefficient than storing them as DDB's.

Comment 15

[Kevin McCluskey (gone)]

In an attempt to provide a more general solution I tried destroying all of the 
frames and flushing the image cache when the display depth changed:
This should remove all of the nsIImages instances and cause them to be 
recreated with the correct depth:

NS_IMETHODIMP
PresShell::HandleEvent(nsIView         *aView,
                       nsGUIEvent*     aEvent,
                       nsEventStatus*  aEventStatus,
                       PRBool&         aHandled)
{
  void*     clientData;
  nsIFrame* frame;
  nsresult  rv = NS_OK;
  
  NS_ASSERTION(!(nsnull == aView), "null view");

  if (aEvent->message == NS_DISPLAYCHANGED) {
    nsCOMPtr<nsIImageManager> imageManager;
    nsresult result;
    imageManager = do_GetService(kImageManagerCID, &result);
    
    if (NS_FAILED(result)) {
      NS_ASSERTION(PR_FALSE, "Can not get image manager");
    } else {
      imageManager->FlushCache();
    }
    ReconstructFrames();
    aHandled = PR_TRUE;
    *aEventStatus = nsEventStatus_eConsumeDoDefault;
    return NS_OK;
  }

Unfortunately, it does not seem to fix the problem at all. The content area 
completely disappears after the depth change, and clicking the url bar causing a 
crash because the DocShell as a null window pointer. Not sure yet, why this 
happens. It should be safe to reframe.

Comment 16

[Henri Sivonen (:hsivonen)]

*** Bug 41444 has been marked as a duplicate of this bug. ***

Comment 17

[Kevin McCluskey (gone)]

This bug is marked "future" because it is not critical for RTM (Release To 
Manufacturing). If anyone believes it is critical, please explain why in
this bug. 

Comment 18

[Hixie (not reading bugmail)]

If this is not fixed by FCS, we must mention it in the release notes...

Comment 19

[Simon Fraser [no longer active]]

Note: dependent bug 36470 is nsbeta2+, so perhaps this should be too?

Comment 20

[Kevin McCluskey (gone)]

Bug 36470 should not be dependant on this bug.

This bug is concerned with what do we do when a NS_DISPLAYDEPTH_CHANGED event is 
generated as the result of the user changing the display depth dynamically.

The solution is to destroy all of the images at the old depth and create new 
ones at the new depth, which will be very inefficent, but that OK because it 
seldom happens, and it happens in response to a user action.

Bug 36470 is the result of using multiple monitors on the Mac. It is 
probably related to palette problems, but fixing this bug will have no effect on 
bug 36470 because NS_DISPLAYDEPTH_CHANGED will never be generated.

By the way Navigator 4.x does not handle display depth changes gracefully 
either.

In my opion this should be nsBeta2-.

Comment 21

[Jay Patel [:jay]]

Putting on [nsbeta2-] radar. Not critical to beta2.  Adding "nsbeta3" keyword 
for consideration of a fix for that milestone. 

Comment 22

[Kevin McCluskey (gone)]

Adding correctness keyword

Comment 23

[Kevin McCluskey (gone)]

Work around is to restart Mozilla after display depth change.

Comment 24

[Kevin McCluskey (gone)]

Marking nsbeta3-

Comment 25

[Matthew T (active 1999-2002)]

I'll take QA if no-one objects, since Eli is elsewhere.

4xp, because 4.x does *so* handle depth changes gracefully; also added 
relnoteRTM.

Comment 26

[pnunn]

*** Bug 56425 has been marked as a duplicate of this bug. ***

Comment 27

[ajbu]

Using the code from bug 6061 (on 6061 it will fix for Macintosh system changes 
only) and mixing it with Kevin's attempt to use the NS_DISPLAYCHANGED handler 
seems to work resetting the chrome/images:


include "nsIChromeRegistry.h"
...
NS_IMETHODIMP
PresShell::HandleEvent(nsIView         *aView,
                       nsGUIEvent*     aEvent,
                       nsEventStatus*  aEventStatus,
                       PRBool&         aHandled)
{
  void*     clientData;
  nsIFrame* frame;
  nsresult  rv = NS_OK;
  
  NS_ASSERTION(!(nsnull == aView), "null view");

  if (aEvent->message == NS_DISPLAYCHANGED) {

    nsCOMPtr<nsIChromeRegistry> 
        chromeReg(do_GetService("@mozilla.org/chrome/chrome-registry;1"));
    if (!chromeReg) {
        throw NS_ERROR_FAILURE;
    } else {
        chromeReg->RefreshSkins();
    }
  }
   

Comment 28

[Oliver Klee]

*** Bug 85062 has been marked as a duplicate of this bug. ***

Comment 29

[Kevin McCluskey (gone)]

Clearing the image cache to refresh images in the content area of the page:

From pavlov:

#include "imgICache.h"

nsCOMPtr<imgICache> imgCache(do_GetService("@mozilla.org/image/cache;1"));
imgCache->ClearCache(TRUE); // chrome images
imgCache->ClearCache(FALSE); // everything 'cept chrome images

This won't effect images currently on the screen though.  You would need to walk
through all of the <html:img> and <xul:image> elements and force them to reload
the image. 

Comment 30

[Simon Fraser [no longer active]]

What about the view manager's backing store?

Comment 31

[Kevin McCluskey (gone)]

Yes that would have to be reallocated as well as any other offscreen images.

Added topembed keyword.

Comment 32

[Marek Z. Jeziorek]

raising severity per embedding customer request

Comment 33

[Simon Fraser [no longer active]]

Note that we'll have to hook up an Apple Event to get notified of display manager 
changes on Mac OS.

Comment 34

[Kevin McCluskey (gone)]

Attachment: fix bug by forcing the backbuffer to be re-allocated

Comment 35

[Kevin McCluskey (gone)]

Simply forcing the backbuffer to be reallocated fixes the problem on WIN32. 

I tested switching back and forth between 256 (Indexed color), true color, and
16bit color and it works fine with this patch.

The new imagelib seems to have taken care of the other issues I was seeing back
on 5/24/2000.  The new imagelib keeps all images as 32bit and coverts when drawing.

The backbuffer needed to be reallocated because all images are blitted into it
before going to the screen. If the backbuffer has a smaller depth then the
screen (Which was the case when going from 256 (Indexed color) to true color)
color information was lost when the images were blitted to the backbuffer.

Comment 36

[Simon Fraser [no longer active]]

So how do we get the NS_DISPLAYCHANGED notification to the view manager on other 
platforms?

Comment 37

[Kevin McCluskey (gone)]

n Mac it we should add a method to nsWindow which is called when the display has
changed:

PRBool nsWindow::ReportDisplayChange()
{
	// nsEvent
	nsGUIEvent changeEvent;
	changeEvent.eventStructType = NS_GUI_EVENT;
	changeEvent.message = NS_DISPLAYCHANGED;
	changeEvent.time = PR_IntervalNow();

	// nsGUIEvent
	changeEvent.widget		= this;
	changeEvent.nativeMsg		= nsnull;

	// dispatch event
	return (DispatchWindowEvent(changeEvent));
}

The call to DisplayWindowEvent will take care of getting the message to the
viewmanager.

Comment 38

[Kevin McCluskey (gone)]

GTK needs the same method. It also has a DispatchWindowEvent method which
already passes the XP events up to the viewmanager.

Comment 39

[Kevin McCluskey (gone)]

I split the Mac and GTK work to generate the NS_DISPLAYCHANGE into separate bugs:

Mac  bug 101843
GTK  bug 101844

Comment 40

[karnaze (gone)]

Comment on attachment 50978 [details] [diff] [review]
fix bug by forcing the backbuffer to be re-allocated

r=karnaze

Comment 41

[ramiro]

Removing myself from cc.

Comment 42

[Jaime Rodriguez, Jr.]

Kevin should we nsbranch+ this one? It looks like an old, but interesting bug.

Comment 43

[Kevin McCluskey (gone)]

Marking nsbranch+

Comment 44

[Marc Attinasi]

Comment on attachment 50978 [details] [diff] [review]
fix bug by forcing the backbuffer to be re-allocated

sr=attinasi

Comment 45

[Simon Fraser [no longer active]]

Wait a hookey minute. This shouldn't go in until we've got fixes for all 
platforms.

Comment 46

[Marek Z. Jeziorek]

This bug does not to be done in eMojo timeframe (but it's needed soon by our
embedding customer).

Comment 47

[Kevin McCluskey (gone)]

"Wait a hookey minute. This shouldn't go in until we've got fixes for all 
platforms."

The patch covers the XP portion of fixing the problem. The platform specific
work is done under separate bugs and it will be easier to develop the platform
specific work if the XP portion is already checked in.

It just happens that the WIN32 platform specific code is already in the tree so
fixing the XP portion fixes it for WIN32.

Comment 48

[Kevin McCluskey (gone)]

Note: The bug does NOT apply to Linux. It isn't possible to dynamically change
the display depth. I closed out bug 101844 as invalid.

bug 101843 covers the Mac platform specific work.

Comment 49

[Jaime Rodriguez, Jr.]

check it in - PDT+

Comment 50

[Kevin McCluskey (gone)]

checked into the trunk and MOZILLA_0_9_4_BRANCH.

Comment 51

[timeless]

i wonder if it could apply to QtEmbedded

Comment 52

[lchiang]

wow, an old bug.

mpt - you're the qa contact; will you be able to verify this bug?  If not,
please reassign qa contact to me.  Thanks.

Comment 53

[Matthew T (active 1999-2002)]

I can't verify this since it doesn't fix the originally reported user-visible
bug. Changing QA.

Comment 54

[Kevin McCluskey (gone)]

The reporter indicated both Mac and WIN32 were failing. I fixed it on WIN32 and
filed a separate bug for the Mac (bug 101843). We should be able verify this bug
is fixed on WIN32. 

Comment 55

[Terri Preston]

Verified fixed win xp 2001111503 and win 2k build 2001111403