Closed Bug 1074733 Opened 11 years ago Closed 10 years ago

Canvas fillRect rendering fails with negative width or height beyond canvas boundary

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Version:
32 Branch
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla43
Tracking Flags:
Tracking Status
firefox43 --- fixed

People

(Reporter: shophlf, Assigned: lsalzman)

References

Details

Attachments

(7 files, 1 obsolete file)

Working testcase based on comment #0
586 bytes, text/html
Details
test case to show bug
1.40 KB, text/html
Details
Output from Firefox and IE
32.60 KB, image/png
Details
part 2 - Add reftest to verify canvas 2d rects with negative dimensions are rendered properly
2.72 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
part 3 - remove failure meta-data for WPT tests with canvas drawImage with negative rects
1.83 KB, patch
jgraham
: review+
Details | Diff | Splinter Review
part 4 - fix test_canvas.html tests with negative rects to no longer expect failure
2.94 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
part 1 - Normalize Canvas 2D rects to be positive by flipping negative width or height.
3.07 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Windows NT 6.1; Trident/7.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; .NET4.0C; Tablet PC 2.0; rv:11.0) like Gecko Steps to reproduce: HTML: <canvas id="test" width="200" height="200"></canvas> JAVASCRIPT: var canvas=document.getElementById('test'); var ctx = canvas.getContext("2d"); ctx.fillStyle = 'green'; ctx.fillRect(150, 150, -151, -110); // this fails to render as left edge is off canvas ctx.fillRect(-1, 150, 151, -110); // this is equivalent and renders // however ctx.rect(150, 150, -151, -110); ctx.fill(); // works as expected ctx.fillRect(150, 150, -151, -110); // this fails to render Actual results: fillRect with negative widths or heights which take the edge 'off canvas' do not render Expected results: The fillRect should have rendered (chrome and IE render OK)
Can you attach a minimal self-contained testcase, please.
Flags: needinfo?(shophlf)
The testcase in comment #0 (attached) wfm (looks identical in Chrome and Firefox, anyway)
Attachment #8497410 - Attachment mime type: text/plain → text/html
And so does this: data:text/html;charset=utf-8,<!DOCTYPE html>%0D%0A<html lang%3D"en">%0D%0A <head>%0D%0A <title>Test<%2Ftitle>%0D%0A <%2Fhead>%0D%0A <body>%0D%0A<canvas id%3D"test" width%3D"200" height%3D"200"><%2Fcanvas>%0D%0A<script>%0D%0Avar canvas%3Ddocument.getElementById('test')%3B%0D%0Avar ctx %3D canvas.getContext("2d")%3B%0D%0Actx.fillStyle %3D 'green'%3B%0D%0Actx.fillRect(150%2C 150%2C -151%2C -110)%3B %2F%2F this fails to render as left edge is off canvas%0D%0A<%2Fscript>%0D%0A <%2Fbody>%0D%0A<%2Fhtml>%0D%0A Tempted to mark this WFM.
@Gijs: I see a small artifact with your testcase https://bugzilla.mozilla.org/attachment.cgi?id=8497410 on Nightly. Do you see it too? http://i.imgur.com/2uqIFIb.png
(In reply to Loic from comment #4) > @Gijs: I see a small artifact with your testcase > https://bugzilla.mozilla.org/attachment.cgi?id=8497410 on Nightly. Do you > see it too? http://i.imgur.com/2uqIFIb.png I don't, neither on OS X, nor on my Windows 8 machine. Might be GPU-specific?
Component: Untriaged → Canvas: 2D
Product: Firefox → Core
Yep, it disappears with HWA off, and I got a regression range.
(In reply to Loic from comment #6) > Yep, it disappears with HWA off, and I got a regression range. Great, can you file a different bug? :-)
Yes, but I'm going to update my GPU drivers before that, maybe it will fix it.
I filed bug 1074810.
Attached file test case to show bug —
Flags: needinfo?(shophlf)
Hi Test case test.html added (In reply to Loic from comment #1) > Can you attach a minimal self-contained testcase, please.
This testcase, too, renders the same on Fx 33 on OS X, compared to Chrome. I see 3 colored swatches (green, red, blue). I see the same on Firefox 33 on Windows. What version of Firefox are you using? Can you post the graphics output from Help > Troubleshooting information? Does the problem happen if you restart in safe mode? (I suspect GPU-acceleration is messing with things here)
Flags: needinfo?(shophlf)
Application Basics ------------------ Name: Firefox Version: 32.0.3 User Agent: Mozilla/5.0 (Windows NT 6.1; rv:32.0) Gecko/20100101 Firefox/32.0 Crash Reports for the Last 3 Days --------------------------------- All Crash Reports Extensions ---------- Name: FiddlerHook Version: 2.4.6.0 Enabled: false ID: fiddlerhook@fiddler2.com Graphics -------- Adapter Description: NVIDIA GeForce FX 5200 Adapter Drivers: nvd3dum Adapter RAM: 128 Device ID: 0x0322 DirectWrite Enabled: false (6.2.9200.16492) Driver Date: 10-9-2006 Driver Version: 9.6.8.5 GPU #2 Active: false GPU Accelerated Windows: 1/1 Direct3D 9 Vendor ID: 0x10de WebGL Renderer: Google Inc. -- ANGLE (NVIDIA GeForce FX 5200 Direct3D9Ex vs_2_0 ps_2_0) windowLayerManagerRemote: false AzureCanvasBackend: skia AzureContentBackend: cairo AzureFallbackCanvasBackend: cairo AzureSkiaAccelerated: 0 Important Modified Preferences ------------------------------ browser.cache.disk.capacity: 358400 browser.cache.disk.smart_size.first_run: false browser.cache.disk.smart_size.use_old_max: false browser.cache.frecency_experiment: 3 browser.places.smartBookmarksVersion: 7 browser.sessionstore.upgradeBackup.latestBuildID: 20140923175406 browser.startup.homepage_override.buildID: 20140923175406 browser.startup.homepage_override.mstone: 32.0.3 extensions.lastAppVersion: 32.0.3 network.cookie.prefsMigrated: true places.database.lastMaintenance: 1412017075 places.history.expiration.transient_current_max_pages: 94629 plugin.disable_full_page_plugin_for_types: application/pdf plugin.importedState: true privacy.sanitize.migrateFx3Prefs: true storage.vacuum.last.index: 1 storage.vacuum.last.places.sqlite: 1411927526 JavaScript ---------- Incremental GC: true Accessibility ------------- Activated: false Prevent Accessibility: 0 Library Versions ---------------- NSPR Expected minimum version: 4.10.6 Version in use: 4.10.6 NSS Expected minimum version: 3.16.5 Basic ECC Version in use: 3.16.5 Basic ECC NSSSMIME Expected minimum version: 3.16.5 Basic ECC Version in use: 3.16.5 Basic ECC NSSSSL Expected minimum version: 3.16.5 Basic ECC Version in use: 3.16.5 Basic ECC NSSUTIL Expected minimum version: 3.16.5 Version in use: 3.16.5 Experimental Features --------------------- (In reply to :Gijs Kruitbosch from comment #12) > This testcase, too, renders the same on Fx 33 on OS X, compared to Chrome. I > see 3 colored swatches (green, red, blue). I see the same on Firefox 33 on > Windows. > > What version of Firefox are you using? Can you post the graphics output from > Help > Troubleshooting information? Does the problem happen if you restart > in safe mode? > > (I suspect GPU-acceleration is messing with things here)
Flags: needinfo?(shophlf)
Right, so if you restart Firefox in safe mode (Help > Restart with add-ons disabled) - do you still see the same issue?
Flags: needinfo?(shophlf)
(safe mode, contrary to the help menu label, will also disable hardware acceleration)
Component: Canvas: 2D → Graphics
Hi Yes I do. I've also seen this on a friend's laptop (Fx 32 under W8) (which is why I investigated it). Also checked on a W8 pc with fx 32.0.3. Am I the only one seeing this (on 3 separate pcs)? I note that I'm on 32.0.3 (which it tells me is up to date) but there's a ref to V33 above... (In reply to :Gijs Kruitbosch from comment #14) > Right, so if you restart Firefox in safe mode (Help > Restart with add-ons > disabled) - do you still see the same issue?
Flags: needinfo?(shophlf)
Maybe you could attach a screenshot of what you're seeing, because we are not able to reproduce it.
(In reply to M from comment #16) > Hi > Yes I do. > I've also seen this on a friend's laptop (Fx 32 under W8) (which is why I > investigated it). > Also checked on a W8 pc with fx 32.0.3. > Am I the only one seeing this (on 3 separate pcs)? Probably not, but we should figure out what you're seeing (see Loic's suggestion - a screenshot of what you expect (chrome/IE) and what you do see would be helpful!) > I note that I'm on 32.0.3 (which it tells me is up to date) but there's a > ref to V33 above... 33 is in beta (https://beta.mozilla.org/ ). I checked 32 on OS X just now, and that produces the same rendering for me. Could be that it's Windows-specific and got fixed in 33. I'm checking against 32 on Windows in a bit. Either way, screenshots would be useful :-)
(it looks the same for me on 32.0.3 on my win8 machine, I'm afraid)
OK - I've uploaded a snip from both IE and firefox M (In reply to Loic from comment #17) > Maybe you could attach a screenshot of what you're seeing, because we are > not able to reproduce it.
I see you're running Windows 7 32 bits with an old GPU (NVIDIA GeForce FX 5200). Which drivers did you install? These ones for Vista? The last drivers for your GPU on Win XP are 175.19 (23.6.2008). Are they compatible with Win 7?
I think my setup is a red herring. I've seen this on 3 totally different pc's, some new, some old, XP, W7, W8. In fact all pc's I have access to. I really support open sources and the communities behind them - hence my researching this bug to find the cause (fillRect) and reporting it here. I have a fix for my s/w (rect() then fill() rather than fillRect) so I'm OK. If you can't reproduce it I suggest you drop it. I think I've done all I can. Good luck M (In reply to Loic from comment #22) > I see you're running Windows 7 32 bits with an old GPU (NVIDIA GeForce FX > 5200). Which drivers did you install? These ones for Vista? > > The last drivers for your GPU on Win XP are 175.19 (23.6.2008). Are they > compatible with Win 7?
I can reproduce the problem on Mac OS X 10.9.5 with both Firefox stable 40.0.3 and the developer edition 42.0a2 (2015-09-08). The same happens on Mac OS X 10.10 and Windows 7 running on VMWare Fusion 7 (tested with FF 32, 40.0.3 and the developer edition 42.0a2 (2015-09-08)).
Surprisingly, 11 months on, this reproduces trivially on my OSX machine (2013 15" retina MBP, NVIDIA GeForce GT 650M 1024 MB). I expect there is some difference in acceleration/gfx architecture that wasn't enabled for me before that is enabled now. Either way, um, Milan, can you have a look / prioritize this?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(milan)
Our use case does not include strokeRect, but it seems the same problem affects it as well. Should a separate report be opened?
strokeRect would be the same root cause, we don't need a separate bug. It reproduces because we switched OS X to use Skia for canvas (which is what unaccelerated Windows use as well.) And Skia's SkCanvas::quickReject doesn't like this scenario. If you were to set preference gfx.canvas.azure.backends to cairo, the problem would go away (not recommending this, just as a way to confirm this is a Skia issue.) Lee, it may be worth asking Skia upstream how they'd feel about a change to SkCanvas::quickReject to support left > right and bottom > top cases.
Flags: needinfo?(milan)
Setting the preference gfx.canvas.azure.backends to cairo makes the problem disappear for both fillRect and strokeRect.
The canvas spec allows rects to be specified with negative width or height, but both our Rect object and Skia's SkRect object do not support such rectangles. Upon inspection of Chromium's canvas 2d code, we decided it made most sense to normalize the rects at the level of Canvas2DRenderingContext code rather than within the DrawTargets. Since the Rect passed in is technically empty, as far as our own code is concerned, it could cause bustage elsewhere if propagated downstream, so it is best to fix it upstream before this occurs. Thus, this patch fixes the entry points for clearRect, fillRect, strokeRect, and drawImage to normalize any such negative dimensions before invalid Rect objects would have been created for them.
Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
Attachment #8658935 - Flags: review?(jmuizelaar)
This adds a reftest based on the reporter's submitted test case, with some simplifications. It just verifies that the rects supplied with negative dimensions render the same as rects that have positive dimensions but occupying the same space.
Attachment #8658937 - Flags: review?(jmuizelaar)
Attachment #8658937 - Flags: review?(jmuizelaar) → review+
Now that rects are normalized to be positive in 2d canvas, WPT tests are passing that weren't passing before, so failure meta-data needs to be removed.
Attachment #8659353 - Flags: review?(james)
Comment on attachment 8659353 [details] [diff] [review] part 3 - remove failure meta-data for WPT tests with canvas drawImage with negative rects Review of attachment 8659353 [details] [diff] [review]: ----------------------------------------------------------------- Assuming this all passes try. Note that in general there's no need to get me to review wpt metadata changes unless your primary reviewer doesn't feel comfortable reviewing those changes.
Attachment #8659353 - Flags: review?(james) → review+
This fixes the 2d.drawImage.negativedest and 2d.drawImage.negativesource tests to no longer be todo and expect to pass. The case in 2d.drawImage.outsidesource that checked for negative width and height throwing an error needs to be removed since it should now actually pass, which causes the test overall to now pass, so the todo() call had to be fixed as well.
Attachment #8659385 - Flags: review?(jmuizelaar)
Attachment #8659385 - Flags: review?(jmuizelaar) → review+
Blocks: 655328
Comment on attachment 8658935 [details] [diff] [review] part 1 - Normalize Canvas 2D rects to be positive by flipping negative width or height. Review of attachment 8658935 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/canvas/CanvasRenderingContext2D.cpp @@ +2477,5 @@ > // > // rects > // > > +// bug 1074733 I'd rather have a spec citation than a bug number here.
Attachment #8658935 - Flags: review?(jmuizelaar) → review+
Cosmetic change only. Expanded comment to note that the canvas spec does not disallow negative dimensions in the rect, so we must generate an appropriate rect for the corner points.
Attachment #8658935 - Attachment is obsolete: true
Attachment #8659967 - Flags: review+
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: