Closed Bug 473443 Opened 12 years ago Closed 12 years ago

drawImage of canvas onto self is incorrect


(Core :: Graphics, defect)

Not set



Tracking Status
fennec 1.0a1-wm+ ---


(Reporter: dougt, Assigned: jrmuizel)




(Whiteboard: [wm-m1-b+])


(4 files, 8 obsolete files)

+++ This bug was initially created as a clone of Bug #433235 +++

When drawing a canvas onto an overlapping region of itself, changes in an output row are read back in as input when drawing later rows, resulting in incorrect output.

In bug 433235, we force a copy of the canvas by creating an offscreen surface and draw to that.  However, on mobile, allocation of this offscreen surface is undesirable (on windows mobile it fails as we get OOM due to fragmentation pretty quickly).

it would be great if we could DrawImage using the same canvas without creating a temporary.
Attached patch patch v.1 (obsolete) — Splinter Review
this is probably not the solution we want.  Can we get a cairo optimization that allows us to not force a copy?
this should block fennec alpha on windows mobile.
Flags: blocking-fennec1.0?
Flags: wanted1.9.1?
Flags: blocking-fennec1.0? → blocking-fennec1.0+
how about a ifdef or pref for the short term?
Yeah, just use the MOZ_GFX_MOBILE_OPTIMIZE or whatever define we already have.

However, I still think you should get some more info -- specifically, how big of a surface is being created, what runs out of memory, etc; I have a hard time believing that allocating a second screen-sized surface is causing problems, because if it is, I don't see how we can ever browse the web at all if we're that memory constrained...

You could do some tracing on both the win32 and image surface creation functions (the internal ones, if appropriate), and dump the sizes that are being requested, make sure there isn't anything bogus there.
i traced the image surface creation.  It failed when we tried to malloc a buffer with the size of 1440x800.  This allocation was for the offscreen canvas:

The native patch resulted in a similar OOM situation, but i forgot where exactly.
Interestingly, the cairo self-copy and self-copy-overlap tests pass for me on win32. Vlad, do you have any idea what's broken in cairo that requires us to make a copy?
Interesting.. the canvas self-copy test was failing, in bug 433235.

Specifically, .  Though I realize that the spec possibly says something rather dumb here, but I'm not sure what the intended cairo behaviour is...
Attached patch mobile workaround (obsolete) — Splinter Review
no one really likes this.
Attachment #357041 - Attachment is obsolete: true
Attachment #359081 - Flags: review?(vladimir)
Comment on attachment 359081 [details] [diff] [review]
mobile workaround

after a bunch more testing, i am seeing lots of drawing problems in Fennec.

What looks like is happening is that we duplicate a region that we have painted repeatably offsetting it by some amount small amount.

for example, sometimes, if i pan 50 px to the right, the section that is draw (the 50px wide, however high region), is repeatedly drawn from left to right.
Attachment #359081 - Attachment is obsolete: true
it looks like after we do the blit we don't redraw properly to me.
regarding comment #12.  That test was run on a mac.

Summary (using the attachment 359081 [details] [diff] [review]):

Mac Minefield      Passes
Windows Minefield  FAILS
Windows CE Fennec  FAILS
So I did some digging. Cairo's behaviour when drawing onto itself is undefined. Some surfaces make a copy (quartz, pdf etc) and some do not (image, win32). So the only easy option is to basically do as we are (i.e. making a copy when needed).

Down the road we probably want to formalize cairo's semantics so that patterns are immutable and they can possibly have copy-on-write semantics. This could also let us add optimized self-copy compositing operations like memmove for src etc.

how far down the road? :-)

in the short term, would a better work around be not to duplicate the entire canvas, but rather only the clipping region?

for example, just make sure that this is always true:

I think this will typically reallocate less memory.
on windows mobile, with my short term suggestion above, we still run into the problem that I reported in the first comment.  We fail to allocate the temporary and do not update fennec's canvas resulting in only displaying the checkerboard background.
Whiteboard: [wm-m1-b+]
Attached patch patch v.3 bitblt (obsolete) — Splinter Review
jeff's bitblt patch.

jeff this includes the change to replace (sx/sy} with (dx/dy) to the call to bitblt.
Does the patch work with the replacement of (sx sy) with (dx dy)? If it does, that doesn't make any sense to me.
Do we actually hit that fast path?  Won't we have win32 surfaces there in canvas?  (Though maybe not, I think we're forcing image surfaces on wince?)
According to Doug we force image surfaces on wince.
on windows mobile, we force everything to be image surfaces.  I do see that printf() that says "FAST PATH" when panning.
Duplicate of this bug: 475360
bitblt had some bugs in it: negative offsets were not handled properly and the adjustment of line pointers was totally broken for 3 of the 4 cases.
Attachment #359879 - Attachment is obsolete: true
Attached patch fixes shearing issue (obsolete) — Splinter Review
Attachment #360856 - Attachment is obsolete: true
Attachment #361314 - Flags: review?(vladimir)
Attachment #361314 - Flags: review?(vladimir) → review?(jmuizelaar)
Attached patch v.5 cleanup (obsolete) — Splinter Review
This makes sure that we only use bitblt when it's valid and cleans up bitblt to be a bit more clear.

Is this hunk correct?

-            // force a copy if we couldn't get the surface, or if it's
-            // the same as what we have
-            if (sourceSurface == mSurface || NS_FAILED(rv))
+            // force a copy if we couldn't get the surface
+            if (NS_FAILED(rv))
Attachment #361314 - Attachment is obsolete: true
Attachment #361635 - Flags: review?
Attachment #361314 - Flags: review?(jmuizelaar)
Attachment #361635 - Flags: review? → review?(vladimir)
Comment on attachment 361635 [details] [diff] [review]
v.5 cleanup

Actually, I take that r+ back... problems here:

1) overlapping non-opaque surfaces as you pointed out; easy fix, just do the surface copy if we don't take the fast path

2) fast path doesn't take clip into account.  maybe just check if there s a clip set and skip fast path if so? but I think fennec sets a clip often, so..
tracking-fennec: --- → 1.0a1-wm+
Assignee: nobody → jmuizelaar
(In reply to comment #14)
> Down the road we probably want to formalize cairo's semantics so that patterns
> are immutable and they can possibly have copy-on-write semantics. This could
> also let us add optimized self-copy compositing operations like memmove for src
> etc.

This won't work. It doesn't make sense have immutable patterns and use bitblt type self-copy operation.

The better long term solution to this may be to avoid doing self-copies using cairo.
this was checked in for WINCE, however the sheering issues were never resolved.  I backed out the change:
i missed part of the backout and backed that part out this morning:
This is what was landed on the tree plus a fix for a typo that was causing the shearing problem.
Attachment #361635 - Attachment is obsolete: true
Jeff, can this be closed out now?
This patch hasn't landed yet. It's also still not a good long term solution. I can't see painting a canvas on itself really ever being performant. We should work to see if we can avoid doing it in any places that need to be fast.
Sorry, I thought the "This is what was landed on the tree" meant it landed, now I see what you meant.  Is this ready for review then?  Is this something you want to land or is there another approach you'd like to take?
Closed: 12 years ago
Resolution: --- → FIXED
I've avoided using a common define for this because it's more a short term solution than anything else.
Attachment #373858 - Flags: review?(vladimir)
apologies for the drive by, but MOZ_GFX_OPTIMIZE_MOBILE might be better
(In reply to comment #36)
> apologies for the drive by, but MOZ_GFX_OPTIMIZE_MOBILE might be better

Is that defined for both platforms?
Attached patch Add a bitBlt method to canvas (obsolete) — Splinter Review
Here's an alternative approach to fixing the fennec scrolling problem.

It adds a bitBlt method to a canvas context that can be used to copy a section of the target surface. (I'll probably rename it to copyRect() as that better reflects what it does). This approach avoids having to add a bunch of special cases and complexity to the drawImage routine which I'm not in a rush to do, especially so late in the release cycle. We also avoid having to make a copy when using xlib and win32 surfaces unlike the previous approach.

It should be noted that I don't every want to expose this method to content and would like to remove it as soon as fennec switches to a method of scrolling that's more suited to hardware acceleration.
This version of the patch renames the bitBlt to copyRect and adds a check to make sure we are running as chrome.
Attachment #376567 - Attachment is obsolete: true
Attachment #376568 - Attachment is obsolete: true
Attachment #376720 - Flags: review?(vladimir)
clearing wanted1.9.1 flag.  we need this for fennec.
Flags: wanted1.9.1?
Attachment #376720 - Flags: review?(vladimir)
You need to log in before you can comment on or make changes to this bug.