[Webkit-unassigned] [Bug 44926] Multiple accelerated 2D canvases should be able to use the same GraphicsContext3D
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Aug 31 12:54:42 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=44926
--- Comment #14 from Vangelis Kokkevis <vangelis at chromium.org> 2010-08-31 12:54:41 PST ---
(From update of attachment 66074)
Looks good! Just a couple of comments, inlined below:
> +void Canvas2DLayerChromium::updateContents()
> +{
> + unsigned textureId = m_framebuffer->textureIdForCompositor();
m_textureChanged isn't set for this layer type. There are two options here:
1. Make sure the proper modes are set when GraphicsContext3D creates the compositor texture so you don't worry about it here
2. Move this logic up to CanvasLayerChromium::draw() and set the modes before using the texture if m_textureChanged == true
I prefer #1 if possible.
> + if (m_textureChanged) {
> + glBindTexture(GL_TEXTURE_2D, textureId);
> + // Set the min-mag filters to linear and wrap modes to GL_CLAMP_TO_EDGE
> + // to get around NPOT texture limitations of GLES.
> + glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
> + glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
> + glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
> + glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
> + m_textureChanged = false;
> + }
> + // Update the contents of the texture used by the compositor.
> + if (m_contentsDirty) {
> + m_framebuffer->swapBuffers();
> + m_contentsDirty = false;
> + }
> +}
> +
> diff --git a/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.h b/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.h
> +
> +// A layer containing an accelerated 2d canvas
> +class Canvas2DLayerChromium : public CanvasLayerChromium {
> +public:
> + static PassRefPtr<Canvas2DLayerChromium> create(CanvasFramebuffer*, GraphicsLayerChromium* owner);
> + virtual bool drawsContent() { return true; }
> + virtual void updateContents();
> +
> + void setCanvasFramebuffer(CanvasFramebuffer*);
> +
> +protected:
> + virtual unsigned textureId();
> +
> +private:
> + explicit Canvas2DLayerChromium(CanvasFramebuffer*, GraphicsLayerChromium* owner);
> + CanvasFramebuffer* m_framebuffer;
> + bool m_textureChanged;
m_textureChanged is already defined in CanvasLayerChromium so no need to re-define it here. Please see comment above.
> +
> + static unsigned m_shaderProgramId;
> +};
> +
> +}
> diff --git a/WebCore/platform/graphics/chromium/CanvasLayerChromium.h b/WebCore/platform/graphics/chromium/CanvasLayerChromium.h
> index 053efffa01e142838121453c1a4b8a194b6fa4c1..d6da6ac44e9107af100e54d3ecae365562de0d2f 100644
> --- a/WebCore/platform/graphics/chromium/CanvasLayerChromium.h
> +++ b/WebCore/platform/graphics/chromium/CanvasLayerChromium.h
> @@ -38,17 +38,13 @@
>
> namespace WebCore {
>
> -class GraphicsContext3D;
> -
> -// A Layer containing a WebGL or accelerated 2d canvas
> +// Base class for WebGL and accelerated 2d canvases.
Should description be:
"Base class for _layers_ containing WebGL and accelerated 2d canvases"?
> class CanvasLayerChromium : public LayerChromium {
> public:
> - static PassRefPtr<CanvasLayerChromium> create(GraphicsLayerChromium* owner = 0);
> - virtual bool drawsContent() { return m_context; }
> - virtual void updateContents();
> - virtual void draw();
> + explicit CanvasLayerChromium(GraphicsLayerChromium* owner);
Shouldn't the constructor be in the private or protected section?
> + virtual ~CanvasLayerChromium();
>
> - void setContext(const GraphicsContext3D* context);
> + virtual void draw();
>
> class SharedValues {
> public:
> @@ -69,21 +65,16 @@ public:
> bool m_initialized;
> };
>
> - class PrepareTextureCallback : public Noncopyable {
> +
> +void WebGLLayerChromium::updateContents()
> +{
> + ASSERT(m_context);
> + if (m_textureChanged) {
See previous comments.
> + glBindTexture(GL_TEXTURE_2D, m_textureId);
> + // Set the min-mag filters to linear and wrap modes to GL_CLAMP_TO_EDGE
> + // to get around NPOT texture limitations of GLES.
> + glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
> + glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
> + glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
> + glTexParameterf(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
> + m_textureChanged = false;
> + }
> + // Update the contents of the texture used by the compositor.
> + if (m_contentsDirty) {
> + m_context->prepareTexture();
> + m_contentsDirty = false;
> + }
> +}
> +
> +void WebGLLayerChromium::setContext(const GraphicsContext3D* context)
> +{
> + m_context = const_cast<GraphicsContext3D*>(context);
> +
> + unsigned int textureId = m_context->platformTexture();
You should probably check for textureId == 0 as that indicates a failure somewhere.
> + if (textureId != m_textureId)
> + m_textureChanged = true;
> + m_textureId = textureId;
> +}
> +
> +}
> diff --git a/WebCore/platform/graphics/gpu/CanvasFramebuffer.h b/WebCore/platform/graphics/gpu/CanvasFramebuffer.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..d52aba1cb68acbced5d34eb6348c5c890333d903
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifndef CanvasFramebuffer_h
> +#define CanvasFramebuffer_h
> +
> +// Manages a framebuffer to use for a 2d canvas.
move the above comment down, right before the class definition?
> +
> +#include "IntSize.h"
> +
> +#include <wtf/Noncopyable.h>
> +#include <wtf/OwnPtr.h>
> +#include <wtf/PassOwnPtr.h>
> +
> +namespace WebCore {
> +
> +class SharedContext3D;
> +
> +class CanvasFramebuffer : public Noncopyable {
> +public:
> + static PassOwnPtr<CanvasFramebuffer> create(SharedContext3D*, const IntSize&);
> + ~CanvasFramebuffer();
> +
> + void bind();
> diff --git a/WebCore/platform/graphics/gpu/SharedContext3D.cpp b/WebCore/platform/graphics/gpu/SharedContext3D.cpp
> new file mode 100644
> index 0000000000000000000000000000000000000000..3b738f01f7281392937fed5e0df257cc9cfb92f4
> --- /dev/null
> +
> +// static
> +PassRefPtr<SharedContext3D> SharedContext3D::create(PassOwnPtr<GraphicsContext3D> context)
> +{
> + return adoptRef(new SharedContext3D(context));
> +}
> +
> +SharedContext3D::SharedContext3D(PassOwnPtr<GraphicsContext3D> context)
> + : m_context(context)
> + , m_quadVertices(0)
> + , m_solidFillShader(SolidFillShader::create(m_context.get()))
> + , m_texShader(TexShader::create(m_context.get()))
> +
Please remove necessary new-line.
> +{
> +}
> +
> +SharedContext3D::~SharedContext3D()
> +{
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list