113480 - Test10 paints over desktop

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Mike Pinkerton (not reading bugmail)]

tip build from 12/4. load Viewer test 10 (animated images). content area bg
image paints over the desktop in addition to in the bg of the window.

probably some offscreen GWorld-fu.

Comment 1

[Simon Fraser [no longer active]]

This is happening because the image tiling code in nsImageMac is not setting up 
the port, and therefore not clipping.

Comment 2

[Simon Fraser [no longer active]]

Attachment: Fixes this and some other drawing bugs

Comment 3

[Simon Fraser [no longer active]]

Attachment: Tidier patch

The real patch. Ensures that we save and restore the port and device around
some CopyBits calls, using a new utility class.

Comment 4

[Simon Fraser [no longer active]]

Reviews, please?

Comment 5

[saari (gone)]

Comment on attachment 62597 [details] [diff] [review]
Tidier patch

Any reason you moved away from the St* prefix for stack based classes? I kind
of liked that. 

r=saari

Comment 6

[Simon Fraser [no longer active]]

Not really; I can rename it.

Comment 7

[Mike Pinkerton (not reading bugmail)]

in the ctor for nsDrawingSurfacePortSetter, could you null out all the member
variables just to be safe, rather than relying on setting only the ones you know
you will need based off |mIsOffscreen|? Just a nit, but as the code evolves, it
might cause unexpected problems.

what else might this affect? plugins?

Comment 8

[Simon Fraser [no longer active]]

Damn, that patch causes some nasty port badness that hoses the machine on 9.

Comment 9

[Simon Fraser [no longer active]]

I found another cause of bad drawing issues in the Mac GFX code.
The view manager creates offscreen drawing surfaces for blending (which happens 
in test 10, for example), and one of those surfaces is selected as the current 
drawing surface. However, when the view manager is torn down on the next page 
load, the surfaces are freed, and their GWorlds disposed of. However, we fail to 
reset the current device to a valid GDevice.

Comment 10

[Simon Fraser [no longer active]]

Well, this is Hard.

The crux of the problem is that we use SetPort/SetGWorld in a bunch of places in 
the code without any coordination between them, with the result that we often end 
up with a calling pattern which leaves the current device set to a GWorld's 
device, and the port set to the window's port.

The gfx APIs do not have any hooks for pre/post drawing getting and setting; the 
general strategy seems to be that we simply set the port (via 
SelectDrawingSurface, usually) before drawing into it. However, if we've set the 
port to the offscreen GWorld backbuffer, then code in widget calls SetPort() 
using the window's port, we're hosed.

nsRenderingContextMac makes some attempt to save and restore the port on 
initialization and destruction, but those are totally bogus because rendering 
context lifetimes are not well specified, and are certainly not guaranteed to be 
nested.

The only solution I can think of currently is to have all port setting (to on-
screen or off-screen ports) go via some bottleneck, which will have to live in 
the non-component GFX dll (since both widget and gfx call it), or will have to be 
an XPCOM service. That can then have the smarts to save and restore GDevices as 
necessary. Better ideas are welcome.

Comment 11

[Simon Fraser [no longer active]]

Attachment: Spring cleaning of port maintenance code in Gfx

Comment 12

[Simon Fraser [no longer active]]

Attachment: Spring cleaning of port management in widget

Comment 13

[Simon Fraser [no longer active]]

The attachments clean up the port/gdevice maintenance in widget and gfx, and add 
some assertions that should catch bad port settings in future.

The changes consist of the following:

Various Gfx files:
* Replaced GrafPtrs with CGrafPtrs, since all ports we should deal with will be 
CGrafPtrs, Carbon APIs use CGrafPtrs, and GWorlds are CGrafPtrs. These are moste 
of the changes in:
  nsUnicodeRenderingToolkit.h
  nsUnicodeRenderingToolkit.cpp

nsATSUIUtils.cpp:
* GrafPtr->CGrafPtr  
* Also removed the GrafPtr param from StartDraw, and removed EndDraw, replacing
  with use of stack-based StPortSetter.

nsCarbonHelpers.h:
* Added GetTheWindowList() util.
* Added IsPortOffscreen() for non-Carbon
* Various GrafPtr -> CGrafPtr

nsDeviceContextMac.cpp:
* Various GrafPtr -> CGrafPtr
* Use of StPortSetter

nsDrawingSurfaceMac.cpp:
* Set the port back to a known good port in the dtor.
* Various GrafPtr -> CGrafPtr
* Tidyup, added some "Not implemented!" assertions (they don't fire).
* Use StGWorldPortSetter when erasing the GWorld

nsGfxUtils.h:
* added a new ValidateDrawingState() function that checks to see if the current
  port/device settings are reasonable. This is intended just for debug 
  assertions.

* added a nsGraphicsUtils with 3 static methods:
    SafeSetPort  --  calls SetGWorld with the supplied port, and either nsnull
      or GetMainDevice() so that the device is correctly set.
    SafeSetPortWindowPort -- as above with a WindowPtr parameter.
    SetPortToKnownGoodPort -- sets the port to that of the first window in the
      window list (if any).

  nsGraphicsUtils::SafeSetPort() should now be called instead of 
::SetPortWindowPort() everywhere.

* Fixed StPortSetter to correctly save and restore the current GDevice.

* Added a StGWorldPortSetter that should *only* be used for temporary port 
  setting to a GWorld.

nsGraphicState.h/cpp:
* Various GrafPtr -> CGrafPtr
* Save mOffx and mOffy when Initting from a CGrafPtr.

nsIDrawingSurfaceMac.h:
* GrafPtr -> CGrafPtr

nsImageMac.cpp:
* Here is an important part of the fix; we need to ensure that the current port
  is set to the destination port for drawing and tiling images.

+	if (!mImageBitsHandle)
+		return NS_ERROR_FAILURE;

This also fixes bug 112685.

nsRenderingContextMac.h/cpp:
* This class used to save and restore the port/orgin in its Init() and dtor 
methods,
  which, because lifetimes are not guaranteed to be nested, is bogus, and has 
been
  removed.

* The NS_ASSERTION(ValidateDrawingState(), "Bad drawing state"); have 
delibarately
  been left in.

* mGSStack was changed from a nsVoidArray* to an nsAutoVoidArray to reduce
  allocations.

+	// on Mac, select it too, to ensure that the port gets set correct for
+	// tiling and image drawing
+	//SelectDrawingSurface(mCurrentSurface);

Ignore this change; it will be removed.  
  
nsRenderingContextImpl.cpp:
  Initialized some member variables that need to be (rather than doing it in the 
derived class).

nsUnicodeRenderingToolkit.cpp:
* GrafPtr -> CGrafPtr
* Made some static data const.

Widget
------

nsChildWindow.h
* Bit packing, covered by bug 115141

nsDynamicMDEF.cpp
* array assertions, covered by bug 117109

nsMacEventHandler.cpp
* Use nsGraphicsUtils::SafeSetPortWindowPort rather than SetPortWindowPort().

nsMacMessagePump.cpp
* Ditto, and some drawing state assertions.

nsMacWindow.cpp
* Another important part of the change. When disposing a window, set the port to 
a
  known good port (otherwise we're left pointing to the window manager port).
  
  And, when making a new window, set the port to it immediately.
  
nsWindow.cpp
* Validate implementation, covered by bug 107828.
* Another important change: StartDraw/EndDraw now _always_ Push and Pop the 
rendering
  context state.
  
nsWindow.h, nsBaseWidget.h
* More bit packing, covered by bug 115141

Reviews, please.

Comment 14

[Mike Pinkerton (not reading bugmail)]

- looks like nsWindow::Validate() has tabs

- you should just detab all these files before landing anyway. it's time they
got the treatment ;)

looks good, r=pink.

Comment 15

[Simon Fraser [no longer active]]

beard, can you sr please?

Some notes:
* Plugins and printing continue to work fine with these changes
* Things continue to work with double-buffering off in the view manager

However, on this last point, there are some drawing artifacts that show up if we
turn off view manager double-buffering on OS X (which we should, since the OS
does its own buffering). In the screenshot to come, notice that we sometimes
draw extra stuff when updating the toolbar buttons. The button images are in 1
big GIF, and we draw sections from it for each button/state. Debugging shows
that, even though we pass correct src/dest rects to the CopyMask call, too many
pixels in width are copied, so we pick up parts of the adjacent image.

I've tried playing with the pixMap flags in nsImageMac, but cannot find a way to
fix this. Also, it doesn't always happen. So it's mysterious, but prevents us
from turning off view manager double buffering for now.

Comment 16

[Simon Fraser [no longer active]]

Attachment: Toolbar drawing gliches with double buffering off (unaffected by these patches)

Comment 17

[Simon Fraser [no longer active]]

The good news is that if I remove the unnecessary transparency of the image that
draws the toolbar buttons, then the drawing glitch goes away (it must be a
CopyMask bug).

Comment 18

[Mike Pinkerton (not reading bugmail)]

woohoo! i've been seeing these artifacts for a while on my mach-o build. i
thought i was going crazy and searched far and wide for a magic ifdef XP_UNIX
that we forgot to turn off. Turns out, i had dblbuffering off for the cocoa
stuff i was doing.

I'll stop looking for that magic ifdef now ;)

Comment 19

[Brendan Eich [:brendan]]

beard brings domain expertise, so he's like a super-r= -- but I trust pink and
figure beard can follow up as needed after checkin.  rs=[e-mail] on
the patches.

/be

Comment 20

[Simon Fraser [no longer active]]

Paches checked in.