[EFL][WK2] Implement Accelerated2DCanvas on WK2 Efl port

• Component: WebKit EFL
• Status: RESOLVED WONTFIX

[Kyungjin Kim]

This implementation is based on COORDINATED_GRAPHICS in case of not using GRAPHICS_SURFACE because it is not enabled by default in EFL. 
The graphics surface version of Accelerated2DCanvas will be implemented at some time.

[Kyungjin Kim]

Created attachment 176882 [details]
Patch

[Jinwoo Song]

Comment on attachment 176882 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176882&action=review

> Source/WebCore/platform/graphics/efl/Canvas2DLayerEfl.cpp:19
> +

We prefer BSD license and add '*' at the comment line. Please refer to ewk_defines.h file.

> Source/WebCore/platform/graphics/efl/Canvas2DLayerEfl.h:19
> +

ditto.

[Kyungjin Kim]

Created attachment 176895 [details]
Patch

[Kyungjin Kim]

(In reply to comment #2)
> (From update of attachment 176882 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176882&action=review
> 
> > Source/WebCore/platform/graphics/efl/Canvas2DLayerEfl.cpp:19
> > +
> 
> We prefer BSD license and add '*' at the comment line. Please refer to ewk_defines.h file.
> 
> > Source/WebCore/platform/graphics/efl/Canvas2DLayerEfl.h:19
> > +
> 
> ditto.

Canvas2DLayerEfl inherited from TextureMapperPlatformLayer which is GNU and it's same as other inherited classes such as GraphicsContext3DPrivate, MediaPlayerPrivate.
Do I really need to change it to BSD license?

[Kyungjin Kim]

Created attachment 176900 [details]
Patch

[Kyungjin Kim]

Created attachment 176902 [details]
Patch

[Noam Rosenthal]

Comment on attachment 176902 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176902&action=review

This is a good first attempt :)
However a lot of the choices in this patch don't explain themselves, and there are no comments in the changelog or in the code to explain them...

> Source/WebCore/ChangeLog:10
> +        Implement accelerated 2D canvas using Coordinated Graphics on WK2 Efl port.
> +        This implementation is based on COORDINATED_GRAPHICS.
> +

Needs more information.

> Source/WebCore/platform/graphics/efl/Canvas2DLayerEfl.h:51
> +#else
> +    void paintContents(GraphicsContext&, const IntRect&);
> +#endif

This shouldn't really be inside the #else clause.
There could be a situation where graphics surfaces are compiled in, but allocating a graphisc surface didn't succeed, in which case this layer should still paint with software.

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:50
> +#if ENABLE(ACCELERATED_2D_CANVAS)
> +    virtual void paintContents(GraphicsContext&, const IntRect&) { }
> +    virtual bool is2D() { return false; }
> +#endif

This is a bit confusing.
Perhaps hasGraphicsSurface() would be a better choice, that always returns false if ENABLE(GRAPHICS_SURFACE) is false.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:682
> +#if ENABLE(ACCELERATED_2D_CANVAS) && !USE(GRAPHICS_SURFACE)
> +    if (m_canvasPlatformLayer && m_canvasPlatformLayer->is2D()) {
> +        m_canvasPlatformLayer->paintContents(*context, rect);
> +        return;
> +    }
> +#endif

How about
if (m_canvasPlatformLayer && !m_canvasPlatformLayer->hasGraphicsSurface())
   ... paintContents() ...

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:766
> +#if ENABLE(ACCELERATED_2D_CANVAS) && !USE(GRAPHICS_SURFACE)
> +    if (m_canvasPlatformLayer && m_canvasPlatformLayer->is2D())
> +        setDrawsContent(true);
> +#endif

This is quite strange.
Hard for me to understand why you would want an accelerated 2d canvas layer with no graphics surface; which would essentially mean it's not accelerated.
I'm willing to listen to reason but there is no explanation in the Changelog :)

[Chris Dumez]

View in context: https://bugs.webkit.org/attachment.cgi?id=176902&action=review

> Source/WebCore/platform/graphics/ImageBuffer.h:124
> +        cairo_surface_t* getSurface() const { return m_data.m_surface; }

We usually name getters such as surface() directly, not getSurface().
Also, the coding style was recently altered so that functions returning non-const pointers should not be const.

> Source/WebCore/platform/graphics/efl/Canvas2DLayerEfl.cpp:29
> +PassRefPtr<Canvas2DLayerEfl> Canvas2DLayerEfl::create(ImageBuffer* buffer, IntSize& size)

size argument should probably be const

> Source/WebCore/platform/graphics/efl/Canvas2DLayerEfl.cpp:34
> +Canvas2DLayerEfl::Canvas2DLayerEfl(ImageBuffer* buffer, IntSize& size)

Ditto.

> Source/WebCore/platform/graphics/efl/Canvas2DLayerEfl.h:37
> +    , public RefCounted<Canvas2DLayerEfl> {

Looks like the comma should be inside the #if otherwise it won't build if TEXTURE_MAPPER_GL is not used.

[Kenneth Rohde Christiansen]

(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 176882 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=176882&action=review
> > 
> > > Source/WebCore/platform/graphics/efl/Canvas2DLayerEfl.cpp:19
> > > +
> > 
> > We prefer BSD license and add '*' at the comment line. Please refer to ewk_defines.h file.
> > 
> > > Source/WebCore/platform/graphics/efl/Canvas2DLayerEfl.h:19
> > > +
> > 
> > ditto.
> 
> Canvas2DLayerEfl inherited from TextureMapperPlatformLayer which is GNU and it's same as other inherited classes such as GraphicsContext3DPrivate, MediaPlayerPrivate.
> Do I really need to change it to BSD license?

We prefer BSD, it also means that during refactoring we can actually reuse this code in other files. This is very important.

[Kenneth Rohde Christiansen]

Comment on attachment 176902 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=176902&action=review

On a side node: some work has been ongoing in webkit, so make sure that when the deviceScaleFactor != 1, we don't end up scaling the canvas. Are you aware of this work and how does it work with your implementation

> Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:85
> +    , m_accelerated(false)

isAccelerated is used elsewhere

> Source/WebCore/platform/graphics/cairo/PlatformContextCairo.h:71
> +    void setAccelerated(bool accelerated) { m_accelerated = accelerated; }

Why not be consistent with other ports?

GraphicsContext::setIsAcceleratedContext(bool isAccelerated)
Source/WebCore/platform/graphics/GraphicsContext.h:        void setIsAcceleratedContext(bool);

> Source/WebCore/platform/graphics/efl/Canvas2DLayerEfl.cpp:34
> +Canvas2DLayerEfl::Canvas2DLayerEfl(ImageBuffer* buffer, IntSize& size)

why not const IntSize& ?

> Source/WebCore/platform/graphics/efl/Canvas2DLayerEfl.h:33
> +
> +class Canvas2DLayerEfl :

Is this really EFL specific or cairo specific?

[Kenneth Rohde Christiansen]

Comment on attachment 176902 [details]
Patch

Where is the code that makes this use the cairo gl backend?

[Kyungjin Kim]

(In reply to comment #11)
> (From update of attachment 176902 [details])
> Where is the code that makes this use the cairo gl backend?

It is not decided to be opened yet.

[Kyungjin Kim]

(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 176902 [details] [details])
> > Where is the code that makes this use the cairo gl backend?
> 
> It is not decided to be opened yet.
It is not decided to be updated yet.
GL calls could be included in graphics surface version of Accelerated2DCanvas.

[Kyungjin Kim]

(In reply to comment #10)
> (From update of attachment 176902 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176902&action=review
> 
> On a side node: some work has been ongoing in webkit, so make sure that when the deviceScaleFactor != 1, we don't end up scaling the canvas. Are you aware of this work and how does it work with your implementation

Sorry, I'm not because ImageBufferCairo doesn't care the deviceScaleFactor currently.
> 
> > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.cpp:85
> > +    , m_accelerated(false)
> 
> isAccelerated is used elsewhere
> 
> > Source/WebCore/platform/graphics/cairo/PlatformContextCairo.h:71
> > +    void setAccelerated(bool accelerated) { m_accelerated = accelerated; }
> 
> Why not be consistent with other ports?
> 
> GraphicsContext::setIsAcceleratedContext(bool isAccelerated)
> Source/WebCore/platform/graphics/GraphicsContext.h:        void setIsAcceleratedContext(bool);

They are in consistency with PlatformContextSkia.h and GraphicsCotnextSkia.cpp. Actually they are exactly same as skia port.
> 
> > Source/WebCore/platform/graphics/efl/Canvas2DLayerEfl.cpp:34
> > +Canvas2DLayerEfl::Canvas2DLayerEfl(ImageBuffer* buffer, IntSize& size)
> 
> why not const IntSize& ?

Thanks for your comment(same as Christophe's)
> 
> > Source/WebCore/platform/graphics/efl/Canvas2DLayerEfl.h:33
> > +
> > +class Canvas2DLayerEfl :
> 
> Is this really EFL specific or cairo specific?

EFL specific.

[Kyungjin Kim]

(In reply to comment #8)
> View in context: https://bugs.webkit.org/attachment.cgi?id=176902&action=review
> 
> > Source/WebCore/platform/graphics/ImageBuffer.h:124
> > +        cairo_surface_t* getSurface() const { return m_data.m_surface; }
> 
> We usually name getters such as surface() directly, not getSurface().
> Also, the coding style was recently altered so that functions returning non-const pointers should not be const.
> 
> > Source/WebCore/platform/graphics/efl/Canvas2DLayerEfl.cpp:29
> > +PassRefPtr<Canvas2DLayerEfl> Canvas2DLayerEfl::create(ImageBuffer* buffer, IntSize& size)
> 
> size argument should probably be const
> 
> > Source/WebCore/platform/graphics/efl/Canvas2DLayerEfl.cpp:34
> > +Canvas2DLayerEfl::Canvas2DLayerEfl(ImageBuffer* buffer, IntSize& size)
> 
> Ditto.
> 
> > Source/WebCore/platform/graphics/efl/Canvas2DLayerEfl.h:37
> > +    , public RefCounted<Canvas2DLayerEfl> {
> 
> Looks like the comma should be inside the #if otherwise it won't build if TEXTURE_MAPPER_GL is not used.

all done. Thanks for your comments.

[Kyungjin Kim]

(In reply to comment #9)
> (In reply to comment #4)
> > (In reply to comment #2)
> > > (From update of attachment 176882 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=176882&action=review
> > > 
> > > > Source/WebCore/platform/graphics/efl/Canvas2DLayerEfl.cpp:19
> > > > +
> > > 
> > > We prefer BSD license and add '*' at the comment line. Please refer to ewk_defines.h file.
> > > 
> > > > Source/WebCore/platform/graphics/efl/Canvas2DLayerEfl.h:19
> > > > +
> > > 
> > > ditto.
> > 
> > Canvas2DLayerEfl inherited from TextureMapperPlatformLayer which is GNU and it's same as other inherited classes such as GraphicsContext3DPrivate, MediaPlayerPrivate.
> > Do I really need to change it to BSD license?
> 
> We prefer BSD, it also means that during refactoring we can actually reuse this code in other files. This is very important.

It's been changed. Thanks for the kind explanation.

[Kyungjin Kim]

(In reply to comment #7)
> (From update of attachment 176902 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176902&action=review
> 
> This is a good first attempt :)
> However a lot of the choices in this patch don't explain themselves, and there are no comments in the changelog or in the code to explain them...
> 
> > Source/WebCore/ChangeLog:10
> > +        Implement accelerated 2D canvas using Coordinated Graphics on WK2 Efl port.
> > +        This implementation is based on COORDINATED_GRAPHICS.
> > +
> 
> Needs more information.

I updated ChangeLog for more information.
> 
> > Source/WebCore/platform/graphics/efl/Canvas2DLayerEfl.h:51
> > +#else
> > +    void paintContents(GraphicsContext&, const IntRect&);
> > +#endif
> 
> This shouldn't really be inside the #else clause.
> There could be a situation where graphics surfaces are compiled in, but allocating a graphisc surface didn't succeed, in which case this layer should still paint with software.

done.
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:50
> > +#if ENABLE(ACCELERATED_2D_CANVAS)
> > +    virtual void paintContents(GraphicsContext&, const IntRect&) { }
> > +    virtual bool is2D() { return false; }
> > +#endif
> 
> This is a bit confusing.
> Perhaps hasGraphicsSurface() would be a better choice, that always returns false if ENABLE(GRAPHICS_SURFACE) is false.
> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:682
> > +#if ENABLE(ACCELERATED_2D_CANVAS) && !USE(GRAPHICS_SURFACE)
> > +    if (m_canvasPlatformLayer && m_canvasPlatformLayer->is2D()) {
> > +        m_canvasPlatformLayer->paintContents(*context, rect);
> > +        return;
> > +    }
> > +#endif
> 
> How about
> if (m_canvasPlatformLayer && !m_canvasPlatformLayer->hasGraphicsSurface())
>    ... paintContents() ...

Thanks for your suggestion. I made hasGraphicsSurface() as a virtual to be handled in Canvas2DLayerEfl because I'm worried if it might be able to be executed for unwanted layers, feed me back please.
> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:766
> > +#if ENABLE(ACCELERATED_2D_CANVAS) && !USE(GRAPHICS_SURFACE)
> > +    if (m_canvasPlatformLayer && m_canvasPlatformLayer->is2D())
> > +        setDrawsContent(true);
> > +#endif
> 
> This is quite strange.
> Hard for me to understand why you would want an accelerated 2d canvas layer with no graphics surface; which would essentially mean it's not accelerated.
> I'm willing to listen to reason but there is no explanation in the Changelog :)

We are planning to implement Accelerated2DCanvas on WK2 Efl port based on COORDINATED_GRAPHICS for both cases using GRAPHICS_SURFACE and no GRAPHICS_SURFACE because it is not enabled by default in EFL currently. 
Graphics surface version of Accelerated2DCanvas will be implemented soon.
I updated ChangeLog.

[Kyungjin Kim]

Created attachment 177177 [details]
Patch

[Noam Rosenthal]

Comment on attachment 177177 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177177&action=review

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:45
> +    virtual bool hasGraphicsSurface() const { return true; }

I think it's better if this function returned true when GRAPHICS_SURFACE is on, and false when off, like what you did inside Canvas2DLayerEfl.cpp. 
Maybe Zeno has a different idea though :)

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:678
> +        m_canvasPlatformLayer->paintContents(*context, rect);

This is not right; 
You should paint the canvas' content after painting the regular content, and adjust it to contentsRect().
Otherwise the canvas'  background and borders would disappear.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:763
> +    if (m_canvasPlatformLayer && !m_canvasPlatformLayer->hasGraphicsSurface())
> +        setDrawsContent(true);

I don't like setting drawsContent from within CoordinatedGraphicsLayer.
Better to refactor the functions that use it, like shouldHaveBackingStore()

[Kalyan]

(In reply to comment #1)
> Created an attachment (id=176882) [details]
> Patch

(In reply to comment #18)
> Created an attachment (id=177177) [details]
> Patch

We should also check for ENABLE(ACCELERATED_2D_CANVAS) when returning platformLayer. As I see it, we don't even create or declare the layer otherwise.
Same at other places where layer comes into play.

for example:

#if USE(ACCELERATED_COMPOSITING)
 PlatformLayer* ImageBuffer::platformLayer() const
 {
 #if ENABLE(ACCELERATED_2D_CANVAS) && PLATFORM(EFL)  
  return static_cast<TextureMapperPlatformLayer*>(m_data.m_platformLayer.get());
 #else
  return 0;
 #endif
 }
 #endif 

Currently we expose the surface through IMageB

[Kalyan]

(In reply to comment #20)

> Currently we expose the surface through IMageB
sorry, ignore this line.

hasGraphicsSurface() -> perhaps useGraphicsSurface  or useShareableSurface ??
useGraphicsSurface would sync in with the checks already in place

[Kalyan]

(In reply to comment #18)
> Created an attachment (id=177177) [details]
> Patch

Canvas2DLayerEfl.h  -> There isnt any things specific to EFL(Atleast from the code I see here). 

Two different approaches we could take:
1)As I see it, we can rename it for something more generic (i.e Canvas2DAcceleratedLayer or any other generic name) which could be reused in other places too (see my comments below, expanding on this front.)
2)Can/should we extend PlatformContextCairo to handle this functionality. 

There are common things between GraphicsContext3DPrivate and this class which could be shared. It would be good for us to have a base class which encapsulates the common functionality and than class deriving from it to handle 2DCanvas and 3D Canvas specific stuff. I guess this is out of scope for this changeset but we should do this at some point.

>>for both cases using GRAPHICS_SURFACE and no GRAPHICS_SURFACE because it is >>not enabled by default in EFL currently.
>>  Graphics surface version of Accelerated2DCanvas will be implemented soon. 

This is true currently as WebGL is disabled by default.Currently we dont have any other use case for GRAPHICS_SURFACE (w.r.t EFL port atleast). However there is no dependency of GRAPHICS_SURFACE usage on WebGL (matter of doing some changes in configiration ??). Is the plan to use GraphicsSurface as the default path and fall back to accelerated canvas with no GRAPHICS_SURFACE (and of course software fallback) ??

Feel free to ignore the below comment:
IMHO,Apart from the compile time option, it would be nice to enable/disable this feature during run time (i.e like chrome). This would be quite handy to fix any regressions(software vs accelerated canvas), do some benchmarks as to what use cases do we really benefit etc.

[Kyungjin Kim]

Created attachment 177414 [details]
Patch

[Kyungjin Kim]

(In reply to comment #19)
> (From update of attachment 177177 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177177&action=review
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h:45
> > +    virtual bool hasGraphicsSurface() const { return true; }
> 
> I think it's better if this function returned true when GRAPHICS_SURFACE is on, and false when off, like what you did inside Canvas2DLayerEfl.cpp. 
> Maybe Zeno has a different idea though :)
adopt canvasHasGraphicsSurface().

> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:678
> > +        m_canvasPlatformLayer->paintContents(*context, rect);
> 
> This is not right; 
> You should paint the canvas' content after painting the regular content, and adjust it to contentsRect().
> Otherwise the canvas'  background and borders would disappear.

Right, this could be a bug, fixed.
> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:763
> > +    if (m_canvasPlatformLayer && !m_canvasPlatformLayer->hasGraphicsSurface())
> > +        setDrawsContent(true);
> 
> I don't like setting drawsContent from within CoordinatedGraphicsLayer.
> Better to refactor the functions that use it, like shouldHaveBackingStore()

adopt canvasHasGraphicsSurface().

[Kyungjin Kim]

(In reply to comment #20)
> (In reply to comment #1)
> > Created an attachment (id=176882) [details] [details]
> > Patch
> 
> (In reply to comment #18)
> > Created an attachment (id=177177) [details] [details]
> > Patch
> 
> We should also check for ENABLE(ACCELERATED_2D_CANVAS) when returning platformLayer. As I see it, we don't even create or declare the layer otherwise.
> Same at other places where layer comes into play.
> 
> for example:
> 
> #if USE(ACCELERATED_COMPOSITING)
>  PlatformLayer* ImageBuffer::platformLayer() const
>  {
>  #if ENABLE(ACCELERATED_2D_CANVAS) && PLATFORM(EFL)  
>   return static_cast<TextureMapperPlatformLayer*>(m_data.m_platformLayer.get());
>  #else
>   return 0;
>  #endif
>  }
>  #endif 
> 
> Currently we expose the surface through IMageB

done

[Kalyan]

(In reply to comment #23)
> Created an attachment (id=177414) [details]
> Patch

I think canvasHasGraphicsSurface should always return false when GRAPHICS_SURFACE is not used , irrespective of whether we have a platformlayer or not.

I mean something like this:
bool CoordinatedGraphicsLayer::canvasHasGraphicsSurface() const
{
#if USE(GRAPHICS_SURFACE)
    if (m_canvasPlatformLayer)
        return true;
#endif
    return false;
}

[Kyungjin Kim]

(In reply to comment #26)
> (In reply to comment #23)
> > Created an attachment (id=177414) [details] [details]
> > Patch
> 
> I think canvasHasGraphicsSurface should always return false when GRAPHICS_SURFACE is not used , irrespective of whether we have a platformlayer or not.
> 
> I mean something like this:
> bool CoordinatedGraphicsLayer::canvasHasGraphicsSurface() const
> {
> #if USE(GRAPHICS_SURFACE)
>     if (m_canvasPlatformLayer)
>         return true;
> #endif
>     return false;
> }

canvasHasGraphicsSurface should always return true when GRAPHICS_SURFACE is used. TRUE is default, returning false is exceptional case so if no canvas layer exists true should be returned because it means no exceptional case. You feel me?

[Kalyan]

(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #23)
> > > Created an attachment (id=177414) [details] [details] [details]
> > > Patch
> > 
> > I think canvasHasGraphicsSurface should always return false when GRAPHICS_SURFACE is not used , irrespective of whether we have a platformlayer or not.
> > 
> > I mean something like this:
> > bool CoordinatedGraphicsLayer::canvasHasGraphicsSurface() const
> > {
> > #if USE(GRAPHICS_SURFACE)
> >     if (m_canvasPlatformLayer)
> >         return true;
> > #endif
> >     return false;
> > }
> 
> canvasHasGraphicsSurface should always return true when GRAPHICS_SURFACE is >used. TRUE is default, returning false is exceptional case so if no canvas >layer exists true should be returned because it means no exceptional case. >You feel me?

k, it is bit confusing that we return true even in case GRAPHICS_SURFACE is not used (in case GraphicsLayer doesn't have a valid platformLayer).

[Kalyan]

(In reply to comment #28)
> (In reply to comment #27)
> > (In reply to comment #26)
> > > (In reply to comment #23)
> > > > Created an attachment (id=177414) [details] [details] [details] [details]

> > canvasHasGraphicsSurface should always return true when GRAPHICS_SURFACE is >used. TRUE is default, returning false is exceptional case so if no canvas >layer exists true should be returned because it means no exceptional case. >You feel me?
> 
> k, it is bit confusing that we return true even in case GRAPHICS_SURFACE is not used (in case GraphicsLayer doesn't have a valid platformLayer).

why is the return value of  canvasHasGraphicsSurface () dependent on m_canvasPlatformLayer?? 

Actually, would it be better to query this from platformlayer itself rather than having it in graphicslayer??

[Kalyan]

(In reply to comment #29)
> (In reply to comment #28)
> > (In reply to comment #27)
> > > (In reply to comment #26)
> > > > (In reply to comment #23)
> > > > > Created an attachment (id=177414) [details] [details] [details] [details] [details]
> 
> > > canvasHasGraphicsSurface should always return true when GRAPHICS_SURFACE is >used. TRUE is default, returning false is exceptional case so if no canvas >layer exists true should be returned because it means no exceptional case. >You feel me?
> > 
> > k, it is bit confusing that we return true even in case GRAPHICS_SURFACE is not used (in case GraphicsLayer doesn't have a valid platformLayer).
> 
> why is the return value of  canvasHasGraphicsSurface () dependent on m_canvasPlatformLayer?? 
> 
> Actually, would it be better to query this from platformlayer itself rather than having it in graphicslayer??

I mean would it be better to query from platformLayer, if it is using a graphicssurface or not, rather than doing it in coordinatedgraphicslayer

[Kyungjin Kim]

(In reply to comment #30)
> (In reply to comment #29)
> > (In reply to comment #28)
> > > (In reply to comment #27)
> > > > (In reply to comment #26)
> > > > > (In reply to comment #23)
> > > > > > Created an attachment (id=177414) [details] [details] [details] [details] [details] [details]
> > 
> > > > canvasHasGraphicsSurface should always return true when GRAPHICS_SURFACE is >used. TRUE is default, returning false is exceptional case so if no canvas >layer exists true should be returned because it means no exceptional case. >You feel me?
> > > 
> > > k, it is bit confusing that we return true even in case GRAPHICS_SURFACE is not used (in case GraphicsLayer doesn't have a valid platformLayer).
> > 
> > why is the return value of  canvasHasGraphicsSurface () dependent on m_canvasPlatformLayer?? 

if m_canvasPlatformlayer is null, it means there's no canvas layer for 2D here so the return value should be dependent on it.
> > 
> > Actually, would it be better to query this from platformlayer itself rather than having it in graphicslayer??
> 
> I mean would it be better to query from platformLayer, if it is using a graphicssurface or not, rather than doing it in coordinatedgraphicslayer

It was. I checked it in Canvas2DLayerEfl originally. But it's been changed by Noam's suggestion. Do you believe checking it in each platformLayer could be better?

[Kyungjin Kim]

(In reply to comment #22)
> (In reply to comment #18)
> > Created an attachment (id=177177) [details] [details]
> > Patch
> 
> Canvas2DLayerEfl.h  -> There isnt any things specific to EFL(Atleast from the code I see here). 

With graphics surface implementation, it could be more EFL specific class. And I'm very careful about making it to a general class unless other ports intent to use it.
> 
> Two different approaches we could take:
> 1)As I see it, we can rename it for something more generic (i.e Canvas2DAcceleratedLayer or any other generic name) which could be reused in other places too (see my comments below, expanding on this front.)
> 2)Can/should we extend PlatformContextCairo to handle this functionality. 
> 
> There are common things between GraphicsContext3DPrivate and this class which could be shared. It would be good for us to have a base class which encapsulates the common functionality and than class deriving from it to handle 2DCanvas and 3D Canvas specific stuff. I guess this is out of scope for this changeset but we should do this at some point.
> 
> >>for both cases using GRAPHICS_SURFACE and no GRAPHICS_SURFACE because it is >>not enabled by default in EFL currently.
> >>  Graphics surface version of Accelerated2DCanvas will be implemented soon. 
> 
> This is true currently as WebGL is disabled by default.Currently we dont have any other use case for GRAPHICS_SURFACE (w.r.t EFL port atleast). However there is no dependency of GRAPHICS_SURFACE usage on WebGL (matter of doing some changes in configiration ??). Is the plan to use GraphicsSurface as the default path and fall back to accelerated canvas with no GRAPHICS_SURFACE (and of course software fallback) ??

May be no. If GRAPHICS_SURFACE is on graphics surface version of accelerated canvas will be used.
> 
> Feel free to ignore the below comment:
> IMHO,Apart from the compile time option, it would be nice to enable/disable this feature during run time (i.e like chrome). This would be quite handy to fix any regressions(software vs accelerated canvas), do some benchmarks as to what use cases do we really benefit etc.

[Kalyan]

(In reply to comment #32)
> (In reply to comment #22)
> > (In reply to comment #18)
> > > Created an attachment (id=177177) [details] [details] [details]
> > > Patch
> > 
> > Canvas2DLayerEfl.h  -> There isnt any things specific to EFL(Atleast from the code I see here). 
> 
> With graphics surface implementation, it could be more EFL specific class. And I'm very careful about making it to a general class unless other ports intent to use it.
 
Are we going to use evas to provide us context and surface for AC?? If this is the case than we should take the same path as GraphicsContext3DPrivate (EFL port). Unless EVAS is going to be used for actual primitive rendering, I dont really see any need to use it.

>> This is true currently as WebGL is disabled by default.Currently we dont >>have any other use case for GRAPHICS_SURFACE (w.r.t EFL port atleast). >>However there is no dependency of GRAPHICS_SURFACE usage on WebGL (matter of >>doing some changes in configiration ??). Is the plan to use GraphicsSurface >>as the default path and fall back to accelerated canvas with no >>GRAPHICS_SURFACE (and of course software fallback) ??

>>May be no. If GRAPHICS_SURFACE is on graphics surface version of accelerated >>canvas will be used.

I meant the current situation in the trunk. Ofcourse, we need to make changes for 2D Acceleration as needed. Than the question is do we really need support for both Accelerated Versions (With GraphicsSurface and Without ?? ). Is this the only reason to have support for both versions??

[Kalyan]

(In reply to comment #31)
> (In reply to comment #30)
> > (In reply to comment #29)
> > > (In reply to comment #28)
> > > > (In reply to comment #27)
> > > > > (In reply to comment #26)
> > > > > > (In reply to comment #23)
> > > > > > > Created an attachment (id=177414) [details] [details] [details] [details] [details] [details] [details]
> 
> if m_canvasPlatformlayer is null, it means there's no canvas layer for 2D here so the return value should be dependent on it.

This is the part which is bit confusing for me. The relevant code from the patch: 


bool CoordinatedGraphicsLayer::canvasHasGraphicsSurface() const
{
#if !USE(GRAPHICS_SURFACE)
    if (m_canvasPlatformLayer)
        return false;
#endif
    return true;
}

we would always return true in case of canvasPlatformlayer being null, irrespective of Graphics_Surface being supported or not.

> > > Actually, would it be better to query this from platformlayer itself rather than having it in graphicslayer??
> > 
> > I mean would it be better to query from platformLayer, if it is using a graphicssurface or not, rather than doing it in coordinatedgraphicslayer
> 
> It was. I checked it in Canvas2DLayerEfl originally. But it's been changed >by Noam's suggestion. Do you believe checking it in each platformLayer could >be better?

K, I see that you had this in your earlier version. 
Unless I have missed something, Noam only suggested to have the implementation in TextureMapperPlatformLayer similar to what you have in Canvas2DLayerEFL. I think we should have a default implementation in TextureMapperPlatformLayer , similar to this:

bool hasGraphicsSurface() const
{
#if USE(GRAPHICS_SURFACE)
        return true;
#endif
    return false;
}

Any platform layer having custom checks could re-implement this function as you suggested.

[Noam Rosenthal]

> K, I see that you had this in your earlier version. 
> Unless I have missed something, Noam only suggested to have the implementation in TextureMapperPlatformLayer similar to what you have in Canvas2DLayerEFL. I think we should have a default implementation in TextureMapperPlatformLayer , similar to this:
> 
> bool hasGraphicsSurface() const
> {
> #if USE(GRAPHICS_SURFACE)
>         return true;
> #endif
>     return false;
> }
> 
> Any platform layer having custom checks could re-implement this function as you suggested.

This was along the lines of my thinking.
However, I still fail to understand the value of accelerated 2D canvas in coordinated graphics without graphics surface. 
What makes sense to me is to disable accelerated 2d canvas if graphics surfaces are disabled.

[Kalyan]

(In reply to comment #35)
> > K, I see that you had this in your earlier version. 
> > Unless I have missed something, Noam only suggested to have the implementation in TextureMapperPlatformLayer similar to what you have in Canvas2DLayerEFL. I think we should have a default implementation in TextureMapperPlatformLayer , similar to this:
> > 
> > bool hasGraphicsSurface() const
> > {
> > #if USE(GRAPHICS_SURFACE)
> >         return true;
> > #endif
> >     return false;
> > }
> > 
> > Any platform layer having custom checks could re-implement this function as you suggested.
> 
> This was along the lines of my thinking.
> However, I still fail to understand the value of accelerated 2D canvas in coordinated graphics without graphics surface. 
> What makes sense to me is to disable accelerated 2d canvas if graphics surfaces are disabled.

How about something like this:

TextureMapperPlatformLayer would have something like:
bool hasGraphicsSurface() const
 {
 #if USE(GRAPHICS_SURFACE)
         return true;
 #endif
     return false;
 }

and in coordinated graphics:
bool canvasHasGraphicsSurface()
{
return (m_canvasPlatformLayer &&  m_canvasPlatformLayer->hasGraphicsSurface());
}

PlatformLayer would be responsible for making the decision of using the graphics surface or not.

[Kalyan]

(In reply to comment #36)
> (In reply to comment #35)

> TextureMapperPlatformLayer would have something like:
> bool hasGraphicsSurface() const
>  {
>  #if USE(GRAPHICS_SURFACE)
>          return true;
>  #endif
>      return false;
>  }

Any deriving class would be able to over-ride this function and return the appropriate value. (i.e False when Graphics surface creation failed or any other reason when it doesn't want to use graphics surface.)

[Kyungjin Kim]

(In reply to comment #36)
> (In reply to comment #35)
> > > K, I see that you had this in your earlier version. 
> > > Unless I have missed something, Noam only suggested to have the implementation in TextureMapperPlatformLayer similar to what you have in Canvas2DLayerEFL. I think we should have a default implementation in TextureMapperPlatformLayer , similar to this:
> > > 
> > > bool hasGraphicsSurface() const
> > > {
> > > #if USE(GRAPHICS_SURFACE)
> > >         return true;
> > > #endif
> > >     return false;
> > > }
> > > 
> > > Any platform layer having custom checks could re-implement this function as you suggested.
> > 
> > This was along the lines of my thinking.
> > However, I still fail to understand the value of accelerated 2D canvas in coordinated graphics without graphics surface. 
> > What makes sense to me is to disable accelerated 2d canvas if graphics surfaces are disabled.
> 
> How about something like this:
> 
> TextureMapperPlatformLayer would have something like:
> bool hasGraphicsSurface() const
>  {
>  #if USE(GRAPHICS_SURFACE)
>          return true;
>  #endif
>      return false;
>  }
> 
> and in coordinated graphics:
> bool canvasHasGraphicsSurface()
> {
> return (m_canvasPlatformLayer &&  m_canvasPlatformLayer->hasGraphicsSurface());
> }
> 
> PlatformLayer would be responsible for making the decision of using the graphics surface or not.

It looks good to me.

[Kyungjin Kim]

(In reply to comment #38)
> (In reply to comment #36)
> > (In reply to comment #35)
> > and in coordinated graphics:
> > bool canvasHasGraphicsSurface()
> > {
> > return (m_canvasPlatformLayer &&  m_canvasPlatformLayer->hasGraphicsSurface());
> > }
> > 
> > PlatformLayer would be responsible for making the decision of using the graphics surface or not.
> 
> It looks good to me.

Actually it should be canvasHasNoGraphicsSurface()
bool CoordinatedGraphicsLayer::canvasHasNoGraphicsSurface() const
{
    return m_canvasPlatformLayer && !m_canvasPlatformLayer->hasGraphicsSurface();
}
to replace if (m_canvasPlatformLayer && !m_canvasPlatformLayer->hasGraphicsSurface())

[Kalyan]

(In reply to comment #39)

> > > PlatformLayer would be responsible for making the decision of using the graphics surface or not.
> > 
> > It looks good to me.
> 
> Actually it should be canvasHasNoGraphicsSurface()
> bool CoordinatedGraphicsLayer::canvasHasNoGraphicsSurface() const
> {
>     return m_canvasPlatformLayer && !m_canvasPlatformLayer->hasGraphicsSurface();
> }
> to replace if (m_canvasPlatformLayer && !m_canvasPlatformLayer->hasGraphicsSurface())

Hmm, I don't understand, why toggle the value returned by platformLayer?? Isint the purpose only  to determine if GraphicsSurface is being used by platformLayer or not.

[Kyungjin Kim]

(In reply to comment #40)
> (In reply to comment #39)
> 
> > > > PlatformLayer would be responsible for making the decision of using the graphics surface or not.
> > > 
> > > It looks good to me.
> > 
> > Actually it should be canvasHasNoGraphicsSurface()
> > bool CoordinatedGraphicsLayer::canvasHasNoGraphicsSurface() const
> > {
> >     return m_canvasPlatformLayer && !m_canvasPlatformLayer->hasGraphicsSurface();
> > }
> > to replace if (m_canvasPlatformLayer && !m_canvasPlatformLayer->hasGraphicsSurface())
> 
> Hmm, I don't understand, why toggle the value returned by platformLayer?? Isint the purpose only  to determine if GraphicsSurface is being used by platformLayer or not.

Because I need a condition that platformLayer exist but the layer doesn't have GraphicsSurface.

[Kalyan]

(In reply to comment #41)
> (In reply to comment #40)
> > (In reply to comment #39)

> Because I need a condition that platformLayer exist but the layer doesn't have GraphicsSurface.

Isint this the responsibility of platformLayer ?? (it should return false when it doesn't have a graphicssurface).

overriding hasGraphicsSurface() in the derived class: 

bool customLayer::hasGraphicsSurface() const
 {
 #if USE(GRAPHICS_SURFACE)
        if (m_graphicssurface)  // any other custom checks
            return true;
 #endif
     return false;
 }

[Kyungjin Kim]

(In reply to comment #42)
> (In reply to comment #41)
> > (In reply to comment #40)
> > > (In reply to comment #39)
> 
> > Because I need a condition that platformLayer exist but the layer doesn't have GraphicsSurface.
> 
> Isint this the responsibility of platformLayer ?? (it should return false when it doesn't have a graphicssurface).
> 
> overriding hasGraphicsSurface() in the derived class: 
> 
> bool customLayer::hasGraphicsSurface() const
>  {
>  #if USE(GRAPHICS_SURFACE)
>         if (m_graphicssurface)  // any other custom checks
>             return true;
>  #endif
>      return false;
>  }

Then canvasHasGraphicsSurface() is no longer needed I think because it was just for a specific condition I said. Canvas2DLayerEfl::hasGraphicsSurface will be implemented later in the graphics surface version if necessary. I think I can fix using only platformLayer()->hasGraphicsSurface().

[Kyungjin Kim]

Created attachment 177913 [details]
Patch

[Kyungjin Kim]

Created attachment 177930 [details]
Patch

[Noam Rosenthal]

Comment on attachment 177930 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177930&action=review

I still don't get the point of accelerated canvas in coordinated graphics without graphics surfaces, and it's still not explained.

> Source/WebCore/ChangeLog:11
> +        for both cases using GRAPHICS_SURFACE and no GRAPHICS_SURFACE because it is not enabled by default in EFL currently.

This is a strange reasoning to implement a non-GraphicsSurface fallback.
There is already a fallback - non-accelerated 2d canvas. Why have another fallback that uses some sort of cairo surface and then paints to software?

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:351
> +    m_canvasPlatformLayer = platformLayer;
> +    m_canvasNeedsDisplay = true;
> +    if (client())
> +        client()->notifyFlushRequired(this);

This lines exist inside the #if clause, why not take them outside the clause altogether rather than have an #else?

[Michael Catanzaro]

Closing this bug because the EFL port has been removed from trunk.

If you feel this bug applies to a different upstream WebKit port and was closed in error, please either update the title and reopen the bug, or leave a comment to request this.