Bug 43656 - [chromium] Add a PrepareTextureCallback to the chromium canvas layer compositor to upload mixed-mode results before compositing
Summary: [chromium] Add a PrepareTextureCallback to the chromium canvas layer composit...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on: 43734
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-06 17:25 PDT by James Robinson
Modified: 2010-08-09 14:22 PDT (History)
6 users (show)

See Also:


Attachments
Patch (3.24 KB, patch)
2010-08-06 17:28 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (5.30 KB, patch)
2010-08-06 19:04 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (5.23 KB, patch)
2010-08-09 13:19 PDT, James Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2010-08-06 17:25:46 PDT
[chromium] Add a PrepareTextureCallback to the chromium canvas layer compositor to upload mixed-mode results before compositing
Comment 1 James Robinson 2010-08-06 17:28:16 PDT
Created attachment 63787 [details]
Patch
Comment 2 WebKit Review Bot 2010-08-06 17:44:50 PDT
Attachment 63787 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3616690
Comment 3 James Robinson 2010-08-06 17:55:22 PDT
I think the EWS bot's a bit behind ToT.  This builds on chromium linux on my box.
Comment 4 Vangelis Kokkevis 2010-08-06 18:19:12 PDT
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?
Comment 5 James Robinson 2010-08-06 18:23:36 PDT
Comment on attachment 63787 [details]
Patch

Good point.  I'll update the patch to clear the pointer out when the PCS dies.
Comment 6 James Robinson 2010-08-06 19:04:11 PDT
Created attachment 63794 [details]
Patch
Comment 7 WebKit Review Bot 2010-08-06 19:49:04 PDT
Attachment 63794 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3567981
Comment 8 Vangelis Kokkevis 2010-08-09 10:42:21 PDT
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  <jamesr@chromium.org>
>  
> +        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):
> +
> +2010-08-06  James Robinson  <jamesr@chromium.org>
> +
>          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
Comment 9 Dimitri Glazkov (Google) 2010-08-09 10:46:00 PDT
Comment on attachment 63794 [details]
Patch

ok.
Comment 10 James Robinson 2010-08-09 11:06:03 PDT
Committed r64993: <http://trac.webkit.org/changeset/64993>
Comment 11 WebKit Review Bot 2010-08-09 11:16:34 PDT
http://trac.webkit.org/changeset/64993 might have broken Chromium Linux Release
Comment 12 James Robinson 2010-08-09 13:19:45 PDT
Created attachment 63923 [details]
Patch
Comment 13 James Robinson 2010-08-09 13:21:57 PDT
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 :).
Comment 14 Dimitri Glazkov (Google) 2010-08-09 13:22:54 PDT
Comment on attachment 63923 [details]
Patch

ok.
Comment 15 James Robinson 2010-08-09 13:54:34 PDT
Comment on attachment 63923 [details]
Patch

Clearing flags on attachment: 63923

Committed r65001: <http://trac.webkit.org/changeset/65001>
Comment 16 James Robinson 2010-08-09 13:54:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 WebKit Review Bot 2010-08-09 14:11:19 PDT
http://trac.webkit.org/changeset/65001 might have broken Chromium Win Release
Comment 18 James Robinson 2010-08-09 14:22:33 PDT
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.