Bug 43656 – [chromium] Add a PrepareTextureCallback to the chromium canvas layer compositor to upload mixed-mode results before compositing

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[chromium] Add a PrepareTextureCallback to the chromium canvas layer compositor to upload mixed-mode results before compositing

[James Robinson]

Created attachment 63787 [details]
Patch

[WebKit Review Bot]

Attachment 63787 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3616690

[James Robinson]

I think the EWS bot's a bit behind ToT.  This builds on chromium linux on my box.

[Vangelis Kokkevis]

Comment on attachment 63787 [details]
Patch

> diff --git a/WebCore/platform/graphics/chromium/CanvasLayerChromium.cpp b/WebCore/platform/graphics/chromium/CanvasLayerChromium.cpp
> index f290e680d30909f97e34553a53785c4918d2b88a..7e732d13ee829fcf8e3c8fa93fd6a20ee1decdcd 100644
> --- a/WebCore/platform/graphics/chromium/CanvasLayerChromium.cpp
> +++ b/WebCore/platform/graphics/chromium/CanvasLayerChromium.cpp
> @@ -75,6 +75,8 @@ void CanvasLayerChromium::updateTextureContents(unsigned textureId)
>      }
>      // Update the contents of the texture used by the compositor.
>      if (m_contentsDirty) {
> +        if (m_prepareTextureCallback)
> +            m_prepareTextureCallback->willPrepareTexture();
>          m_context->prepareTexture();
>          m_contentsDirty = false;
>      }
> diff --git a/WebCore/platform/graphics/chromium/CanvasLayerChromium.h b/WebCore/platform/graphics/chromium/CanvasLayerChromium.h
> index a0025a46dd809d4367148293a43def3b84dd1f7e..98be2704e156b8eaf282d66a804a42ab21818f9b 100644
> --- a/WebCore/platform/graphics/chromium/CanvasLayerChromium.h
> +++ b/WebCore/platform/graphics/chromium/CanvasLayerChromium.h
> @@ -54,11 +54,18 @@ public:
>  
>      static void setShaderProgramId(unsigned shaderProgramId) { m_shaderProgramId = shaderProgramId; }
>  
> +    class PrepareTextureCallback : public Noncopyable {
> +    public:
> +        virtual void willPrepareTexture() = 0;
> +    };
> +    void setPrepareTextureCallback(PassOwnPtr<PrepareTextureCallback> callback) { m_prepareTextureCallback = callback; }
> +
>  private:
>      explicit CanvasLayerChromium(GraphicsLayerChromium* owner);
>      GraphicsContext3D* m_context;
>      unsigned m_textureId;
>      bool m_textureChanged;
> +    OwnPtr<PrepareTextureCallback> m_prepareTextureCallback;
>  
>      static unsigned m_shaderProgramId;
>  };
> diff --git a/WebCore/platform/graphics/skia/PlatformContextSkia.cpp b/WebCore/platform/graphics/skia/PlatformContextSkia.cpp
> index b14c6cd04ca30bd417102e4357063beacae222cb..7b1eb1ab04d16ba32953fdf697aece885b8f4c71 100644
> --- a/WebCore/platform/graphics/skia/PlatformContextSkia.cpp
> +++ b/WebCore/platform/graphics/skia/PlatformContextSkia.cpp
> @@ -33,6 +33,7 @@
>  #include "PlatformContextSkia.h"
>  
>  #include "AffineTransform.h"
> +#include "CanvasLayerChromium.h"
>  #include "GraphicsContext.h"
>  #include "ImageBuffer.h"
>  #include "NativeImageSkia.h"
> @@ -678,10 +679,28 @@ void PlatformContextSkia::applyAntiAliasedClipPaths(WTF::Vector<SkPath>& paths)
>  
>  #if USE(GLES2_RENDERING)
>  
> +class PrepareTextureCallbackImpl : public CanvasLayerChromium::PrepareTextureCallback {
> +public:
> +    static PassOwnPtr<PrepareTextureCallbackImpl> create(PlatformContextSkia* pcs)
> +    {
> +        return new PrepareTextureCallbackImpl(pcs);
> +    }
> +
> +    virtual void willPrepareTexture()
> +    {
> +        m_pcs->prepareForHardwareDraw();
> +    }
> +private:
> +    explicit PrepareTextureCallbackImpl(PlatformContextSkia* pcs) : m_pcs(pcs) {}
> +    PlatformContextSkia* m_pcs;
> +};
> +
>  void PlatformContextSkia::setGraphicsContext3D(GraphicsContext3D* context, const WebCore::IntSize& size)
>  {
>      m_useGPU = true;
>      m_gpuCanvas = new GLES2Canvas(context, size);
> +    CanvasLayerChromium* layer = static_cast<CanvasLayerChromium*>(context->platformLayer());
> +    layer->setPrepareTextureCallback(PrepareTextureCallbackImpl::create(this));
>  }
>  
>  void PlatformContextSkia::prepareForSoftwareDraw() const

WebCore/platform/graphics/skia/PlatformContextSkia.cpp:703
 +      layer->setPrepareTextureCallback(PrepareTextureCallbackImpl::create(this));
Is it ever possible that the CanvasLayerChromium will outlive the PlatformContextSkia and the compositor will end up calling a method on a dead pointer?

[James Robinson]

Comment on attachment 63787 [details]
Patch

Good point.  I'll update the patch to clear the pointer out when the PCS dies.

[James Robinson]

Created attachment 63794 [details]
Patch

[WebKit Review Bot]

Attachment 63794 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3567981

[Vangelis Kokkevis]

Comment on attachment 63794 [details]
Patch

Looks good.


> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 2b756d68f13f9359c4466d195171b923c722bae3..ed63a02cb96b6073d1aabd3cbd71c4948abd604b 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,5 +1,30 @@
>  2010-08-06  James Robinson  <[e-mail]>
>  
> +        Reviewed by NOBODY (OOPS!).
> +
> +        [chromium] Add a PrepareTextureCallback to the chromium canvas layer compositor to upload mixed-mode results before compositing
> +        https://bugs.webkit.org/show_bug.cgi?id=43656
> +
> +        When compositing an accelerated canvas that is using both hardware and software drawing,
> +        we need a callback before compositing the layer to make sure that we upload any software
> +        drawn results to the texture.  This will go away as soon as implement all draw calls
> +        in hardware.
> +
> +        To test, run any canvas demo that runs in mixed mode and verifies that the software results
> +        always show up.
> +
> +        * platform/graphics/chromium/CanvasLayerChromium.cpp:
> +        (WebCore::CanvasLayerChromium::updateTextureContents):
> +        * platform/graphics/chromium/CanvasLayerChromium.h:
> +        (WebCore::CanvasLayerChromium::setPrepareTextureCallback):
> +        * platform/graphics/skia/PlatformContextSkia.cpp:
> +        (WebCore::PrepareTextureCallbackImpl::create):
> +        (WebCore::PrepareTextureCallbackImpl::willPrepareTexture):
> +        (WebCore::PrepareTextureCallbackImpl::PrepareTextureCallbackImpl):
> +        (WebCore::PlatformContextSkia::setGraphicsContext3D):
> +
> [phone]  James Robinson  <[e-mail]>
> +
>          Reviewed by Simon Fraser.
>  
>          Accelerated 2d canvases should get compositing layers
> diff --git a/WebCore/platform/graphics/chromium/CanvasLayerChromium.cpp b/WebCore/platform/graphics/chromium/CanvasLayerChromium.cpp
> index f290e680d30909f97e34553a53785c4918d2b88a..7e732d13ee829fcf8e3c8fa93fd6a20ee1decdcd 100644
> --- a/WebCore/platform/graphics/chromium/CanvasLayerChromium.cpp
> +++ b/WebCore/platform/graphics/chromium/CanvasLayerChromium.cpp
> @@ -75,6 +75,8 @@ void CanvasLayerChromium::updateTextureContents(unsigned textureId)
>      }
>      // Update the contents of the texture used by the compositor.
>      if (m_contentsDirty) {
> +        if (m_prepareTextureCallback)
> +            m_prepareTextureCallback->willPrepareTexture();
>          m_context->prepareTexture();
>          m_contentsDirty = false;
>      }
> diff --git a/WebCore/platform/graphics/chromium/CanvasLayerChromium.h b/WebCore/platform/graphics/chromium/CanvasLayerChromium.h
> index a0025a46dd809d4367148293a43def3b84dd1f7e..98be2704e156b8eaf282d66a804a42ab21818f9b 100644
> --- a/WebCore/platform/graphics/chromium/CanvasLayerChromium.h
> +++ b/WebCore/platform/graphics/chromium/CanvasLayerChromium.h
> @@ -54,11 +54,18 @@ public:
>  
>      static void setShaderProgramId(unsigned shaderProgramId) { m_shaderProgramId = shaderProgramId; }
>  
> +    class PrepareTextureCallback : public Noncopyable {
> +    public:
> +        virtual void willPrepareTexture() = 0;
> +    };
> +    void setPrepareTextureCallback(PassOwnPtr<PrepareTextureCallback> callback) { m_prepareTextureCallback = callback; }
> +
>  private:
>      explicit CanvasLayerChromium(GraphicsLayerChromium* owner);
>      GraphicsContext3D* m_context;
>      unsigned m_textureId;
>      bool m_textureChanged;
> +    OwnPtr<PrepareTextureCallback> m_prepareTextureCallback;
>  
>      static unsigned m_shaderProgramId;
>  };
> diff --git a/WebCore/platform/graphics/skia/PlatformContextSkia.cpp b/WebCore/platform/graphics/skia/PlatformContextSkia.cpp
> index b14c6cd04ca30bd417102e4357063beacae222cb..0dfe494ad786818138de194799dfa6ca56cf0be9 100644
> --- a/WebCore/platform/graphics/skia/PlatformContextSkia.cpp
> +++ b/WebCore/platform/graphics/skia/PlatformContextSkia.cpp
> @@ -33,6 +33,7 @@
>  #include "PlatformContextSkia.h"
>  
>  #include "AffineTransform.h"
> +#include "CanvasLayerChromium.h"
>  #include "GraphicsContext.h"
>  #include "ImageBuffer.h"
>  #include "NativeImageSkia.h"
> @@ -220,6 +221,12 @@ PlatformContextSkia::PlatformContextSkia(skia::PlatformCanvas* canvas)
>  
>  PlatformContextSkia::~PlatformContextSkia()
>  {
> +#if USE(GLES2_RENDERING)
> +    if (m_gpuCanvas) {
> +        CanvasLayerChromium* layer = static_cast<CanvasLayerChromium*>(m_gpuCanvas->context()->platformLayer());
> +        layer->setPrepareTextureCallback(0);
> +    }
> +#endif
>  }
>  
>  void PlatformContextSkia::setCanvas(skia::PlatformCanvas* canvas)
> @@ -678,10 +685,28 @@ void PlatformContextSkia::applyAntiAliasedClipPaths(WTF::Vector<SkPath>& paths)
>  
>  #if USE(GLES2_RENDERING)
>  
> +class PrepareTextureCallbackImpl : public CanvasLayerChromium::PrepareTextureCallback {
> +public:
> +    static PassOwnPtr<PrepareTextureCallbackImpl> create(PlatformContextSkia* pcs)
> +    {
> +        return new PrepareTextureCallbackImpl(pcs);
> +    }
> +
> +    virtual void willPrepareTexture()
> +    {
> +        m_pcs->prepareForHardwareDraw();
> +    }
> +private:
> +    explicit PrepareTextureCallbackImpl(PlatformContextSkia* pcs) : m_pcs(pcs) {}
> +    PlatformContextSkia* m_pcs;
> +};
> +
>  void PlatformContextSkia::setGraphicsContext3D(GraphicsContext3D* context, const WebCore::IntSize& size)
>  {
>      m_useGPU = true;
>      m_gpuCanvas = new GLES2Canvas(context, size);
> +    CanvasLayerChromium* layer = static_cast<CanvasLayerChromium*>(context->platformLayer());
> +    layer->setPrepareTextureCallback(PrepareTextureCallbackImpl::create(this));
>  }
>  
>  void PlatformContextSkia::prepareForSoftwareDraw() const

[Dimitri Glazkov (Google)]

Comment on attachment 63794 [details]
Patch

ok.

[James Robinson]

Committed r64993: <http://trac.webkit.org/changeset/64993>

[WebKit Review Bot]

http://trac.webkit.org/changeset/64993 might have broken Chromium Linux Release

[James Robinson]

Created attachment 63923 [details]
Patch

[James Robinson]

The last patch left out a #include in PlatformContextSkia.cpp that I had locally (which explains why I could compile locally but the EWS bot failed).  This one should be good :).

[Dimitri Glazkov (Google)]

Comment on attachment 63923 [details]
Patch

ok.

[James Robinson]

Comment on attachment 63923 [details]
Patch

Clearing flags on attachment: 63923

Committed r65001: <http://trac.webkit.org/changeset/65001>

[James Robinson]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/65001 might have broken Chromium Win Release

[James Robinson]

Compile fixed at r65002.  The basic issue was that the b.w.o bots compile with USE(ACCELERATED_COMPOSITING) off, whereas the chromium compiles (both on the canaries and our workstations) compile with that on.  I just had to make it compile with both settings.