579808 - unpainted lines with MozTransform scale after scrolling

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Karl Tomlinson (:karlt)]

Attachment: screenshot

STR:

1. from <obj-path>/_tests/testing/mochitest,
   run python runtests.py --test-path=modules/plugin/test/test_bug539565-2.html
   and wait for the test to finish.

2. Scroll to the right and slowly back to the left using the horizontal
   scrollbar thumb.

This seems to be a regression from bug 564991, but reproducing is a bit temperamental (seems to depend on window size) so perhaps there ways to produce this before that bug landed.

Comment 1

[Karl Tomlinson (:karlt)]

Note that as well as unpainted lines that cover the entire embed, there are also unpainted lines that are only in the blue div.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Karl, are you still seeing this?

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: more recent screenshot

(In reply to comment #2)
> Karl, are you still seeing this?

Still seeing this with 9d6b04fbec36.
The unpainted lines now seem to show uninitialized pixels.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I can

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That test is pretty broken for me; scrolling around just fails to repaint the area where the plugin was.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

-- nsDisplayTransform::GetBounds/IsOpaque are wrong. IsOpaque returns true when the list reports as being opaque, but that doesn't mean the whole overflow area for the frame is opaque. The overflow area for the frame might not be covered by stuff in the list (even just because we had to take the union of rectangles to get the overflow area). So, compute the transform bounds as the transform of the list bounds.

-- PrintDisplayListTo needs to be more careful when printing the stored list for an nsDisplayTransform. We need to check whether the list DidComputeVisibility.

-- nsDisplayPlugin::GetBounds is wrong for plugins with layers, in two ways:
  -- When using a plugin with a layer, we need to set the size of the bounds to the size of the layer (as well as adjusting the offset to center the plugin layer within the plugin rect).
  -- If the plugin layer ImageContainer has no image, the plugin hasn't painted yet. We should leave the bounds as the size of the plugin content area instead of setting them to the layer image size (which will be 0,0 since we have no image). If we allow the size of the plugin content area to be set to 0,0, BuildLayer will not actually be called and the plugin never gets around to displaying properly.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Benjamin, you might want this patch for your Windows work.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 478632 [details] [diff] [review]
fix

hmm, the last paragraph of comment #6 may be wrong. We do need to ensure NotifyPainted is triggered, but it's not a good idea to lie about the size of what we're going to paint. Probably we should move the call to NotifyPainted to nsObjectFrame::BuildDisplayList.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

Addresses previous comment. When we have no image, we allow the plugin display item to be 0x0 (it will actually be removed during ComputeVisibility and BuildLayer will never be called). But we move the code to call NotifyPaintWaiter and NotifyPainted from BuildLayer to BuildDisplayList, so it gets called anyway.

Comment 10

[Oleg Romashin (:romaxa)]

Comment on attachment 478634 [details] [diff] [review]
fix v2

this patch breaking == 580160-1.html 580160-1-ref.html reftest

Comment 11

[Oleg Romashin (:romaxa)]

Attachment: reftests results

Comment 12

[Timothy Nikkel (:tnikkel)]

Comment on attachment 478634 [details] [diff] [review]
fix v2

r=me, of course you'll have to fix the reftest fail.

Comment 13

[Oleg Romashin (:romaxa)]

Comment on attachment 478634 [details] [diff] [review]
fix v2

r-, due to reftest failure...
btw roc why do you ask superreview for this patch?

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The 580160-1.html testcase works for me if I load it and call doTest() manually. I presume we're taking the snapshot before the plugin has resized and repainted its layer.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

My patch here actually causes a test failure in layout/reftests/transform/compound-1-ref.html (maybe in combination with other patches in my queue). The problem actually already exists and is nontrivial.

In FrameLayerBuilder in a couple of places we do
  nsIntRect itemVisibleRect =
    aItem->GetVisibleRect().ToNearestPixels(appUnitsPerDevPixel);

This code is assuming that when the item draws, whatever it draws will be snapped to pixel boundaries. But this assumption is overoptimistic for a couple of reasons:
-- Some content, such as an element transformed by rotation, simply doesn't snap its drawing.
-- If the context has a tricky transform on it when we come to draw the content, we might not snap.

Using ToOutsidePixels here would be the conservative approach and it would give correct results. However it would mean that for content that *does* snap, but is not already aligned at pixel boundaries, the visible rect is bigger than the area that actually gets drawn, so a layer containing only that content could no longer be treated as opaque. That could be bad.

In BasicLayers we have a couple of places where we do gfxUtils::ClipToRegionSnapped that have related issues.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Filed bug 615794 on that.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The 580160-1 reftest failure is an intermittent failure on trunk already. Bug 615417 has my explanation of the failure (or at least, one cause of the failure).

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Even with bug 615417 fixed I'm still seeing failures in 580160-1.html with my patch stack. The problem seems to be in the reftest harness; we're not correctly waiting to get the correct-size plugin image before taking the snapshot.

The code in reftest.js controlling how a test waits for completion has become a mess. There are really three different conditions we can be waiting for: 'reftest-wait' to be removed from the root element's class, all paints to be flushed, and all plugin "explicit waiters" to be removed. Right now we wait for one of those conditions at a time and transition from waiting for one to waiting for another. A simpler and more robust design would be to keep checking all three conditions at once in one place and stop when they're all complete simultaneously. We register listeners for a change in each condition that all just call back to that single CheckTestEnd function.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: fix harness

This is simpler, and I hope clearer, and seems to fix the failures I've been seeing.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 495465 [details] [diff] [review]
Part 2: fix harness

No, this needs some work.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'll move the reftest harness changes to a new bug --- bug 617152

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, turns out the test here fails on D3D10 because of bug 617242 (fix in bug).

Comment 23

[Oleg Romashin (:romaxa)]

Comment on attachment 478634 [details] [diff] [review]
fix v2

Patch a bit outdated (some parts are landed)... do we still need it?
I'm ok with this patch, if it is pass plugins reftests

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: updated patch

Some of this landed in bug 617512. I think the rest is ready to land.

Comment 25

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/mozilla-central/rev/9883540535a4