Last Comment Bug 346421 - drawImage causes incorrect clipping/invalidation in CSS-resized canvas
: drawImage causes incorrect clipping/invalidation in CSS-resized canvas
Status: RESOLVED FIXED
: fixed1.8.1, regression, testcase
Product: Core
Classification: Components
Component: Canvas: 2D (show other bugs)
: Trunk
: x86 Windows 2000
-- normal (vote)
: ---
Assigned To: Vladimir Vukicevic [:vlad] [:vladv] (not actively reading bugmail))
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks: 346943 347379
  Show dependency treegraph
 
Reported: 2006-07-29 06:47 PDT by Philip Taylor
Modified: 2007-04-05 20:23 PDT (History)
8 users (show)
mtschrep: blocking1.8.1+
jwalden+bmo: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
test case (1.17 KB, text/html)
2006-07-29 06:48 PDT, Philip Taylor
no flags Details
force clip reset (4.80 KB, patch)
2006-07-31 12:23 PDT, Vladimir Vukicevic [:vlad] [:vladv] (not actively reading bugmail))
pavlov: review+
mbeltzner: approval1.8.1+
Details | Diff | Splinter Review
fix cairo-win32-surface to do correct src clipping (16.68 KB, patch)
2006-08-02 16:54 PDT, Vladimir Vukicevic [:vlad] [:vladv] (not actively reading bugmail))
pavlov: review+
dbaron: approval1.8.1+
Details | Diff | Splinter Review
patch for trunk (5.95 KB, patch)
2006-08-04 11:04 PDT, Vladimir Vukicevic [:vlad] [:vladv] (not actively reading bugmail))
no flags Details | Diff | Splinter Review

Description User image Philip Taylor 2006-07-29 06:47:28 PDT
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.
Comment 1 User image Philip Taylor 2006-07-29 06:48:51 PDT
Created attachment 231225 [details]
test case
Comment 2 User image Philip Taylor 2006-07-29 08:33:34 PDT
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)

then:
Gecko/20060719 Firefox/1.5.0.5 - 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.)
Comment 3 User image Sylvain Pasche 2006-07-29 08:57:30 PDT
Confirming this is a Windows-only bug. The testcase is fine on Linux.
Comment 4 User image Martijn Wargers (dead) 2006-07-29 10:18:43 PDT
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)
Comment 5 User image Mike Schroepfer 2006-07-31 10:34:15 PDT
Vlad - can you take a look?
Comment 6 User image Vladimir Vukicevic [:vlad] [:vladv] (not actively reading bugmail)) 2006-07-31 12:23:02 PDT
Created attachment 231451 [details] [diff] [review]
force clip reset

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).
Comment 7 User image Vladimir Vukicevic [:vlad] [:vladv] (not actively reading bugmail)) 2006-07-31 14:07:22 PDT
Fixed on trunk...
Comment 8 User image Vladimir Vukicevic [:vlad] [:vladv] (not actively reading bugmail)) 2006-08-01 11:36:55 PDT
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.
Comment 9 User image Mike Beltzner [:beltzner, not reading bugmail] 2006-08-01 15:15:30 PDT
Comment on attachment 231451 [details] [diff] [review]
force clip reset

a=drivers. Please land on the MOZILLA_1_8_BRANCH.
Comment 10 User image Vladimir Vukicevic [:vlad] [:vladv] (not actively reading bugmail)) 2006-08-01 15:46:22 PDT
fixed on 1.8.1.
Comment 11 User image Philip Taylor 2006-08-01 16:00:28 PDT
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?
Comment 12 User image Vladimir Vukicevic [:vlad] [:vladv] (not actively reading bugmail)) 2006-08-02 16:49:16 PDT
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.
Comment 13 User image Vladimir Vukicevic [:vlad] [:vladv] (not actively reading bugmail)) 2006-08-02 16:54:21 PDT
Created attachment 231841 [details] [diff] [review]
fix cairo-win32-surface to do correct src clipping

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 carto.net examples, but that's the only other thing that it could affect on the 1.8 branch.
Comment 14 User image Vladimir Vukicevic [:vlad] [:vladv] (not actively reading bugmail)) 2006-08-03 17:28:37 PDT
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.
Comment 15 User image Philip Taylor 2006-08-03 19:07:32 PDT
There is another test case at http://canvex.lazyilluminati.com/misc/imgclip.html 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.
Comment 16 User image Philip Taylor 2006-08-04 09:20:58 PDT
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.
Comment 17 User image Vladimir Vukicevic [:vlad] [:vladv] (not actively reading bugmail)) 2006-08-04 11:01:36 PDT
(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 18 User image David Baron :dbaron: ⌚️UTC-7 2006-08-04 11:04:07 PDT
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.
Comment 19 User image Vladimir Vukicevic [:vlad] [:vladv] (not actively reading bugmail)) 2006-08-04 11:04:08 PDT
Created attachment 232157 [details] [diff] [review]
patch for trunk

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:

{
  save();
  draw_some_path();
  clip();
  draw_something_else();
  restore();
  // no further operations after restore
}

The branch patch fixes this case as well.
Comment 20 User image Vladimir Vukicevic [:vlad] [:vladv] (not actively reading bugmail)) 2006-08-04 11:15:16 PDT
Ok; fixes checked in to both branch and trunk.  For real this time.  I hope.  Really.

Note You need to log in before you can comment on or make changes to this bug.