Bug 57768 - Accelerated Canvas2D path failing when constructing large canvases.
Summary: Accelerated Canvas2D path failing when constructing large canvases.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-04 11:25 PDT by Jeff Timanus
Modified: 2011-04-08 16:01 PDT (History)
6 users (show)

See Also:


Attachments
Patch (12.51 KB, patch)
2011-04-04 12:55 PDT, Jeff Timanus
no flags Details | Formatted Diff | Diff
Patch (10.96 KB, patch)
2011-04-04 13:37 PDT, Jeff Timanus
no flags Details | Formatted Diff | Diff
Patch (14.08 KB, patch)
2011-04-07 10:17 PDT, Jeff Timanus
no flags Details | Formatted Diff | Diff
Patch (7.59 KB, patch)
2011-04-08 10:56 PDT, Jeff Timanus
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeff Timanus 2011-04-04 11:25:23 PDT
The Chromium accelerated canvas 2d path fails when the requested canvas size is larger than supported by the GL implementation.  The code fails to allocate a texture, yet continues to try to use the accelerated path.

This is demonstrated by test: fast/canvas/canvas-large-dimensions.html

This issue is related to:
  https://bugs.webkit.org/show_bug.cgi?id=52341
  https://bugs.webkit.org/show_bug.cgi?id=52391
Comment 1 Jeff Timanus 2011-04-04 12:55:43 PDT
Created attachment 88105 [details]
Patch
Comment 2 Jeff Timanus 2011-04-04 12:57:58 PDT
Comment on attachment 88105 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88105&action=review

> Source/WebCore/ChangeLog:-424
> -        DumpRenderTreeÕs delegate to be dispatched. The delegate doesnÕt expect to be called between

Will upload a new version without these edits.  My editor must have had trouble with the symbols . . .
Comment 3 Jeff Timanus 2011-04-04 13:37:22 PDT
Created attachment 88115 [details]
Patch
Comment 4 James Robinson 2011-04-04 13:44:54 PDT
Comment on attachment 88115 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88115&action=review

> Source/WebCore/platform/graphics/gpu/DrawingBuffer.h:69
> +    bool isValid() const { return m_context; }
> +

IMO instead of having an isValid() DrawingBuffer::create() should return NULL if the drawing buffer couldn't be constructed, and reset() should return false if it can't resize to the new bounds.

> Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:621
>  bool GraphicsContext3DInternal::texImage2D(GC3Denum target, GC3Dint level, GC3Denum internalformat, GC3Dsizei width, GC3Dsizei height, GC3Dint border, GC3Denum format, GC3Denum type, const void* pixels)
>  {
> +    if (width > m_maxTextureSize || height > m_maxTextureSize) {
> +      synthesizeGLError(GraphicsContext3D::INVALID_VALUE);
> +      return false;
> +    }

It seems weird to short-circuit the normal error handling, but only for this function and only for chromium.  What do you think, Ken?

This check is wrong if level != 0, I believe.
Comment 5 Kenneth Russell 2011-04-04 13:53:31 PDT
Comment on attachment 88115 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88115&action=review

>> Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:621
>> +    }
> 
> It seems weird to short-circuit the normal error handling, but only for this function and only for chromium.  What do you think, Ken?
> 
> This check is wrong if level != 0, I believe.

The problem is that the calling code relies on this texImage2D call to return false if something went wrong, and it would be far too expensive to call glGetError after each texImage2D call. This explicit shortcut is a concession to make large layers work properly. Alternatively, we could do some queries at the call site (which is rarely called) to see whether the texture was allocated correctly. It's a problem that glGetTexLevelParameteriv isn't available in GLES2, but maybe there's another query that could be made to see if the texture object was successfully allocated.
Comment 6 Jeff Timanus 2011-04-04 13:55:58 PDT
Comment on attachment 88115 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88115&action=review

>> Source/WebCore/platform/graphics/gpu/DrawingBuffer.h:69
>> +
> 
> IMO instead of having an isValid() DrawingBuffer::create() should return NULL if the drawing buffer couldn't be constructed, and reset() should return false if it can't resize to the new bounds.

Will upload a new patch with your suggested changes.

>> Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:621
>> +    }
> 
> It seems weird to short-circuit the normal error handling, but only for this function and only for chromium.  What do you think, Ken?
> 
> This check is wrong if level != 0, I believe.

You're right that it's a little different to add the error handling here.  I wanted the routine to respect the comments in GraphicsContext3DInternal.h   (http://www.google.com/codesearch/p?vert=chromium#OAMlx_jo-ck/src/third_party/WebKit/Source/WebKit/chromium/src/GraphicsContext3DInternal.h&l=190)

I believe it should work for levels other than 0.
Comment 7 James Robinson 2011-04-04 13:57:20 PDT
(In reply to comment #5)
> (From update of attachment 88115 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88115&action=review
> 
> >> Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:621
> >> +    }
> > 
> > It seems weird to short-circuit the normal error handling, but only for this function and only for chromium.  What do you think, Ken?
> > 
> > This check is wrong if level != 0, I believe.
> 
> The problem is that the calling code relies on this texImage2D call to return false if something went wrong, and it would be far too expensive to call glGetError after each texImage2D call. This explicit shortcut is a concession to make large layers work properly. Alternatively, we could do some queries at the call site (which is rarely called) to see whether the texture was allocated correctly. It's a problem that glGetTexLevelParameteriv isn't available in GLES2, but maybe there's another query that could be made to see if the texture object was successfully allocated.

Can we just make the caller (DrawingBuffer stuff here) glGet() the texture bounds and check that itself?  If the texture object creation fails for any other reason then this check won't do anything.

We query and cache the max texture bounds in some places currently, IIRC
Comment 8 Stephen White 2011-04-04 14:06:06 PDT
Comment on attachment 88115 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88115&action=review

>>>>> Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:621
>>>>> +    }
>>>> 
>>>> It seems weird to short-circuit the normal error handling, but only for this function and only for chromium.  What do you think, Ken?
>>>> 
>>>> This check is wrong if level != 0, I believe.
>>> 
>>> The problem is that the calling code relies on this texImage2D call to return false if something went wrong, and it would be far too expensive to call glGetError after each texImage2D call. This explicit shortcut is a concession to make large layers work properly. Alternatively, we could do some queries at the call site (which is rarely called) to see whether the texture was allocated correctly. It's a problem that glGetTexLevelParameteriv isn't available in GLES2, but maybe there's another query that could be made to see if the texture object was successfully allocated.
>> 
>> You're right that it's a little different to add the error handling here.  I wanted the routine to respect the comments in GraphicsContext3DInternal.h   (http://www.google.com/codesearch/p?vert=chromium#OAMlx_jo-ck/src/third_party/WebKit/Source/WebKit/chromium/src/GraphicsContext3DInternal.h&l=190)
>> 
>> I believe it should work for levels other than 0.
> 
> Can we just make the caller (DrawingBuffer stuff here) glGet() the texture bounds and check that itself?  If the texture object creation fails for any other reason then this check won't do anything.
> 
> We query and cache the max texture bounds in some places currently, IIRC

KBR:  I'm not too worried about a call to getIntegerv(), given that GraphicsContext3D::texImage2DResourceSafe() further up the stack is doing an enormous malloc() and memset() on every texture allocation (we should really doing a glClear() on the bound FBO instead, but that's another bug).  I agree with jamesr:  putting the check in DrawingBuffer would fix the bug, and skip both the malloc() and memset() for large layers.  (BTW, we already do such a check via getIntegerv on every texture creation in platform/graphics/Texture.cpp.)
Comment 9 James Robinson 2011-04-04 14:10:38 PDT
Texture::create() probably should be calling getIntegerv() once and caching the result.  Whoops!
Comment 10 Kenneth Russell 2011-04-04 14:14:19 PDT
(In reply to comment #8)
> KBR:  I'm not too worried about a call to getIntegerv(), given that GraphicsContext3D::texImage2DResourceSafe() further up the stack is doing an enormous malloc() and memset() on every texture allocation (we should really doing a glClear() on the bound FBO instead, but that's another bug).  I agree with jamesr:  putting the check in DrawingBuffer would fix the bug, and skip both the malloc() and memset() for large layers.  (BTW, we already do such a check via getIntegerv on every texture creation in platform/graphics/Texture.cpp.)

I agree that fixing this with a similar check at the call site would be good. It doesn't guarantee that the underlying texImage2D call will succeed, though. The safest thing to do would be to query the width and height of the resulting texture after the call, but we can't do that with the current GraphicsContext3D API, which is targeted at GLES 2.0.

For the time being I think it would be fine to make the calling code behave similarly to Texture.cpp.
Comment 11 Jeff Timanus 2011-04-07 10:17:47 PDT
Created attachment 88657 [details]
Patch
Comment 12 Jeff Timanus 2011-04-07 10:46:16 PDT
Comment on attachment 88657 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=88657&action=review

> Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:205
> +        m_context->texImage2DResourceSafeNoClear(GraphicsContext3D::TEXTURE_2D, 0, internalColorFormat, m_size.width(), m_size.height(), 0, colorFormat, GraphicsContext3D::UNSIGNED_BYTE);

I would like to change this call to a simple texImage2D with a NULL data parameter.  Can someone please confirm if this will work?
Comment 13 Kenneth Russell 2011-04-07 14:20:56 PDT
(In reply to comment #12)
> (From update of attachment 88657 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88657&action=review
> 
> > Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:205
> > +        m_context->texImage2DResourceSafeNoClear(GraphicsContext3D::TEXTURE_2D, 0, internalColorFormat, m_size.width(), m_size.height(), 0, colorFormat, GraphicsContext3D::UNSIGNED_BYTE);
> 
> I would like to change this call to a simple texImage2D with a NULL data parameter.  Can someone please confirm if this will work?

I've looked back through the command buffer client code and am 99% sure that it will work. You'd need to test to be sure.
Comment 14 Zhenyao Mo 2011-04-07 14:27:28 PDT
(In reply to comment #12)
> (From update of attachment 88657 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88657&action=review
> 
> > Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:205
> > +        m_context->texImage2DResourceSafeNoClear(GraphicsContext3D::TEXTURE_2D, 0, internalColorFormat, m_size.width(), m_size.height(), 0, colorFormat, GraphicsContext3D::UNSIGNED_BYTE);
> 
> I would like to change this call to a simple texImage2D with a NULL data parameter.  Can someone please confirm if this will work?

This will break the tests running with in-process gl.
Comment 15 Zhenyao Mo 2011-04-07 14:29:36 PDT
(In reply to comment #12)
> (From update of attachment 88657 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88657&action=review
> 
> > Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:205
> > +        m_context->texImage2DResourceSafeNoClear(GraphicsContext3D::TEXTURE_2D, 0, internalColorFormat, m_size.width(), m_size.height(), 0, colorFormat, GraphicsContext3D::UNSIGNED_BYTE);
> 
> I would like to change this call to a simple texImage2D with a NULL data parameter.  Can someone please confirm if this will work?

Also, it would be more efficient to call texImage2DResourceSafeNoClear because if you pass texImage2D with NULL data to command buffer, it will definitely do the resource safe thing and clear.
Comment 16 Stephen White 2011-04-07 14:32:16 PDT
(In reply to comment #14)
> (In reply to comment #12)
> > (From update of attachment 88657 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=88657&action=review
> > 
> > > Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:205
> > > +        m_context->texImage2DResourceSafeNoClear(GraphicsContext3D::TEXTURE_2D, 0, internalColorFormat, m_size.width(), m_size.height(), 0, colorFormat, GraphicsContext3D::UNSIGNED_BYTE);
> > 
> > I would like to change this call to a simple texImage2D with a NULL data parameter.  Can someone please confirm if this will work?
> 
> This will break the tests running with in-process gl.

I think the reason they broke before is that I was relying on the clearing behaviour that used to exist in both the command buffer and the in-process impl.  If we instead put in an explicit glClear() on the first bind of the DrawingBuffer, I think they should pass.(In reply to comment #15)
> (In reply to comment #12)
> > (From update of attachment 88657 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=88657&action=review
> > 
> > > Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:205
> > > +        m_context->texImage2DResourceSafeNoClear(GraphicsContext3D::TEXTURE_2D, 0, internalColorFormat, m_size.width(), m_size.height(), 0, colorFormat, GraphicsContext3D::UNSIGNED_BYTE);
> > 
> > I would like to change this call to a simple texImage2D with a NULL data parameter.  Can someone please confirm if this will work?
> 
> Also, it would be more efficient to call texImage2DResourceSafeNoClear because if you pass texImage2D with NULL data to command buffer, it will definitely do the resource safe thing and clear.

Really?  Can we change this behaviour?  Canvas does not need resource safety, and this is a big overhead when resizing the window:  the malloc(), the memset() and the texture upload are not necessary, since we know we're going to do a glClear() right away anyway.
Comment 17 Stephen White 2011-04-07 14:34:45 PDT
(In reply to comment #14)
> (In reply to comment #12)
> > (From update of attachment 88657 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=88657&action=review
> > 
> > > Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:205
> > > +        m_context->texImage2DResourceSafeNoClear(GraphicsContext3D::TEXTURE_2D, 0, internalColorFormat, m_size.width(), m_size.height(), 0, colorFormat, GraphicsContext3D::UNSIGNED_BYTE);
> > 
> > I would like to change this call to a simple texImage2D with a NULL data parameter.  Can someone please confirm if this will work?
> 
> This will break the tests running with in-process gl.

I think the reason they failed before was that I was relying on the FBO textures being cleared automatically in the in-process version (in the same way they were in the command buffers).

If we call glClear() the first time the DrawingBuffer is bound, these tests should pass.
Comment 18 Kenneth Russell 2011-04-07 14:35:14 PDT
(In reply to comment #16)
> (In reply to comment #14)
> > (In reply to comment #12)
> > > (From update of attachment 88657 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=88657&action=review
> > > 
> > > > Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:205
> > > > +        m_context->texImage2DResourceSafeNoClear(GraphicsContext3D::TEXTURE_2D, 0, internalColorFormat, m_size.width(), m_size.height(), 0, colorFormat, GraphicsContext3D::UNSIGNED_BYTE);
> > > 
> > > I would like to change this call to a simple texImage2D with a NULL data parameter.  Can someone please confirm if this will work?
> > 
> > This will break the tests running with in-process gl.
> 
> I think the reason they broke before is that I was relying on the clearing behaviour that used to exist in both the command buffer and the in-process impl.  If we instead put in an explicit glClear() on the first bind of the DrawingBuffer, I think they should pass.(In reply to comment #15)
> > (In reply to comment #12)
> > > (From update of attachment 88657 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=88657&action=review
> > > 
> > > > Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:205
> > > > +        m_context->texImage2DResourceSafeNoClear(GraphicsContext3D::TEXTURE_2D, 0, internalColorFormat, m_size.width(), m_size.height(), 0, colorFormat, GraphicsContext3D::UNSIGNED_BYTE);
> > > 
> > > I would like to change this call to a simple texImage2D with a NULL data parameter.  Can someone please confirm if this will work?
> > 
> > Also, it would be more efficient to call texImage2DResourceSafeNoClear because if you pass texImage2D with NULL data to command buffer, it will definitely do the resource safe thing and clear.
> 
> Really?  Can we change this behaviour?  Canvas does not need resource safety, and this is a big overhead when resizing the window:  the malloc(), the memset() and the texture upload are not necessary, since we know we're going to do a glClear() right away anyway.

One point. Even without disabling the resource safety of the command buffer, passing NULL should be more efficient than passing down a zeroed array, because the allocation of the temporary zeroed buffer will occur entirely on the service side, rather than being passed across shared memory in the transfer buffer.
Comment 19 Zhenyao Mo 2011-04-07 14:54:09 PDT
(In reply to comment #17)
> (In reply to comment #14)
> > (In reply to comment #12)
> > > (From update of attachment 88657 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=88657&action=review
> > > 
> > > > Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:205
> > > > +        m_context->texImage2DResourceSafeNoClear(GraphicsContext3D::TEXTURE_2D, 0, internalColorFormat, m_size.width(), m_size.height(), 0, colorFormat, GraphicsContext3D::UNSIGNED_BYTE);
> > > 
> > > I would like to change this call to a simple texImage2D with a NULL data parameter.  Can someone please confirm if this will work?
> > 
> > This will break the tests running with in-process gl.
> 
> I think the reason they failed before was that I was relying on the FBO textures being cleared automatically in the in-process version (in the same way they were in the command buffers).
> 
> If we call glClear() the first time the DrawingBuffer is bound, these tests should pass.

Also, in the in-process gl, we emit an INVALID_VALUE if passed in data is NULL.  You have to remove that before calling texImage2D with NULL.
Comment 20 Zhenyao Mo 2011-04-07 14:56:05 PDT
(In reply to comment #16)
> (In reply to comment #14)
> > (In reply to comment #12)
> > > (From update of attachment 88657 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=88657&action=review
> > > 
> > > > Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:205
> > > > +        m_context->texImage2DResourceSafeNoClear(GraphicsContext3D::TEXTURE_2D, 0, internalColorFormat, m_size.width(), m_size.height(), 0, colorFormat, GraphicsContext3D::UNSIGNED_BYTE);
> > > 
> > > I would like to change this call to a simple texImage2D with a NULL data parameter.  Can someone please confirm if this will work?
> > 
> > This will break the tests running with in-process gl.
> 
> I think the reason they broke before is that I was relying on the clearing behaviour that used to exist in both the command buffer and the in-process impl.  If we instead put in an explicit glClear() on the first bind of the DrawingBuffer, I think they should pass.(In reply to comment #15)
> > (In reply to comment #12)
> > > (From update of attachment 88657 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=88657&action=review
> > > 
> > > > Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:205
> > > > +        m_context->texImage2DResourceSafeNoClear(GraphicsContext3D::TEXTURE_2D, 0, internalColorFormat, m_size.width(), m_size.height(), 0, colorFormat, GraphicsContext3D::UNSIGNED_BYTE);
> > > 
> > > I would like to change this call to a simple texImage2D with a NULL data parameter.  Can someone please confirm if this will work?
> > 
> > Also, it would be more efficient to call texImage2DResourceSafeNoClear because if you pass texImage2D with NULL data to command buffer, it will definitely do the resource safe thing and clear.
> 
> Really?  Can we change this behaviour?  Canvas does not need resource safety, and this is a big overhead when resizing the window:  the malloc(), the memset() and the texture upload are not necessary, since we know we're going to do a glClear() right away anyway.

I don't think we should change the general texImage2D behavior.  However, you could add a new command in the command buffer like texImage2DLightWeight or something.
Comment 21 Kenneth Russell 2011-04-07 14:58:14 PDT
(In reply to comment #19)
> (In reply to comment #17)
> > (In reply to comment #14)
> > > (In reply to comment #12)
> > > > (From update of attachment 88657 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=88657&action=review
> > > > 
> > > > > Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:205
> > > > > +        m_context->texImage2DResourceSafeNoClear(GraphicsContext3D::TEXTURE_2D, 0, internalColorFormat, m_size.width(), m_size.height(), 0, colorFormat, GraphicsContext3D::UNSIGNED_BYTE);
> > > > 
> > > > I would like to change this call to a simple texImage2D with a NULL data parameter.  Can someone please confirm if this will work?
> > > 
> > > This will break the tests running with in-process gl.
> > 
> > I think the reason they failed before was that I was relying on the FBO textures being cleared automatically in the in-process version (in the same way they were in the command buffers).
> > 
> > If we call glClear() the first time the DrawingBuffer is bound, these tests should pass.
> 
> Also, in the in-process gl, we emit an INVALID_VALUE if passed in data is NULL.  You have to remove that before calling texImage2D with NULL.

Mo's right; I forgot about that check in webgraphicscontext3d_in_process_impl.cc in the Chromium tree.

I talked with senorblanco about this topic earlier, and suggested we could have another argument to GraphicsContext3D::create which would indicate whether the caller is trusted (compositor, canvas2d) or untrusted (webgl). In the former case we could disable some of these checks.

twiz, i'm not sure what would be the fastest path forward for you, but feel free to make whatever changes you need to in order to make this most efficient, including adding new entry points. I do think that the constructor idea might be the best long-term direction, though.
Comment 22 Zhenyao Mo 2011-04-07 15:04:41 PDT
(In reply to comment #21)
> (In reply to comment #19)
> > (In reply to comment #17)
> > > (In reply to comment #14)
> > > > (In reply to comment #12)
> > > > > (From update of attachment 88657 [details] [details] [details] [details] [details])
> > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=88657&action=review
> > > > > 
> > > > > > Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:205
> > > > > > +        m_context->texImage2DResourceSafeNoClear(GraphicsContext3D::TEXTURE_2D, 0, internalColorFormat, m_size.width(), m_size.height(), 0, colorFormat, GraphicsContext3D::UNSIGNED_BYTE);
> > > > > 
> > > > > I would like to change this call to a simple texImage2D with a NULL data parameter.  Can someone please confirm if this will work?
> > > > 
> > > > This will break the tests running with in-process gl.
> > > 
> > > I think the reason they failed before was that I was relying on the FBO textures being cleared automatically in the in-process version (in the same way they were in the command buffers).
> > > 
> > > If we call glClear() the first time the DrawingBuffer is bound, these tests should pass.
> > 
> > Also, in the in-process gl, we emit an INVALID_VALUE if passed in data is NULL.  You have to remove that before calling texImage2D with NULL.
> 
> Mo's right; I forgot about that check in webgraphicscontext3d_in_process_impl.cc in the Chromium tree.
> 
> I talked with senorblanco about this topic earlier, and suggested we could have another argument to GraphicsContext3D::create which would indicate whether the caller is trusted (compositor, canvas2d) or untrusted (webgl). In the former case we could disable some of these checks.

We could have a light-weight command buffer where all the sanity checking and other stuff are behind the DEBUG flag, and it simply passes commands down to underlying drivers.

> 
> twiz, i'm not sure what would be the fastest path forward for you, but feel free to make whatever changes you need to in order to make this most efficient, including adding new entry points. I do think that the constructor idea might be the best long-term direction, though.
Comment 23 Stephen White 2011-04-07 15:10:50 PDT
(In reply to comment #21)
> (In reply to comment #19)
> > (In reply to comment #17)
> > > (In reply to comment #14)
> > > > (In reply to comment #12)
> > > > > (From update of attachment 88657 [details] [details] [details] [details] [details])
> > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=88657&action=review
> > > > > 
> > > > > > Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:205
> > > > > > +        m_context->texImage2DResourceSafeNoClear(GraphicsContext3D::TEXTURE_2D, 0, internalColorFormat, m_size.width(), m_size.height(), 0, colorFormat, GraphicsContext3D::UNSIGNED_BYTE);
> > > > > 
> > > > > I would like to change this call to a simple texImage2D with a NULL data parameter.  Can someone please confirm if this will work?
> > > > 
> > > > This will break the tests running with in-process gl.
> > > 
> > > I think the reason they failed before was that I was relying on the FBO textures being cleared automatically in the in-process version (in the same way they were in the command buffers).
> > > 
> > > If we call glClear() the first time the DrawingBuffer is bound, these tests should pass.
> > 
> > Also, in the in-process gl, we emit an INVALID_VALUE if passed in data is NULL.  You have to remove that before calling texImage2D with NULL.
> 
> Mo's right; I forgot about that check in webgraphicscontext3d_in_process_impl.cc in the Chromium tree.

Aha, that's what I ran into before (just forgot where it was).

> I talked with senorblanco about this topic earlier, and suggested we could have another argument to GraphicsContext3D::create which would indicate whether the caller is trusted (compositor, canvas2d) or untrusted (webgl). In the former case we could disable some of these checks.
> 
> twiz, i'm not sure what would be the fastest path forward for you, but feel free to make whatever changes you need to in order to make this most efficient, including adding new entry points. I do think that the constructor idea might be the best long-term direction, though.

I think the constructor param is a good idea.  Since this would require chrome-side changes (as would the in-process change above), perhaps we should split the work into just fixing the layout test, and address the performance issues in a subsequent patch (after the chrome changes).

My preference would be to land something between revs 2 and 3 of this patch (including the DrawingBuffer::create returning NULL change that jamesr suggested, but excluding the ResourceSafeNoClear changes).  This would fix the layout test, and we can look at improving the performance once the chrome-side changes have landed.

Does that sound good?
Comment 24 Jeff Timanus 2011-04-07 15:25:40 PDT
(In reply to comment #23)
> (In reply to comment #21)
> > (In reply to comment #19)
> > > (In reply to comment #17)
> > > > (In reply to comment #14)
> > > > > (In reply to comment #12)
> > > > > > (From update of attachment 88657 [details] [details] [details] [details] [details] [details])
> > > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=88657&action=review
> > > > > > 
> > > > > > > Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:205
> > > > > > > +        m_context->texImage2DResourceSafeNoClear(GraphicsContext3D::TEXTURE_2D, 0, internalColorFormat, m_size.width(), m_size.height(), 0, colorFormat, GraphicsContext3D::UNSIGNED_BYTE);
> > > > > > 
> > > > > > I would like to change this call to a simple texImage2D with a NULL data parameter.  Can someone please confirm if this will work?
> > > > > 
> > > > > This will break the tests running with in-process gl.
> > > > 
> > > > I think the reason they failed before was that I was relying on the FBO textures being cleared automatically in the in-process version (in the same way they were in the command buffers).
> > > > 
> > > > If we call glClear() the first time the DrawingBuffer is bound, these tests should pass.
> > > 
> > > Also, in the in-process gl, we emit an INVALID_VALUE if passed in data is NULL.  You have to remove that before calling texImage2D with NULL.
> > 
> > Mo's right; I forgot about that check in webgraphicscontext3d_in_process_impl.cc in the Chromium tree.
> 
> Aha, that's what I ran into before (just forgot where it was).
> 
> > I talked with senorblanco about this topic earlier, and suggested we could have another argument to GraphicsContext3D::create which would indicate whether the caller is trusted (compositor, canvas2d) or untrusted (webgl). In the former case we could disable some of these checks.
> > 
> > twiz, i'm not sure what would be the fastest path forward for you, but feel free to make whatever changes you need to in order to make this most efficient, including adding new entry points. I do think that the constructor idea might be the best long-term direction, though.
> 
> I think the constructor param is a good idea.  Since this would require chrome-side changes (as would the in-process change above), perhaps we should split the work into just fixing the layout test, and address the performance issues in a subsequent patch (after the chrome changes).
> 
> My preference would be to land something between revs 2 and 3 of this patch (including the DrawingBuffer::create returning NULL change that jamesr suggested, but excluding the ResourceSafeNoClear changes).  This would fix the layout test, and we can look at improving the performance once the chrome-side changes have landed.
> 
> Does that sound good?

Yes, that sounds very good.  I was hoping it would be an easy change to remove the heavy initialization code, but after all of this discussion, it is obvious that it is a separate issue to address.
Comment 25 Jeff Timanus 2011-04-08 10:56:59 PDT
Created attachment 88842 [details]
Patch
Comment 26 Jeff Timanus 2011-04-08 10:58:12 PDT
Hopefully the last revision.  This final patch only corrects the resizing behaviour, but does not address the performance of the texture construction.
Comment 27 Kenneth Russell 2011-04-08 12:11:23 PDT
Comment on attachment 88842 [details]
Patch

Looks good.
Comment 28 WebKit Commit Bot 2011-04-08 16:01:52 PDT
Comment on attachment 88842 [details]
Patch

Clearing flags on attachment: 88842

Committed r83355: <http://trac.webkit.org/changeset/83355>
Comment 29 WebKit Commit Bot 2011-04-08 16:01:59 PDT
All reviewed patches have been landed.  Closing bug.