[Webkit-unassigned] [Bug 44926] Multiple accelerated 2D canvases should be able to use the same GraphicsContext3D

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=44926





--- Comment #12 from Stephen White <senorblanco at chromium.org>  2010-08-31 11:30:31 PST ---
> WebCore/html/canvas/CanvasRenderingContext2D.cpp:1283
>      sourceCanvas->makeRenderingResultsAvailable();
Are you sure it isn't necessary to do something here?  Obviously, reading back to the buffer isn't right in this case, but we need some way to make sure that the sourceCanvas is up-to-date (e.g., last software draw is uploaded to framebuffer) before we draw.

And actually, I think this will break the software path, since it needs to make this call.  If the GPU truly doesn't need this, it should probably be doing an !isAccelerated() check instead.

(Reading on a bit further, it seems you're doing the GPU equivalent of this call in drawImageBuffer().  Fair enough, although it would be kind of cool to unify them in the same makeRenderingResultsAvailable() function.  And you'll still need to fix the software path).

> WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:38
> +#include "PlatformContextSkia.h"
Is this #include really needed?

> WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:63
> +        glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
Should be glTexParameteri (someone really ought to fix the original too).

> WebCore/platform/graphics/chromium/GLES2Canvas.cpp:130
> +    m_context->useFillSolidProgram(color, matrix);
Please use matrix, color order -- this matches the order in the shader code.

> WebCore/platform/graphics/chromium/GLES2Canvas.cpp:232
> +    drawQuad(IntSize(tileBoundsWithBorder.width(), tileBoundsWithBorder.height()), srcRectClippedInTileSpace, dstRectIntersected, transform, alpha);
I think tileBoundsWithBorder.size() should work here, rather than constructing your own IntSize().

> WebCore/platform/graphics/chromium/GLES2Canvas.cpp:249
> +    m_context->useTextureProgram(alpha, matrix, texMatrix);
Should be matrix, texMatrix, alpha (to be consistent w/shader).

> WebCore/platform/graphics/chromium/GLES2Canvas.cpp:261
> +    return m_context->createTexture(ptr, format, width, height);
OT:  Now that uploaded textures are shared between all contexts, I think we could actually get rid of the hash table entirely, and put a RefPtr<Texture> in the NativeImageSkia itself, if we wanted to.  This should help tie its lifetime to that of the image cache.

> WebCore/platform/graphics/chromium/GLES2Canvas.cpp:266
> +    return m_context->getTexture(ptr);
Does this function really need to transfer ownership?  If not, it should be passing a bare ptr, as it was before (see http://webkit.org/coding/RefPtr.html).

> WebCore/platform/graphics/chromium/GLES2Canvas.h:79
> +    PassRefPtr<Texture> getTexture(NativeImagePtr);
Again, since the ownership is not transferred, these should be bare pointers.

> WebCore/platform/graphics/chromium/GLES2Canvas.h:87
> +    unsigned textureIdForCanvas() const;
This will all have to change once the compositor supports texture tiling, but I'm sure you knew that.

> WebCore/platform/graphics/chromium/WebGLLayerChromium.cpp:63
> +        glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
glTexParameteri (not your fault, just hoping someone finally fixes this).

> WebCore/platform/graphics/gpu/CanvasFramebuffer.cpp:67
> +    m_context->copyTextureToCompositor(m_offscreenColorTexture, m_offscreenParentColorTexture);
As discussed, I would have preferred that this end up in the same code as WebGL on the GPU process side, but that can be refactored later.

> WebCore/platform/graphics/gpu/SharedContext3D.cpp:61
> +}
This looks like it's unused.

> WebCore/platform/graphics/gpu/SharedContext3D.cpp:278
> +        m_context->bindTexture(target, texture);
This looks superfluous -- same code as below.

> WebCore/platform/graphics/gpu/SharedContext3D.cpp:285
> +void SharedContext3D::useFillSolidProgram(const Color& color, const AffineTransform& transform)
Please switch these back so they match the shader.

> WebCore/platform/graphics/gpu/SharedContext3D.cpp:290
> +void SharedContext3D::useTextureProgram(float alpha, const AffineTransform& transform, const AffineTransform& texTransform)
Ibid.

> WebCore/platform/graphics/gpu/SharedContext3D.h:87
> +    bool supportsBGRA();
Is this needed?  It looks like Texture is unchanged, so it'll still call GraphicsContext3D::supportsBGRA() (although I could be wrong).

> WebCore/platform/graphics/skia/PlatformContextSkia.cpp:791
> +        m_uploadTexture = context->createTexture(0, Texture::BGRA8, bitmap.width(), bitmap.height());
Hmm.. hashing on the NULL ptr (besides making it "magic" and fragile, in case anyone else tries the same trick) will share a single upload texture for all canvases.  This means we might hang onto an 8K texture long after a big canvas is gone, and we only need to upload little 150x150 canvases, e.g.

> WebCore/platform/graphics/skia/PlatformContextSkia.h:184
> +    void setSharedContext3D(SharedContext3D*, CanvasFramebuffer*, const IntSize&);
I'd rather the CanvasFramebuffer be owned by the GLES2Canvas, if possible.  But we can talk about that later.

> WebKit/chromium/src/WebViewImpl.cpp:2220
> +        attr.stencil = true;
Nit:  probably not much point in requesting stencil, since it looks like your FBO implementation doesn't support it.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.