Bug 57768 – Accelerated Canvas2D path failing when constructing large canvases.

• Component: Canvas
• Status: RESOLVED FIXED

[Jeff Timanus]

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

[Jeff Timanus]

Created attachment 88105 [details]
Patch

[Jeff Timanus]

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 . . .

[Jeff Timanus]

Created attachment 88115 [details]
Patch

[James Robinson]

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.

[Kenneth Russell]

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.

[Jeff Timanus]

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.

[James Robinson]

(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

[Stephen White]

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.)

[James Robinson]

Texture::create() probably should be calling getIntegerv() once and caching the result.  Whoops!

[Kenneth Russell]

(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.

[Jeff Timanus]

Created attachment 88657 [details]
Patch

[Jeff Timanus]

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?

[Kenneth Russell]

(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.

[Zhenyao Mo]

(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.

[Zhenyao Mo]

(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.

[Stephen White]

(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.

[Stephen White]

(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.

[Kenneth Russell]

(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.

[Zhenyao Mo]

(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.

[Zhenyao Mo]

(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.

[Kenneth Russell]

(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.

[Zhenyao Mo]

(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.

[Stephen White]

(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?

[Jeff Timanus]

(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.

[Jeff Timanus]

Created attachment 88842 [details]
Patch

[Jeff Timanus]

Hopefully the last revision.  This final patch only corrects the resizing behaviour, but does not address the performance of the texture construction.

[Kenneth Russell]

Comment on attachment 88842 [details]
Patch

Looks good.

[WebKit Commit Bot]

Comment on attachment 88842 [details]
Patch

Clearing flags on attachment: 88842

Committed r83355: <http://trac.webkit.org/changeset/83355>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.