drawImage causes incorrect clipping/invalidation in CSS-resized canvas

RESOLVED FIXED

Status

()

Core
Canvas: 2D
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Philip Taylor, Assigned: vlad)

Tracking

({fixed1.8.1, regression, testcase})

Trunk
x86
Windows 2000
fixed1.8.1, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

11 years ago
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.
(Reporter)

Comment 1

11 years ago
Created attachment 231225 [details]
test case
(Reporter)

Comment 2

11 years ago
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

11 years ago
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)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.9a1?
Flags: blocking1.8.1?
Keywords: regression, testcase

Comment 5

11 years ago
Vlad - can you take a look?
Flags: blocking1.8.1? → blocking1.8.1+
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).
Assignee: nobody → vladimir
Status: NEW → ASSIGNED
Attachment #231451 - Flags: review?(pavlov)

Updated

11 years ago
Attachment #231451 - Flags: review?(pavlov) → review+
Fixed on trunk...
Status: ASSIGNED → RESOLVED
Last Resolved: 11 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+
fixed on 1.8.1.
Keywords: fixed1.8.1
(Reporter)

Comment 11

11 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #231451 - Attachment is obsolete: true
Attachment #231841 - Flags: review?(pavlov)

Updated

11 years ago
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?
(Reporter)

Comment 15

11 years ago
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.
(Reporter)

Comment 16

11 years ago
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.

Updated

11 years ago
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+
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.
Ok; fixes checked in to both branch and trunk.  For real this time.  I hope.  Really.
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED

Updated

10 years ago
Flags: blocking1.9a1?
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.