Closed Bug 615741 Opened 9 years ago Closed 9 years ago

too large canvases don't draw and are black

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- -

People

(Reporter: joe, Assigned: mattwoodrow)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

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.
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).
blocking2.0: --- → ?
(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.
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.
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.
Attached patch Fix for OpenGL (obsolete) — Splinter Review
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.
Attachment #494327 - Flags: review?(joe)
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.
Attachment #494327 - Flags: review?(joe) → review+
(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.
Once again, I sadly blocking-, but I'd love to have a patch.
blocking2.0: ? → -
(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.
Attached patch Fix for OpenGL v2 (obsolete) — Splinter Review
Fixed review comments and moved size check into ::Initialize()

Carrying forward r=joe.
Attachment #494327 - Attachment is obsolete: true
Attachment #496719 - Flags: review+
Fixed crash bug.

Carrying forward r=joe
Attachment #496719 - Attachment is obsolete: true
Attachment #497678 - Flags: review+
Summary: too large canvases don't draw and be black → too large canvases don't draw and are black
Attachment #497678 - Flags: approval2.0+
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.
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.
Duplicate of this bug: 622384
This is a web compat regression, right?  Do we just think this is rare enough that not blocking is ok?
Keywords: regression
Yes to both.
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.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
FYI, I posted a proof of concept for tiling texture images in bug 607687, which enables canvases bigger than the texture size.
Assignee: nobody → matt.woodrow+bugzilla
You need to log in before you can comment on or make changes to this bug.