Closed Bug 346421 Opened 15 years ago Closed 15 years ago

drawImage causes incorrect clipping/invalidation in CSS-resized canvas


(Core :: Canvas: 2D, defect)

Windows 2000
Not set





(Reporter: philip, Assigned: vlad)



(Keywords: fixed1.8.1, regression, testcase)


(3 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1b1) Gecko/20060728 BonEcho/2.0b1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1b1) Gecko/20060728 BonEcho/2.0b1

If a canvas is resized with CSS to be displayed at a different size to the internal buffer, and the last command called is a drawImage, then only the region covered by the last drawImage is painted to the screen - everything is clipped to the last image's rectangle, and only the background is drawn outside that region. This worked correctly in FF 1.5, but not in 2.0. If a drawing command other than drawImage is used last, then it works correctly.

Reproducible: Always

Steps to Reproduce:
1. View the test case
2. Wait for the images to load
Actual Results:  
On the normally-sized canvas, there is a large red rectangle and a small green rectangle and two images. On the resized-with-CSS canvas, it is mostly blue (the background colour), with only the second image (plus the sections of the red and green rectangles behind it) visible.

Expected Results:  
The two outputs should be the same (except for the second being stretched wider), and all the rectangles and images should be drawn.
Attached file test case
A possibly related issue: if the 2nd and 4th drawImage calls are shifted upwards by half a pixel, i.e.

   drawImage(img, 0, 0, 192, 192, 16+128, 15.5, 128, 128)

Gecko/20060719 Firefox/ - all is displayed correctly (like before).
Gecko/20060728 BonEcho/2.0b1 - the rectangles and the first image are now drawn (unlike before), but the second image is not drawn. (Actually, if they're moved to overlap, the second image seems to be clipped to the first image's shape.)
Gecko/20060728 Minefield/3.0a1 - all is displayed correctly - the rectangles and the first image are now drawn (unlike before), and the second image is also drawn. (The stretched image is a lot blurrier than in 2.0, though.)
Confirming this is a Windows-only bug. The testcase is fine on Linux.
On the 1.8.1 branch, this regressed between 2006-05-04 and 2006-05-06, probably regressed from bug 333613.
On trunk something changed between 2006-06-01 (with this build, the second image is very small) and 2006-06-09 (you see current behavior with that build)
Ever confirmed: true
Flags: blocking1.9a1?
Flags: blocking1.8.1?
Keywords: regression, testcase
Vlad - can you take a look?
Flags: blocking1.8.1? → blocking1.8.1+
Attached patch force clip reset (obsolete) — Splinter Review
So, this is a fix for the issue, but I haven't been able to figure out -why- it's a fix.  If the problem was that BitBlt was using the source surface clip as well, then it should have affected the non-CSS-scaled version as well.  I'll dig more into this, but this gets things working for now (both trunk and branch).
Assignee: nobody → vladimir
Attachment #231451 - Flags: review?(pavlov)
Attachment #231451 - Flags: review?(pavlov) → review+
Fixed on trunk...
Closed: 15 years ago
Resolution: --- → FIXED
Comment on attachment 231451 [details] [diff] [review]
force clip reset

Asking for 1.8.1; I'm still not happy with the fix (since I don't fully understand the underlying bug), but it won't /hurt/ anything, so is safe nonetheless.
Attachment #231451 - Flags: approval1.8.1?
Comment on attachment 231451 [details] [diff] [review]
force clip reset

a=drivers. Please land on the MOZILLA_1_8_BRANCH.
Attachment #231451 - Flags: approval1.8.1? → approval1.8.1+
From the patch/check-in:

+    fprintf (stderr, "x0 %f y0 %f sx %f sy %f\n", x0, y0, sx, sy); fflush(stderr);

Should that still be there?
I knew there was a reason I was uneasy about this.  The patch is bogus and should be backed out; it caused bug 346943 among other things.

The underlying problem is that pixman (because of RENDER) applies clip to surfaces used as a source, instead of just as the destination (which is the cairo semantics, and the semantics of most other APIs).  The Xlib backend works around this by creating two separate Pictures for each Drawable, one for src and one for dst.

On win32, a cairo_image_surface (and thus a pixman picture) is created for each DIB to use for fallback purposes, pointing to the same data.  When a clip is set on the win32 surface, a corresponding clip is set to on the image surface.  However, if the clip is changed via cairo and nothing is actually drawn on the surface, this doesn't get pushed down to the surface itself, it's just a serial number change.  So, if the surface is used as a source in this state, and fallback needs to happen, the cairo_image_surface will be returned with a bogus clip set, and the broken-looking rendering will happen as shown here.

The real fix is to make sure that cairo_image_surface does the same thing as the xlib backend -- creates two pixman pictures pointing to the same data, and uses one for src operations and one for dst operations.  This is a very intrusive change, but it'll probably be something that I will do for trunk and for cairo itself.  An intermediate fix is to patch the win32 surface to use two cairo_image_surfaces -- one as src and one as dst.
Blocks: 346943
Resolution: FIXED → ---
This patch does the win32 src/dst fallback surface split, and also reverts the previous canvas patch.  This doesn't seem to break any SVG, from my tests of some examples, but that's the only other thing that it could affect on the 1.8 branch.
Attachment #231451 - Attachment is obsolete: true
Attachment #231841 - Flags: review?(pavlov)
Attachment #231841 - Flags: review?(pavlov) → review+
Comment on attachment 231841 [details] [diff] [review]
fix cairo-win32-surface to do correct src clipping

Requesting 1.8.1; I don't want this to go into the trunk because the cairo version on the trunk is different, and I'd like to do the real fix there (in the cairo image surface code).  This patch fixes things for me, and does not seem to cause any problems with SVG in my testing.
Attachment #231841 - Flags: approval1.8.1?
There is another test case at which currently fails (20060803 BonEcho/2.0b1; first drawImage is clipped to current path instead of to dest. rectangle) and sounds like it could possibly be related to (and fixed by) this.
I compiled with the latest patch: it fixes the initial test case in this bug (thanks!) but not the one in my previous comment, so I moved that into bug 347379.
Blocks: 347379
(In reply to comment #16)
> I compiled with the latest patch: it fixes the initial test case in this bug
> (thanks!) but not the one in my previous comment, so I moved that into bug
> 347379.

Arg, that's because I diffed the wrong version of the file, which didn't have the extra cairo_new_path().  I just checked in a fix for the original problem here and for your problem into the trunk.  A slightly different fix will probably go in to the branch, once I get approval.
Comment on attachment 231841 [details] [diff] [review]
fix cairo-win32-surface to do correct src clipping

a=dbaron on behalf of drivers.  Please check in to MOZILLA_1_8_BRANCH and add the keyword fixed1.8.1 once you have done so.
Attachment #231841 - Flags: approval1.8.1? → approval1.8.1+
Attached patch patch for trunkSplinter Review
This is the patch that I landed on the trunk; it's basically the same as what I want on the branch, except I left the cairo bug workaround as #if 1 in the canvas code, and didn't do the win32 surface change -- pending a real fix in cairo.  There are still some cases where this could break -- specifically, a canvas draw sequence that looks like:

  // no further operations after restore

The branch patch fixes this case as well.
Ok; fixes checked in to both branch and trunk.  For real this time.  I hope.  Really.
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Flags: blocking1.9a1?
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.