Closed Bug 1074733 Opened 10 years ago Closed 9 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: