Last Comment Bug 662450 - canvas not properly cleared when images drawn -- clearRect seems broken
: canvas not properly cleared when images drawn -- clearRect seems broken
Status: VERIFIED FIXED
[blocks-fx5b5]
: regression
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: 5 Branch
: x86 Mac OS X
: -- major (vote)
: mozilla7
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
http://disneydigitalbooks.go.com/tron/
Depends on:
Blocks: 639689
  Show dependency treegraph
 
Reported: 2011-06-06 18:21 PDT by Cameron Kaiser [:spectre]
Modified: 2015-10-07 18:43 PDT (History)
12 users (show)
roc: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
unaffected


Attachments
screenshot of abnormal painting, 10.5 x86 Fx5b3 (840.56 KB, image/png)
2011-06-06 18:21 PDT, Cameron Kaiser [:spectre]
no flags Details
screenshot of abnormal painting, 10.5 x86 Fx5b3 (626.82 KB, image/png)
2011-06-06 18:24 PDT, Cameron Kaiser [:spectre]
no flags Details
fix (1.50 KB, patch)
2011-06-07 20:54 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
jmuizelaar: review+
asa: approval‑mozilla‑beta+
Details | Diff | Splinter Review
test (2.23 KB, patch)
2011-06-07 21:59 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
jmuizelaar: review+
Details | Diff | Splinter Review
An alternative patch (704 bytes, patch)
2011-06-08 09:26 PDT, Jeff Muizelaar [:jrmuizel]
roc: review-
Details | Diff | Splinter Review

Description Cameron Kaiser [:spectre] 2011-06-06 18:21:54 PDT
Created attachment 537707 [details]
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] 2011-06-06 18:24:01 PDT
Created attachment 537708 [details]
screenshot of abnormal painting, 10.5 x86 Fx5b3

Probably should crop this.
Comment 2 Cameron Kaiser [:spectre] 2011-06-06 19:50:53 PDT
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] 2011-06-07 13:53:52 PDT
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) 2011-06-07 15:16:56 PDT
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] 2011-06-07 15:22:33 PDT
I can reproduce this on 10.7 with acceleration on.
Comment 6 Jeff Muizelaar [:jrmuizel] 2011-06-07 15:27:16 PDT
I can also reproduce this in 6.0a2 on the same machine.
Comment 7 Jeff Muizelaar [:jrmuizel] 2011-06-07 15:34:39 PDT
But it works fine in 7. i.e. 5 => broken, 6 => broken, 7 => fixed.
Comment 8 Joe Drew (not getting mail) 2011-06-07 15:38:37 PDT
comment 4 was with Firefox 7 (nightly).
Comment 9 Joe Drew (not getting mail) 2011-06-07 15:39:50 PDT
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] 2011-06-07 16:18:34 PDT
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] 2011-06-07 16:38:13 PDT
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] 2011-06-07 18:11:31 PDT
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] 2011-06-07 18:17:31 PDT
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] 2011-06-07 19:32:56 PDT
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] 2011-06-07 20:00:02 PDT
The Dbg.PreDraw function seems to cause the problem.
Comment 16 Jeff Muizelaar [:jrmuizel] 2011-06-07 20:39:07 PDT
I have a reduced test case here:
http://people.mozilla.com/~jmuizelaar/p/tron/reduced.html
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-07 20:54:59 PDT
Created attachment 537947 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2011-06-07 21:59:18 PDT
Created attachment 537952 [details] [diff] [review]
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] 2011-06-08 07:16:47 PDT
Comment on attachment 537947 [details] [diff] [review]
fix

>From: Robert O'Callahan <robert@ocallahan.org>
>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 christian 2011-06-08 09:05:05 PDT
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) 2011-06-08 09:12:21 PDT
Nobody has tested whether backing out bug 622072 fixes this bug. I'm going to test that now.
Comment 22 Jeff Muizelaar [:jrmuizel] 2011-06-08 09:25:20 PDT
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] 2011-06-08 09:26:11 PDT
Created attachment 538037 [details] [diff] [review]
An alternative patch
Comment 24 Joe Drew (not getting mail) 2011-06-08 11:07:12 PDT
(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) 2011-06-08 11:08:04 PDT
(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) 2011-06-08 11:27:24 PDT
OK, re-verified: this is not caused by bug 622072.
Comment 27 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-06-08 11:59:53 PDT
(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 christian 2011-06-08 12:35:48 PDT
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] 2011-06-08 12:41:55 PDT
Landed on beta. I'm going to wait a bit before landing on aurora.
Comment 30 Jeff Muizelaar [:jrmuizel] 2011-06-08 13:32:40 PDT
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 christian 2011-06-08 13:48:30 PDT
Does this change our stopgap for beta? We haven't gone to build yet but will do so soon...
Comment 32 Jeff Muizelaar [:jrmuizel] 2011-06-08 14:00:32 PDT
(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) (Exited; email my personal email if necessary) 2011-06-08 14:42:39 PDT
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] 2011-06-08 14:45:09 PDT
(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) (Exited; email my personal email if necessary) 2011-06-08 14:52:10 PDT
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) (Exited; email my personal email if necessary) 2011-06-08 14:52:54 PDT
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) (Exited; email my personal email if necessary) 2011-06-08 14:58:45 PDT
(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] 2011-06-08 14:59:36 PDT
(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] 2011-06-08 18:55:03 PDT
(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) (Exited; email my personal email if necessary) 2011-06-08 20:59:05 PDT
(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] 2011-06-09 05:45:33 PDT
Thanks for the fix; I ran off new TenFourFox and Fx5 beta builds and the issue is resolved.
Comment 42 George Carstoiu 2011-06-09 06:05:48 PDT
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] 2011-06-09 14:31:42 PDT
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) 2011-06-09 17:20:31 PDT
I finally finished bisecting.

The first bad revision is:
changeset:   63840:a34fe4ee349a
user:        Robert O'Callahan <robert@ocallahan.org>
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) (Exited; email my personal email if necessary) 2011-06-09 19:51:49 PDT
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, jst@mozilla.com) 2011-06-13 15:19:23 PDT
We took a one-off fix for 5 here, so this is fixed for 5.
Comment 47 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-06-16 20:44:47 PDT
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) (Exited; email my personal email if necessary) 2011-06-16 20:57:46 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/a33c32eca2f9
Comment 49 Philipp von Weitershausen [:philikon] 2011-06-17 07:34:44 PDT
http://hg.mozilla.org/mozilla-central/rev/a33c32eca2f9
Comment 50 Cameron Kaiser [:spectre] 2011-07-08 06:18:27 PDT
Did this get landed in 6? I'm still seeing it.
Comment 51 Jeff Muizelaar [:jrmuizel] 2011-07-08 07:48:25 PDT
Umm, no I don't think it did.
Comment 52 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-07-08 16:52:13 PDT
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) (Exited; email my personal email if necessary) 2011-07-11 15:35:17 PDT
http://hg.mozilla.org/releases/mozilla-beta/rev/f0af5464f529
Comment 54 Cameron Kaiser [:spectre] 2011-07-11 20:59:00 PDT
Thanks, roc!
Comment 55 Vlad [QA] 2011-07-14 00:53:49 PDT
VERIFIED FIXED on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0) Gecko/20100101 Firefox/6.0 (beta2)

Note You need to log in before you can comment on or make changes to this bug.