Closed Bug 662450 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Graphics, defect)

5 Branch
x86
macOS
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla7
Tracking Status
firefox5 + fixed
firefox6 + fixed
firefox7 --- unaffected

People

(Reporter: spectre, Assigned: roc)

References

()

Details

(Keywords: regression, Whiteboard: [blocks-fx5b5])

Attachments

(4 files, 1 obsolete file)

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
Probably should crop this.
Attachment #537707 - Attachment is obsolete: true
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.
Tinkering with it, I suspect the canvas has something to do with the glitches. Was this caused by bug 622072?
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.
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
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
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.
Attached patch fixSplinter Review
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)
Attached patch testSplinter Review
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.
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.
(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 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;
+       }
+
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.
(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.
Thanks for the fix; I ran off new TenFourFox and Fx5 beta builds and the issue is resolved.
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
Closed: 9 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Attachment #537947 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora-
release team is going to track since it sounds like we want a different solution for aurora than we took on beta.
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.
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
Flags: in-testsuite+
Whiteboard: [blocks-fx5b5][needs landing] → [blocks-fx5b5]
Attachment #537947 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [blocks-fx5b5] → [blocks-fx5b5][needs landing]
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+
Attachment #537947 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Thanks, roc!
VERIFIED FIXED on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:6.0) Gecko/20100101 Firefox/6.0 (beta2)
You need to log in before you can comment on or make changes to this bug.