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

VERIFIED FIXED in Firefox 5

Status

()

Core
Graphics
--
major
VERIFIED FIXED
6 years ago
2 years ago

People

(Reporter: spectre, Assigned: roc)

Tracking

({regression})

5 Branch
mozilla7
x86
Mac OS X
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox5+ fixed, firefox6+ fixed, firefox7 unaffected)

Details

(Whiteboard: [blocks-fx5b5], URL)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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
(Reporter)

Comment 1

6 years ago
Created attachment 537708 [details]
screenshot of abnormal painting, 10.5 x86 Fx5b3

Probably should crop this.
Attachment #537707 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
tracking-firefox5: --- → ?
(Reporter)

Comment 2

6 years ago
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.
(Reporter)

Comment 3

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

Updated

6 years ago
tracking-firefox5: ? → +

Updated

6 years ago
tracking-firefox5: + → ?
Keywords: regression, regressionwindow-wanted

Updated

6 years ago
tracking-firefox5: ? → +

Updated

6 years ago
Whiteboard: [blocks-fx5b5]
I can't reproduce this at all on 10.6, even turning off hardware acceleration and even running Minefield in 32-bit mode.
I can reproduce this on 10.7 with acceleration on.
I can also reproduce this in 6.0a2 on the same machine.
But it works fine in 7. i.e. 5 => broken, 6 => broken, 7 => fixed.
tracking-firefox6: --- → ?
Summary: graphical glitches with horizontal layer scrolling (unaccelerated) → canvas not properly cleared when images drawn
comment 4 was with Firefox 7 (nightly).
So what we're really looking for is a fix window, since this fails in 5 and 6 but works in 7.
I have a test case that I'm working on reducing up at http://people.mozilla.com/~jmuizelaar/p/tron/Tron_%20Legacy.html
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.
Summary: canvas not properly cleared when images drawn → canvas not properly cleared when images drawn -- clearRect doesn't seems broken
(Reporter)

Comment 12

6 years ago
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
(Reporter)

Comment 13

6 years ago
Cairo 1.10 landed on 5/27. (bug 562746)
I don't see anything else obvious around that time that would have affected it.
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.
The Dbg.PreDraw function seems to cause the problem.
I have a reduced test case here:
http://people.mozilla.com/~jmuizelaar/p/tron/reduced.html
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.
Assignee: nobody → roc
Attachment #537947 - Flags: review?(jmuizelaar)
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.
Attachment #537952 - Flags: review?(jmuizelaar)
Summary: canvas not properly cleared when images drawn -- clearRect doesn't seems broken → canvas not properly cleared when images drawn -- clearRect seems broken
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

6 years ago
So, for beta should we wait for the better fix or go with what's here? Or, should we back out bug 622072?
Nobody has tested whether backing out bug 622072 fixes this bug. I'm going to test that now.
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.
Created attachment 538037 [details] [diff] [review]
An alternative patch
Attachment #538037 - Flags: review?(roc)
(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.
(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...
OK, re-verified: this is not caused by bug 622072.
(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

6 years ago
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)
Attachment #537947 - Flags: approval-mozilla-beta+
Attachment #537947 - Flags: approval-mozilla-aurora+
Landed on beta. I'm going to wait a bit before landing on aurora.
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

6 years ago
Does this change our stopgap for beta? We haven't gone to build yet but will do so soon...
(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 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.
Attachment #538037 - Flags: review?(roc) → review+
(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.
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 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.
Attachment #538037 - Flags: review+ → review-
(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.
(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.
(Reporter)

Comment 39

6 years ago
(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.
(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.
(Reporter)

Comment 41

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

Comment 42

6 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Status: RESOLVED → VERIFIED

Updated

6 years ago
Attachment #537947 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora-

Comment 43

6 years ago
release team is going to track since it sounds like we want a different solution for aurora than we took on beta.
tracking-firefox6: ? → +
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
Blocks: 639689
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.

Updated

6 years ago
status-firefox5: --- → fixed
We took a one-off fix for 5 here, so this is fixed for 5.
Attachment #537952 - Flags: review?(jmuizelaar) → review+
Attachment #537947 - Flags: review?(jmuizelaar) → review+
Whiteboard: [blocks-fx5b5] → [blocks-fx5b5][needs landing]
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.
Attachment #537947 - Flags: approval-mozilla-aurora- → approval-mozilla-aurora?
http://hg.mozilla.org/integration/mozilla-inbound/rev/a33c32eca2f9
status-firefox6: --- → affected
Flags: in-testsuite+
Whiteboard: [blocks-fx5b5][needs landing] → [blocks-fx5b5]
http://hg.mozilla.org/mozilla-central/rev/a33c32eca2f9
Target Milestone: --- → mozilla7

Updated

6 years ago
Attachment #537947 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [blocks-fx5b5] → [blocks-fx5b5][needs landing]
(Reporter)

Comment 50

6 years ago
Did this get landed in 6? I'm still seeing it.
Umm, no I don't think it did.
Comment on attachment 537947 [details] [diff] [review]
fix

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

Agh, sorry. Rerequesting approval for FF6.
Attachment #537947 - Flags: approval-mozilla-beta?
Attachment #537947 - Flags: approval-mozilla-beta+
Attachment #537947 - Flags: approval-mozilla-aurora+

Updated

6 years ago
Attachment #537947 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
http://hg.mozilla.org/releases/mozilla-beta/rev/f0af5464f529
status-firefox6: affected → fixed
status-firefox7: --- → unaffected
Whiteboard: [blocks-fx5b5][needs landing] → [blocks-fx5b5]
(Reporter)

Comment 54

6 years ago
Thanks, roc!

Comment 55

6 years ago
VERIFIED FIXED on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0) Gecko/20100101 Firefox/6.0 (beta2)
Keywords: regressionwindow-wanted
You need to log in before you can comment on or make changes to this bug.