Fix double buffering for offscreen rects

[GoogleCodeExporter]

The overflow branch could not be landed because of this serious bug, which
is unrelated to issue 28.

The test case is

http://www.floodgap.com/software/classilla/bleh/scaugh.html

Here, a simple scrollframe instantiated by overflow:autio is enough, but
overflow:auto is not the only place this appears. With the overflow branch
landed, this bug goes nuts.

It is specific to OS 9's Mozilla, because OS X 1.3.1 does not show it, but
Netscape 7 on OS 9 does, along with WaZilla and WaMCom and of course
Classilla. It is also specific to Mozilla, because iCab renders this correctly.

Essentially we cannot make any major updates to the layout engine without
fixing this bug. My suspicion is this is due to this section in
widget::nsWindow. Notice that Carbon has a totally different code path.

This is not fixed by simply repainting the whole window, unlike issue 28.
When a full window repaint is triggered, say by a windowshade, blank areas
appear. This implies that the damage rect is wrong.

//
// ScrollBits
//
// ::ScrollRect() unfortunately paints the invalidated area with the 
// background pattern. This causes lots of ugly flashing and makes us look 
// pretty bad. Instead, we roll our own ::ScrollRect() by using ::CopyBits() 
// to scroll the image in the view and then set the update
// rgn appropriately so that the compositor can blit it to the screen.
//
// This will also work with system floating windows over the area that is
// scrolling.
//
// Under Carbon, this whole routine is basically moot as Apple has answered
// our prayers with ::ScrollWindowRect().
//
void
nsWindow::ScrollBits ( Rect & inRectToScroll, PRInt32 inLeftDelta, PRInt32
inTopDelta )
{                        
#if TARGET_CARBON
  ::ScrollWindowRect ( mWindowPtr, &inRectToScroll, inLeftDelta, inTopDelta, 
                        kScrollWindowInvalidate, NULL );
#else
  // Get Frame in local coords from clip rect (there might be a border
around view)

Original issue reported on code.google.com by classi...@floodgap.com on 20 Oct 2009 at 4:23

• Blocked on: #1

[GoogleCodeExporter]

The tracking bug for overflow branch is issue 66.

Original comment by classi...@floodgap.com on 20 Oct 2009 at 4:53

[GoogleCodeExporter]

overflow:autio - Is also in FF3 w/ DistroRelease: Ubuntu 8.10

https://bugs.launchpad.net/firefox/+bug/292282
https://bug463341.bugzilla.mozilla.org/attachment.cgi?id=410060

From txt of 1st link...  " I wrote this bookmarklet to disable overflow:auto 
and it
seems to eliminate the jumping:
javascript:(function(){var
alltags,i;alltags=document.getElementsByTagName('body')[0].getElementsByTagName(
'*');i=alltags.length;while(i--)alltags[i].style.overflow='hidden'})();
" -----------------------------------------
 Searching for how it's rectified in later Moz. releases - regardless of platform.
For ideas.

Original comment by a_tony_...@inbox.com on 5 Nov 2009 at 11:48

[GoogleCodeExporter]

That's jumping, however, not shearing/repaint failure.

Original comment by classi...@floodgap.com on 6 Nov 2009 at 5:44

[GoogleCodeExporter]

Back to work on this.

Some observations:

The problem is truly repainting, not scrolling. Scrolling just makes the 
repainting
failure visible. Therefore, ScrollBits is exonerated for the time being.

The problem does not manifest if the scrollframe is below a certain critical 
size. It
*can* scroll, as long as the total Y dimension of the scrollframe is below that 
size.
In practice, it seems to be around 1024 pixels tall.

Listening to Faith No More while trying to sort this out was probably not a good
idea; Mike Patton does not facilitate debugging.

Original comment by classi...@floodgap.com on 17 Jan 2010 at 6:03

[GoogleCodeExporter]

For example, if you shrink the font down with Cmd-Minus and get the logical
scrollframe size down below that critical limit, it displays and more 
importantly
repaints fine.

Original comment by classi...@floodgap.com on 17 Jan 2010 at 6:05

[GoogleCodeExporter]

Most of yesterday and this morning in the debugger yielded the following. In
nsViewManager::Refresh, when the Y of the bounding box exceeds the maximum 
screen
height, the scroll goes haywire.

// breakpoint here
  nsRect damageRectInPixels;
  aRegion->GetBoundingBox(&damageRectInPixels.x, &damageRectInPixels.y,
                          &damageRectInPixels.width, &damageRectInPixels.height);

x yyyy wwww hh
0 1000 1013 16
0 1016 1013 16
0 1032 1013 16
0 1048 1013 16
0 1064 1013 16
0 1080 1013 16 << error
0 1096 1013 16 << error

This is on a 1920x1080 display, so this seems more than a coincidence. That 
jives
nicely with the 1024 pixel estimate, and makes sense that if you shrink the
scrollframe so it all fits, you get no problems.

Original comment by classi...@floodgap.com on 18 Jan 2010 at 8:05

[GoogleCodeExporter]

Disabling double buffering fixes the bug!!

nsViewManager::Refresh

  // check if the rendering context wants double-buffering or not
  if (aContext) {
    PRBool contextWantsBackBuffer = PR_TRUE;
    aContext->UseBackbuffer(&contextWantsBackBuffer);
    if (!contextWantsBackBuffer)
      aUpdateFlags &= ~NS_VMREFRESH_DOUBLE_BUFFER;
  }

  if (1) { // PR_FALSE == mAllowDoubleBuffering) { // force double buffering off
    // Turn off double-buffering of the display
    aUpdateFlags &= ~NS_VMREFRESH_DOUBLE_BUFFER;
  }

This is not an acceptable workaround because the display is jittery as all 
get-out on
repaints, but the test case renders correctly!

Since double buffering is irrelevant on OS X, this explains why it shows up on 
OS 9
1.3.1 but not OS X Fizzilla 1.3.1. Now to figure out what the double bufferer is
doing wrong.

Original comment by classi...@floodgap.com on 18 Jan 2010 at 10:30

[GoogleCodeExporter]

NS_VMREFRESH_DOUBLE_BUFFER unsurprisingly is only in nsViewManager, and the 
only two
parts that are affected by the flag are also in ::Refresh:

    if ((aUpdateFlags & NS_VMREFRESH_DOUBLE_BUFFER) && ds) {  
      // backbuffer's (0,0) is mapped to damageRect.x, damageRect.y
      localcx->Translate(-damageRect.x, -damageRect.y);
      aRegion->Offset(-damageRectInPixels.x, -damageRectInPixels.y);
    }

[...]

    if ((aUpdateFlags & NS_VMREFRESH_DOUBLE_BUFFER) && ds) {
      // Setup the region relative to the destination's coordinates 
      aRegion->Offset(damageRectInPixels.x, damageRectInPixels.y);
      localcx->SetClipRegion(*aRegion, nsClipCombine_kReplace, result);
      localcx->Translate(damageRect.x, damageRect.y);
      localcx->SetClipRect(paintRect, nsClipCombine_kIntersect, result);
      localcx->CopyOffScreenBits(ds, 0, 0, damageRectInPixels,
NS_COPYBITS_USE_SOURCE_CLIP_REGION);
    }

Original comment by classi...@floodgap.com on 18 Jan 2010 at 10:47

[GoogleCodeExporter]

In gfxComponent, nsRenderingContextMac::CopyOffScreenBits, at

    Rect macSrcRect, macDstRect;
    ::SetRect(&macSrcRect, x, y, x + dstRect.width, y + dstRect.height);
    ::SetRect(&macDstRect, dstRect.x, dstRect.y, dstRect.x + dstRect.width, dstRect.y +
dstRect.height);
// breakpoint here

The destination rect, when scroll breaks down, has a Y >= 1080.

Original comment by classi...@floodgap.com on 18 Jan 2010 at 11:48

[GoogleCodeExporter]

However, the obvious change,

    ::SetRect(&macDstRect, dstRect.x,
        (dstRect.y >= 1080 || dstRect.y + dstRect.height > 1080) ? 1080-dstRect.height :
dstRect.y,
        dstRect.x + dstRect.width,
        (dstRect.y >= 1080 || dstRect.y + dstRect.height > 1080) ? 1080 : dstRect.y +
dstRect.height);

didn't fix the problem, although it didn't break anything else either obviously.

Original comment by classi...@floodgap.com on 18 Jan 2010 at 11:59

[GoogleCodeExporter]

But this workaround in nsViewManager::Refresh does fix it reliably and with a 
minimum
of jitter:

  nsRect damageRectInPixels;
  aRegion->GetBoundingBox(&damageRectInPixels.x, &damageRectInPixels.y,
                          &damageRectInPixels.width, &damageRectInPixels.height);

  if (damageRectInPixels.y >= 1080 || 
        damageRectInPixels.y + damageRectInPixels.height >= 1080) {
    aUpdateFlags &= ~NS_VMREFRESH_DOUBLE_BUFFER;
  }

So we need to add some code to get the actual height of the screen, and then 
I'll run
off a shlb to plug into the PB1400 to see if the assumption also holds for, 
say, 800x600.

It took a lot of careful prayer to find this. I'm just happy to have something 
to
allow me to advance further, even if it's only a hack around an actual solution 
later.

Original comment by classi...@floodgap.com on 19 Jan 2010 at 12:13

[GoogleCodeExporter]

This does not fix issue 28, but I expected that.

Original comment by classi...@floodgap.com on 19 Jan 2010 at 12:38

[GoogleCodeExporter]

Here is the code that works on both the MDD G4 and the PB1400, and accounts for
screen height. I moved the code that converts damageRect into appunits up 
because
nsIDeviceContext::GetDeviceSurfaceDimensions only works in app units, not 
pixels.

// Do this first so that we have damageRect in appunits in case we need
// Classilla issue 65 to become relevant. Won't hurt anything else.
  nsRect damageRect;
  float  p2t;
  mContext->GetDevUnitsToAppUnits(p2t);
  damageRect.x = NSToIntRound(damageRectInPixels.x * p2t);
  damageRect.y = NSToIntRound(damageRectInPixels.y * p2t);
  damageRect.width = NSToIntRound(damageRectInPixels.width * p2t);
  damageRect.height = NSToIntRound(damageRectInPixels.height * p2t);

// This works around the double buffering problem that caused
// Classilla issue 65
// Carbon doesn't appear to need it, at least on OS X, but OS 8/9 do.
#if defined(XP_MAC) && !defined(TARGET_CARBON)
/* If our damageRect goes off the screen (in practice we only check for 
vertical,
   although I suppose horizontal could do this -- I'll worry about that if people
   complain), then double buffering fails. For those damageRects we simply disable
   double buffering. This is good enough in virtually all practical cases. -- Cameron */

   /* Should this handle negative rects? */

  // Figure out how tall the screen is by asking our device context for dimensions.
  nsCOMPtr<nsIDeviceContext> dc;
  localcx->GetDeviceContext(*getter_AddRefs(dc));
  if (dc == nsnull) { // this is a big WTF. can't think where it would occur tho.
    NS_WARNING("wtf: RenderingContext has no DeviceContext");
    return;
  }
  PRInt32 dc_width, dc_height;
  dc->GetDeviceSurfaceDimensions(dc_width, dc_height); // IN APP UNITS! NOT PIXELS.
  if (dc_height > 0 && // mild paranoia: ensure valid hgt, and no part of rect outside it
        (damageRect.y >= dc_height || 
        damageRect.y + damageRect.height >= dc_height)) {
    // Fail. Remove the double buffer flag and render straight out.
    aUpdateFlags &= ~NS_VMREFRESH_DOUBLE_BUFFER;
  }
#endif
// end issue

The next step is to reinstate the overflow branch (issue 66). If that works, 
then we
can close both of these bugs and get a huge boost in layout!

Original comment by classi...@floodgap.com on 19 Jan 2010 at 2:12

[GoogleCodeExporter]

Experimentation on a few sites on the PB1400 implies I might need to account 
for the
horizontal case as well. Since the PB1400 is specifically supported under 
Classilla,
we'll need to throw something in there for that also.

In the future I might profile this to make the call for surface dimensions as
infrequently as possible and save a little processing time.

Original comment by classi...@floodgap.com on 19 Jan 2010 at 5:17

[GoogleCodeExporter]

Tested a modification above for the horizontal case. This doesn't appear to fix 
the
problem on the PB1400, particularly on wide (1024px+) pages such as Ars 
Technica's
new footer design. I will leave the horizontal case in there as it probably 
fixes
other things and doesn't seem to break anything, but overwide divs definitely 
cause a
similar problem with scroll repaints. I have to see if this responds to 
conventional
repaint events later (windowshading, etc) to see if there is a workaround on 
those
systems.

Original comment by classi...@floodgap.com on 20 Jan 2010 at 6:19

[GoogleCodeExporter]

Also, I should see (I'll do this tomorrow, I'm bushed) if disabling double 
buffering
entirely fixes the problem on the PB1400. If it does NOT, then that fractured
scrolling must be due to another problem and I will split this bug. If it DOES, 
then
we may need a kludge for a kludge, such as disabling double buffering for 
800x600
screens and hoping this doesn't annoy too many people. Experimental renderer 
actually
makes the problem worse on the 1400, but works normally on the G4.

Original comment by classi...@floodgap.com on 20 Jan 2010 at 6:31

[GoogleCodeExporter]

Finally, I should check that the reported appunit width is what I expect, to 
make
sure that the code is not buggy like the first six hundred times I wrote it and
cussed out CodeWarrior.

Original comment by classi...@floodgap.com on 20 Jan 2010 at 6:33

[GoogleCodeExporter]

More notes. I can cause the horizontal case breakdown in Classilla on my G4 by
shrinking the window, so maybe that code needs to be made relative to window 
size.
Mozilla 1.3.1 on OS X is ALSO not vulnerable to horizontal breakdown.

Original comment by classi...@floodgap.com on 20 Jan 2010 at 1:40

[GoogleCodeExporter]

The horizontal scrolling problem does NOT repair itself when double buffering 
is off,
and it DOES respond to repaints, so it is actually an instance of issue 28. I'll
tweak some optimizations into this code, and then we'll try to reinstate issue 
66.

Original comment by classi...@floodgap.com on 21 Jan 2010 at 2:45

[GoogleCodeExporter]

DING DONG THE BUG IS SERIOUSLY WOUNDED AND ON LIFE SUPPORT! It's not dead, 
because I
didn't actually fix the double buffering code. However, it is very difficult, 
even
making great effort, to notice any significant flicker on my spins round the 
Net with
the overflow branch tonight, so I am downgrading this to Medium in raptures of 
glee!

Original comment by classi...@floodgap.com on 21 Jan 2010 at 6:34

• Changed title: Fix double buffering for offscreen rects
• Added labels: Priority-Medium
• Removed labels: Priority-Critical

[GoogleCodeExporter]

As a followup, to work right on github.com, the code had to be altered to 
ALWAYS do
the height check, even if double buffering had been disabled by something else. 
This
doesn't make sense.

Original comment by classi...@floodgap.com on 27 Jan 2010 at 5:59

[GoogleCodeExporter]

Dogfood note: while the flicker is infrequent, it is still noticible on pages 
that
are affected by this problem.

Original comment by classi...@floodgap.com on 25 Feb 2010 at 6:12

[GoogleCodeExporter]

Looks like the bug is not fully beaten:

http://www.google.com/support/youtube/bin/answer.py?hl=en&answer=175292

has a small strip that is still affected by it.

Original comment by classi...@floodgap.com on 1 Mar 2010 at 1:26

[GoogleCodeExporter]

Original comment by classi...@floodgap.com on 23 May 2010 at 9:42