615741 - too large canvases don't draw and are black

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Joe Drew (not getting mail)]

Go to http://hg.mozilla.org/mozilla-central . Click on "graph," then click More at the bottom a couple of times. Once you've gone beyond your computer's max texture size, the graph (canvas) goes away (on Windows); on OS X it becomes totally black.

We have to come up with a generalized solution for this. Probably the best bet is to back the canvas with software in these cases, and only draw the visible rect. Unfortunately that'll probably make things slower.

Comment 1

[Joe Drew (not getting mail)]

Jeff and I disagree as to whether this should block. I think if push came to shove, I'd grudgingly accept releasing with this bug, but I really really want it fixed. Jeff, I think, doesn't want this to block 4 at all (but I presume would approve a patch if one appeared).

Comment 2

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #1)
> Jeff and I disagree as to whether this should block. I think if push came to
> shove, I'd grudgingly accept releasing with this bug, but I really really want
> it fixed. Jeff, I think, doesn't want this to block 4 at all (but I presume
> would approve a patch if one appeared).

I think that's a slight mis-characterization, I'm not really sure whether it should block or not. However, I'm not even sure how we'd fix this on Direct2D.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think the most logical way to fix this would be to allow the canvas backing surface to be a different size from the CSS pixel size, so that we scale when drawing into and blitting out of the surface. Then we could simply clamp the backing surface size to the max texture size.

Comment 4

[Matt Woodrow (:mattwoodrow)]

For GL (where the backing surface isn't hardware accelerated), we can fix this by not uploading the entire canvas as a texture and using the visible area instead.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Fix for OpenGL

I'm not 100% sure if the check for GL_MAX_TEXTURE_SIZE is sufficient.

Looking at http://www.opengl.org/resources/faq/technical/texture.htm 21.130 and the docs for glTexImage2D. The only listed error case is exceeding GL_MAX_TEXTURE_SIZE by 2, and the resource page agrees with this: "If your texture isn't hardware accelerated, but still within the size restrictions returned by GL_MAX_TEXTURE_SIZE, it should still render correctly."

It then adds that this is only an estimate and you can use the GL_PROXY_TEXTURE_2D functionality for a definitive test.

This code fixes the example test case, and we could add a CanCreateTexture(location, width, height, format) function for GLContext that includes a proxy check if we need it.

This won't perform particularly well, but its at least correct.

I have a couple of WIP patches to improve performance using GetAsImageSurface and attempting to retain content between paints. Not sure if it's worth the effort for this rare case.

Comment 6

[Joe Drew (not getting mail)]

Comment on attachment 494327 [details] [diff] [review]
Fix for OpenGL

>diff --git a/gfx/layers/opengl/CanvasLayerOGL.cpp b/gfx/layers/opengl/CanvasLayerOGL.cpp

>@@ -197,16 +197,28 @@ CanvasLayerOGL::Updated(const nsIntRect&

>+      //Check the maximum texture size supported by GL. glTexImage2D supports
>+      //images of up to 2 + GL_MAX_TEXTURE_SIZE

Space after //

>+      GLint texSize = gl()->GetMaxTextureSize();; 

Extra semicolon

>+      if (mUpdatedRect.width > (2 + texSize) || mUpdatedRect.height > (2 + texSize)) {

What's the purpose of this "2 +"?

>+        mDelayedUpdates = PR_TRUE;
>+        // This should only ever occur with 2d canvas, WebGL can't already have a texture
>+        // of this size can it?
>+        NS_ASSERTION(mCanvasSurface, 
>+                     "Invalid texture size when WebGL surface already exists at that size?");

NS_ABORT_IF_FALSE so we actually notice.

>@@ -245,32 +257,57 @@ CanvasLayerOGL::RenderLayer(int aPreviou

>+  } else if (mDelayedUpdates) {
>+    NS_ASSERTION(mCanvasSurface, "WebGL canvases should always be using full texture upload");

NS_ABORT_IF_FALSE

>diff --git a/gfx/thebes/GLContext.cpp b/gfx/thebes/GLContext.cpp

>@@ -202,16 +202,17 @@ GLContext::InitWithPrefix(const char *pr

>+        { (PRFuncPtr*) &mSymbols.fGetTexLevelParameteriv, { "GetTexLevelParameteriv", NULL } },

>diff --git a/gfx/thebes/GLContext.h b/gfx/thebes/GLContext.h

>@@ -1284,16 +1288,22 @@ public:

>+    void fGetTexLevelParameteriv(GLenum target, GLint level, GLenum pname, GLint* params) {
>+        BEFORE_GL_CALL;
>+        mSymbols.fGetTexLevelParameteriv(target, level, pname, params);
>+        AFTER_GL_CALL;
>+    }
>+

>diff --git a/gfx/thebes/GLContextSymbols.h b/gfx/thebes/GLContextSymbols.h

>+    typedef void (GLAPIENTRY * PFNGLGETTEXLEVELPARAMETERIV) (GLenum target, GLint level, GLenum pname, GLint* params);
>+    PFNGLGETTEXLEVELPARAMETERIV fGetTexLevelParameteriv;

Unless I'm missing something, these three changes aren't needed?

Looks good. You're right that the correct fix is to use GL_PROXY_TEXTURE_2D, but this is probably a good first approximation that will fix most of the cases.

I don't know if I'm in love with the name _mDelayedUpdates_, but I'm not sure I have a better suggestion.

Comment 7

[Bas Schouten (:bas.schouten)]

(In reply to comment #3)
> I think the most logical way to fix this would be to allow the canvas backing
> surface to be a different size from the CSS pixel size, so that we scale when
> drawing into and blitting out of the surface. Then we could simply clamp the
> backing surface size to the max texture size.

I agree with that idea for Direct2D based canvasses. I am also of the opinion we should not block on this. I wonder what IE9 does, the graph doesn't seem to work in IE9 at all.

Comment 8

[Joe Drew (not getting mail)]

Once again, I sadly blocking-, but I'd love to have a patch.

Comment 9

[Matt Woodrow (:mattwoodrow)]

(In reply to comment #6)

> >+      if (mUpdatedRect.width > (2 + texSize) || mUpdatedRect.height > (2 + texSize)) {
> 
> What's the purpose of this "2 +"?

This is what the docs (http://www.opengl.org/sdk/docs/man/xhtml/glTexImage2D.xml) say the limit is. The comment was meant to reflect that, I can try make this clearer.


> 
> Unless I'm missing something, these three changes aren't needed?

Indeed. Just a remainder from testing GL_PROXY_TEXTURE_2D.
> 
> Looks good. You're right that the correct fix is to use GL_PROXY_TEXTURE_2D,
> but this is probably a good first approximation that will fix most of the
> cases.
> 
> I don't know if I'm in love with the name _mDelayedUpdates_, but I'm not sure I
> have a better suggestion.

Me neither! Happy to change it if anyone comes up with something.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Fix for OpenGL v2

Fixed review comments and moved size check into ::Initialize()

Carrying forward r=joe.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Fix for OpenGL v3

Fixed crash bug.

Carrying forward r=joe

Comment 12

[julompp]

Not that this helps any, but http://www.sqlite.org/docsrc/timeline?n=500 is also affected by this.

Some IE9 platform preview fails in the same way once the canvas gets big enough (for example n=20 works, n=500 doesn't).

In these kinds of graphs, I don't think lowering the resolution would look good. And I don't see why somebody would use a canvas this big, except for a graph. So it's a pretty bad situation.

Comment 13

[jorge alves]

Bug 595956 covers the same issue.

Unless you guys want to keep it open for the direct2d/3d case I think I'm going ahead and resolve it as a duplicate of this one.

Comment 15

[Boris Zbarsky [:bzbarsky]]

This is a web compat regression, right?  Do we just think this is rare enough that not blocking is ok?

Comment 16

[Joe Drew (not getting mail)]

Yes to both.

Comment 17

[Joe Drew (not getting mail)]

v3 was checked in in December: http://hg.mozilla.org/mozilla-central/rev/ff4c041ae7de

This is the OpenGL version of bug 633936, and it's now fixed.

Comment 18

[Florian Hänel [:heeen]]

FYI, I posted a proof of concept for tiling texture images in bug 607687, which enables canvases bigger than the texture size.