473443 - drawImage of canvas onto self is incorrect

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Doug Turner (:dougt)]

+++ 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.

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

this is probably not the solution we want.  Can we get a cairo optimization that allows us to not force a copy?

Comment 2

[Doug Turner (:dougt)]

this should block fennec alpha on windows mobile.

Comment 3

[Doug Turner (:dougt)]

how about a ifdef or pref for the short term?

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 5

[Doug Turner (:dougt)]

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:

http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser.xul#196

The native patch resulted in a similar OOM situation, but i forgot where exactly.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

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?

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Interesting.. the canvas self-copy test was failing, in bug 433235.

Specifically, http://philip.html5.org/tests/canvas/misc/drawimage-self.html .  Though I realize that the spec possibly says something rather dumb here, but I'm not sure what the intended cairo behaviour is...

Comment 8

[Doug Turner (:dougt)]

Attachment: mobile workaround

no one really likes this.

Comment 9

[Doug Turner (:dougt)]

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.

Comment 10

[Stuart Parmenter]

it looks like after we do the blit we don't redraw properly to me.

Comment 11

[Doug Turner (:dougt)]

with attachment 359081 [details] [diff] [review] applied, the testcase
http://philip.html5.org/tests/canvas/misc/drawimage-self.html fails.

Comment 12

[Doug Turner (:dougt)]

with attachment 359081 [details] [diff] [review] applied to Minefield (tip), the testcase
http://philip.html5.org/tests/canvas/misc/drawimage-self.html passes.

Comment 13

[Doug Turner (:dougt)]

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

Comment 14

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 15

[Doug Turner (:dougt)]

jeff, 

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:

http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/nsCanvasRenderingContext2D.cpp#2967

I think this will typically reallocate less memory.

Comment 16

[Doug Turner (:dougt)]

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.

Comment 17

[Doug Turner (:dougt)]

Attachment: patch v.3 bitblt

jeff's bitblt patch.

jeff this includes the change to replace (sx/sy} with (dx/dy) to the call to bitblt.

Comment 18

[Jeff Muizelaar [:jrmuizel]]

Does the patch work with the replacement of (sx sy) with (dx dy)? If it does, that doesn't make any sense to me.

Comment 19

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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?)

Comment 20

[Jeff Muizelaar [:jrmuizel]]

According to Doug we force image surfaces on wince.

Comment 21

[Doug Turner (:dougt)]

on windows mobile, we force everything to be image surfaces.  I do see that printf() that says "FAST PATH" when panning.

Comment 23

[Jeff Muizelaar [:jrmuizel]]

Attachment: v.4 something that actually has a good chance of working

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.

Comment 24

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: fixes shearing issue

Comment 25

[Jeff Muizelaar [:jrmuizel]]

Attachment: v.5 cleanup

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))

Comment 26

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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..

Comment 27

[Jeff Muizelaar [:jrmuizel]]

(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.

Comment 28

[Doug Turner (:dougt)]

this was checked in for WINCE, however the sheering issues were never resolved.  I backed out the change:

http://hg.mozilla.org/mozilla-central/rev/5fed80e87db9

Comment 29

[Doug Turner (:dougt)]

i missed part of the backout and backed that part out this morning:

http://hg.mozilla.org/mozilla-central/rev/6814f4ed7e7f

Comment 30

[Jeff Muizelaar [:jrmuizel]]

Attachment: v.6 fixes the shearing issue

This is what was landed on the tree plus a fix for a typo that was causing the shearing problem.

Comment 31

[Brad Lassey [:blassey] (use needinfo?)]

Jeff, can this be closed out now?

Comment 32

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 33

[Brad Lassey [:blassey] (use needinfo?)]

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?

Comment 34

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/c65d261e7bf6

Comment 35

[Jeff Muizelaar [:jrmuizel]]

Attachment: Enable the bitblt optimization on MOZ_PLATFORM_HILDON in addition to WINCE

I've avoided using a common define for this because it's more a short term solution than anything else.

Comment 36

[Brad Lassey [:blassey] (use needinfo?)]

apologies for the drive by, but MOZ_GFX_OPTIMIZE_MOBILE might be better

Comment 37

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #36)
> apologies for the drive by, but MOZ_GFX_OPTIMIZE_MOBILE might be better

Is that defined for both platforms?

Comment 38

[Brad Lassey [:blassey] (use needinfo?)]

yup, from here http://mxr.mozilla.org/mozilla-central/source/configure.in#2121 and here http://mxr.mozilla.org/mozilla-central/source/configure.in#6302

Comment 39

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add a bitBlt method to canvas

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.

Comment 40

[Jeff Muizelaar [:jrmuizel]]

Attachment: Port fennec to bitBlt from drawImage.

Comment 41

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add a copyRect method to canvas

This version of the patch renames the bitBlt to copyRect and adds a check to make sure we are running as chrome.

Comment 42

[Jeff Muizelaar [:jrmuizel]]

Attachment: Port fennec to copyRect

Comment 43

[Doug Turner (:dougt)]

clearing wanted1.9.1 flag.  we need this for fennec.