662450 - canvas not properly cleared when images drawn -- clearRect seems broken

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Cameron Kaiser [:spectre]]

Attachment: screenshot of abnormal painting, 10.5 x86 Fx5b3

The Mac layer rendering "stutters" when objects move horizontally, leaving a trail. Regressed from 4. I can't enable acceleration on this machine, so I'm not sure if it is simply a problem for unaccelerated Macs. I do not see it on Windows. I see it on both Intel 10.5 Firefox 5 beta (current) on a C2D Mac mini and PPC 10.4 TenFourFox on a G5. Attached screenshot is from the mini running 10.5 x86.

Easiest to see on the Tron: Legacy HTML5 demo ( http://disneydigitalbooks.go.com/tron/ ); it fractures all over the place. Firefox 4.0.1 and TenFourFox 4.0.2 don't do this.

  Graphics

        Adapter Description
        0x24000,0x20400

        Vendor ID
        0000

        Device ID
        0000

        Adapter RAM

        Adapter Drivers

        Driver Version

        Driver Date

        Direct2D Enabled
        false

        DirectWrite Enabled
        false

        WebGL Renderer
        (WebGL unavailable)

        GPU Accelerated Windows
        0/1

Comment 1

[Cameron Kaiser [:spectre]]

Attachment: screenshot of abnormal painting, 10.5 x86 Fx5b3

Probably should crop this.

Comment 2

[Cameron Kaiser [:spectre]]

Build identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.5; rv:2.2a1pre) Gecko/20110324 Firefox/4.2a1pre has the bug.

/20110323 does not.

Comment 3

[Cameron Kaiser [:spectre]]

Tinkering with it, I suspect the canvas has something to do with the glitches. Was this caused by bug 622072?

Comment 4

[Joe Drew (not getting mail)]

I can't reproduce this at all on 10.6, even turning off hardware acceleration and even running Minefield in 32-bit mode.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

I can reproduce this on 10.7 with acceleration on.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

I can also reproduce this in 6.0a2 on the same machine.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

But it works fine in 7. i.e. 5 => broken, 6 => broken, 7 => fixed.

Comment 8

[Joe Drew (not getting mail)]

comment 4 was with Firefox 7 (nightly).

Comment 9

[Joe Drew (not getting mail)]

So what we're really looking for is a fix window, since this fails in 5 and 6 but works in 7.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

I have a test case that I'm working on reducing up at http://people.mozilla.com/~jmuizelaar/p/tron/Tron_%20Legacy.html

Comment 11

[Jeff Muizelaar [:jrmuizel]]

The test case can be fixed with the following patch:

diff --git a/Tron_ Legacy_files/experience.js b/Tron_ Legacy_files/experience.js
index f47cc7b..9eef7f9 100644
--- a/Tron_ Legacy_files/experience.js  
+++ b/Tron_ Legacy_files/experience.js  
@@ -542,6 +542,7 @@ function Experience()
                this.DrawCallCount = 0;
                
                // clear surface & prepare for rendering of all visible pages
+               surface.fillRect(0, 0, 0, 0);
                surface.clearRect(0, 0, surfaceWidth, surfaceHeight);
                surface.save();
                surface.translate(mRoundedViewportX, 20);

This seems to, for what ever reason, cause the clearRect to happen work.

Comment 12

[Cameron Kaiser [:spectre]]

20110525 has the bug.
20110527 has the bug.
20110528 does NOT have the bug (is now corrected).
about:buildconfig yields http://hg.mozilla.org/mozilla-central/rev/9260bd59d5ce

Comment 13

[Cameron Kaiser [:spectre]]

Cairo 1.10 landed on 5/27. (bug 562746)
I don't see anything else obvious around that time that would have affected it.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Cairo does is_clear tracking, I wonder if it's getting confused and causing the clear to be missed. I'm not sure what would've caused this to happen in 4 though.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

The Dbg.PreDraw function seems to cause the problem.

Comment 16

[Jeff Muizelaar [:jrmuizel]]

I have a reduced test case here:
http://people.mozilla.com/~jmuizelaar/p/tron/reduced.html

Comment 17

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

Attachment: fix

This fixes it for me.

I will write a test and see if I can figure out why it's passing on trunk.

Comment 18

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

Attachment: test

This fails on beta before the patch, and passes after it.

It appears to also fail on trunk (at least in a build from May 12). I'm doing a fresh trunk build to verify.

Comment 19

[Jeff Muizelaar [:jrmuizel]]

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

>From: Robert O'Callahan <[e-mail]>
>Bug 662450. Don't use an image source with Quartz's CLEAR operator, since Quartz seems to have bugs with that. r=jrmuizel
>
>diff --git a/gfx/cairo/cairo/src/cairo-quartz-surface.c b/gfx/cairo/cairo/src/cairo-quartz-surface.c
>--- a/gfx/cairo/cairo/src/cairo-quartz-surface.c
>+++ b/gfx/cairo/cairo/src/cairo-quartz-surface.c
>@@ -1642,16 +1642,28 @@ _cairo_quartz_setup_state (cairo_quartz_
>         state.action = DO_NOTHING;
>         return state;
>     }
>     if (status) {
>         state.action = DO_UNSUPPORTED;
>         return state;
>     }
> 
>+    if (op == CAIRO_OPERATOR_CLEAR) {
>+        /* Quartz seems to have a broken "fast path" for CLEAR used with an
>+           image source; if the image is entirely outside the clip rect, it
>+           does nothing. Since the source isn't supposed to matter, we can
>+           just use a solid color fill instead. This is more efficient to
>+           set up anyway. */
>+        CGContextSetRGBStrokeColor(context, 0, 0, 0, 1.0);
>+        CGContextSetRGBFillColor(context, 0, 0, 0, 1.0);
>+        state.action = DO_SOLID;
>+        return state;
>+    }
>+

I'm not sure this is a bug in Quartz. I expect CGContextDrawImage is always bound by the source. I wonder if I better fix would be to change the source on the cairo surface.

Comment 20

[LegNeato]

So, for beta should we wait for the better fix or go with what's here? Or, should we back out bug 622072?

Comment 21

[Joe Drew (not getting mail)]

Nobody has tested whether backing out bug 622072 fixes this bug. I'm going to test that now.

Comment 22

[Jeff Muizelaar [:jrmuizel]]

Summary thus far:
- this probably doesn't affect a lot of content other than the tron page
- my feeling is that the fix will not be large or very risky (e.g. roc's patch)
- the problem seems limited to mac (it doesn't show up on windows, no one's tested linux)
- we don't know why this problem appeared
- we don't know why this problem went away (the cairo update is suspected as fixing it)

Given what we know so far, I wouldn't have any major objections to fixing this after the beta.

Comment 23

[Jeff Muizelaar [:jrmuizel]]

Attachment: An alternative patch

Comment 24

[Joe Drew (not getting mail)]

(In reply to comment #21)
> Nobody has tested whether backing out bug 622072 fixes this bug. I'm going
> to test that now.

Backing out bug 622072 does not fix this bug.

Comment 25

[Joe Drew (not getting mail)]

(In reply to comment #24)
> (In reply to comment #21)
> > Nobody has tested whether backing out bug 622072 fixes this bug. I'm going
> > to test that now.
> 
> Backing out bug 622072 does not fix this bug.

Belay that - I built the wrong thing. Rebuilding...

Comment 26

[Joe Drew (not getting mail)]

OK, re-verified: this is not caused by bug 622072.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #2)
> Build identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.5; rv:2.2a1pre)
> Gecko/20110324 Firefox/4.2a1pre has the bug.
> 
> /20110323 does not.

Which builds in particular (and what was the changeset they were built on)?  Those were days with multiple nightly builds per day.  Can you reduce the range further?

Comment 28

[LegNeato]

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

Approved for beta and aurora (after the r+ of course, which is expected to come in shortly)

Comment 29

[Jeff Muizelaar [:jrmuizel]]

Landed on beta. I'm going to wait a bit before landing on aurora.

Comment 30

[Jeff Muizelaar [:jrmuizel]]

It looks like the reason this is fixed on trunk is the following hunk added
to fill()

+       op = _reduce_op (gstate);
+       if (op == CAIRO_OPERATOR_CLEAR) {
+           pattern = &_cairo_pattern_clear.base;
+       } else {
+           _cairo_gstate_copy_transformed_source (gstate, &source_pattern.base);
+           pattern = &source_pattern.base;
+       }
+

Comment 31

[LegNeato]

Does this change our stopgap for beta? We haven't gone to build yet but will do so soon...

Comment 32

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #31)
> Does this change our stopgap for beta? We haven't gone to build yet but will
> do so soon...

No this doesn't change our stopgap measure.

Comment 33

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

Comment on attachment 538037 [details] [diff] [review]
An alternative patch

Review of attachment 538037 [details] [diff] [review]:
-----------------------------------------------------------------

You're right! I didn't realize CLEAR was supposed to be bounded in cairo.

Comment 34

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #33)
> Comment on attachment 538037 [details] [diff] [review] [review]
> An alternative patch
> 
> Review of attachment 538037 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> You're right! I didn't realize CLEAR was supposed to be bounded in cairo.

CLEAR is not bounded by the source in cairo.

Comment 35

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

Yeah, I just realized that. CLEAR is bounded by the mask. The mask in this case is the path for the fill. The mask should NOT be determined by the contents of the source.

Comment 36

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

Comment on attachment 538037 [details] [diff] [review]
An alternative patch

Review of attachment 538037 [details] [diff] [review]:
-----------------------------------------------------------------

So this patch is actually incorrect and should not be needed.

Comment 37

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

(In reply to comment #19)
> I'm not sure this is a bug in Quartz. I expect CGContextDrawImage is always
> bound by the source.

Yes, that makes sense.

> I wonder if I better fix would be to change the source
> on the cairo surface.

We have to change the Quartz source so it's not bounded, but cairo callers shouldn't have to change.

(In reply to comment #30)
> It looks like the reason this is fixed on trunk is the following hunk added
> to fill()
> 
> +       op = _reduce_op (gstate);
> +       if (op == CAIRO_OPERATOR_CLEAR) {
> +           pattern = &_cairo_pattern_clear.base;
> +       } else {
> +           _cairo_gstate_copy_transformed_source (gstate,
> &source_pattern.base);
> +           pattern = &source_pattern.base;
> +       }
> +

Makes sense, and that looks like a good change to fix this bug. So we don't need to change any code on trunk, but we should land the test.

Comment 38

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #36)
> Comment on attachment 538037 [details] [diff] [review] [review]
> An alternative patch
> 
> Review of attachment 538037 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> So this patch is actually incorrect and should not be needed.

It's true it shouldn't be needed. I'd argue that it's more correct though. There's no reason we should be doing clearRect with the source from the last operation.

Comment 39

[Cameron Kaiser [:spectre]]

(In reply to comment #27)
> (In reply to comment #2)
> > Build identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.5; rv:2.2a1pre)
> > Gecko/20110324 Firefox/4.2a1pre has the bug.
> > 
> > /20110323 does not.
> 
> Which builds in particular (and what was the changeset they were built on)? 
> Those were days with multiple nightly builds per day.  Can you reduce the
> range further?

I apologize, but unless I'm looking in the wrong place, I only see a single nightly build for each of those days. The changeset in 20110324 was http://hg.mozilla.org/mozilla-central/rev/e11c2f95f781 and of note was bug 639689.

Comment 40

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

(In reply to comment #38)
> It's true it shouldn't be needed. I'd argue that it's more correct though.
> There's no reason we should be doing clearRect with the source from the last
> operation.

Sure there is: the clear operation shouldn't depend on the source, and since it's more work to set the source, we shouldn't do that.

Comment 41

[Cameron Kaiser [:spectre]]

Thanks for the fix; I ran off new TenFourFox and Fx5 beta builds and the issue is resolved.

Comment 42

[George Carstoiu]

Verified issue on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:5.0) Gecko/20100101 Firefox/5.0 - 5b5, taking into consideration the screenshot posted as attachment. 

Issue no longer present - marking as Verified Fixed.

Comment 43

[Asa Dotzler [:asa]]

release team is going to track since it sounds like we want a different solution for aurora than we took on beta.

Comment 44

[Joe Drew (not getting mail)]

I finally finished bisecting.

The first bad revision is:
changeset:   63840:a34fe4ee349a
user:        Robert O'Callahan <[e-mail]>
date:        Thu Mar 24 16:13:58 2011 +1300
summary:     Bug 639689. Part 3: Remove unnecessary full context save/restore, and redundant SetPattern call. r=joe

http://hg.mozilla.org/mozilla-central/rev/a34fe4ee349a

Comment 45

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

OK, that makes sense. I guess before that patch, the full restore would unset the image as the source, so we didn't tickle this bug.

Comment 46

[Johnny Stenback (:jst)]

We took a one-off fix for 5 here, so this is fixed for 5.

Comment 47

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

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

This is the right fix for aurora after all.

The comment is incorrect --- it's not a Quartz bug, just a bug in the way we're using Quartz --- but it's hardly worth fixing since this patch isn't going to land on central anyway.

Comment 48

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/a33c32eca2f9

Comment 49

[Philipp von Weitershausen [:philikon]]

http://hg.mozilla.org/mozilla-central/rev/a33c32eca2f9

Comment 50

[Cameron Kaiser [:spectre]]

Did this get landed in 6? I'm still seeing it.

Comment 51

[Jeff Muizelaar [:jrmuizel]]

Umm, no I don't think it did.

Comment 52

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

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

Review of attachment 537947 [details] [diff] [review]:
-----------------------------------------------------------------

Agh, sorry. Rerequesting approval for FF6.

Comment 53

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

http://hg.mozilla.org/releases/mozilla-beta/rev/f0af5464f529

Comment 54

[Cameron Kaiser [:spectre]]

Thanks, roc!

Comment 55

[Vlad [QA]]

VERIFIED FIXED on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0) Gecko/20100101 Firefox/6.0 (beta2)