Bug 130969 – [EFL] Composite all layers into one evas_object using TextureMapper.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Hyowon Kim]

The EFL port uses two evas_objects to render the entire webpage: backing_store and compositingObject. 
backing_store is used to paint a non-composited layer with ewk functions, 
whereas compositingObject is used to paint other layers with TextureMapper.
We need to integrate these separated painting paths.

[Hyowon Kim]

Created attachment 228179 [details]
Patch

[Ryuan Choi]

Comment on attachment 228179 [details]
Patch

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

Thanks, I really wait this.

> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:49
> +    // FIXME: falling back to software mode
> +    ASSERT(m_evasGLContext);

I am not sure whether it is better to cut the software mode first and push back in another bug.

We normally use software mode in EWebLauncher and elm_web as a default.

> Source/WebKit/efl/ewk/ewk_view.cpp:-276
>          size_t count;
>          size_t allocated;
>      } repaints;
> -    WTF::Vector<WebCore::IntRect> m_rectsToScroll;
> -    WTF::Vector<WebCore::IntSize> m_scrollOffsets;

Can we remove repaints too?

> Source/WebKit/efl/ewk/ewk_view.cpp:873
> +    Ewk_View_Private_Data* priv = static_cast<Ewk_View_Private_Data*>(data);
> +    priv->acceleratedCompositingContext->flushAndRenderLayers();

We can just pass acceleratedCompositingContext.

> Source/WebKit/efl/ewk/ewk_view.cpp:938
> +    if (!priv->acceleratedCompositingContext) {
> +        ERR("Could not create accelerated compositing context.");
> +        return;
> +    }

Unnecessary check routine.

[Hyowon Kim]

Created attachment 228359 [details]
Patch

[Ryuan Choi]

Comment on attachment 228359 [details]
Patch

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

> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:40
> +    , m_rootCompositingLayer(0)

nullptr

> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:42
> +    , m_isAccelerated(true)

Nit, after setting false as a default, we can turn on it at 52 line.
It reduces one if statment.

> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:76
> +        return true;
> +    m_viewSize = size;

Empty line here.

By the way, should we return boolean?

> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:181
>  void AcceleratedCompositingContext::attachRootGraphicsLayer(GraphicsLayer* rootLayer)
>  {

From now, this is not to attach root graphics layer.

How about changing method name properly?

> Source/WebKit/efl/ewk/ewk_view.cpp:870
> +void _ewk_view_accelerated_compositing_cb(void* data, Evas_Object*)

static ?

[Hyowon Kim]

(In reply to comment #2)
> (From update of attachment 228179 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228179&action=review
> 
> Thanks, I really wait this.
> 
> > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:49
> > +    // FIXME: falling back to software mode
> > +    ASSERT(m_evasGLContext);
> 
> I am not sure whether it is better to cut the software mode first and push back in another bug.
> 
> We normally use software mode in EWebLauncher and elm_web as a default.

Done. I've added the fall back to software mode.

> > Source/WebKit/efl/ewk/ewk_view.cpp:-276
> >          size_t count;
> >          size_t allocated;
> >      } repaints;
> > -    WTF::Vector<WebCore::IntRect> m_rectsToScroll;
> > -    WTF::Vector<WebCore::IntSize> m_scrollOffsets;
> 
> Can we remove repaints too?

Yes, but to avoid making the patch size bigger, let's handle that in a new bug later.

> > Source/WebKit/efl/ewk/ewk_view.cpp:873
> > +    Ewk_View_Private_Data* priv = static_cast<Ewk_View_Private_Data*>(data);
> > +    priv->acceleratedCompositingContext->flushAndRenderLayers();
> 
> We can just pass acceleratedCompositingContext.
> 
> > Source/WebKit/efl/ewk/ewk_view.cpp:938
> > +    if (!priv->acceleratedCompositingContext) {
> > +        ERR("Could not create accelerated compositing context.");
> > +        return;
> > +    }
> 
> Unnecessary check routine.

Done.

[Hyowon Kim]

Created attachment 228366 [details]
Patch

[Ryuan Choi]

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

LGTM

[Hyowon Kim]

(In reply to comment #4)
> (From update of attachment 228359 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228359&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:40
> > +    , m_rootCompositingLayer(0)
> 
> nullptr

Done.

> > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:42
> > +    , m_isAccelerated(true)
> 
> Nit, after setting false as a default, we can turn on it at 52 line.
> It reduces one if statment.
> 
> > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:76
> > +        return true;
> > +    m_viewSize = size;
> 
> Empty line here.
> 
> By the way, should we return boolean?

The caller was not checking the return value, so I replaced bool with void and added some error logs.

> > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:181
> >  void AcceleratedCompositingContext::attachRootGraphicsLayer(GraphicsLayer* rootLayer)
> >  {
> 
> From now, this is not to attach root graphics layer.
> 
> How about changing method name properly?

I found out some unnecessary code from your comment.
We will not create a dummy root, m_rootLayer, instead m_rootLayer is set as an actual compositing root layer.

> > Source/WebKit/efl/ewk/ewk_view.cpp:870
> > +void _ewk_view_accelerated_compositing_cb(void* data, Evas_Object*)
> 
> static ?

Done.

[Hyowon Kim]

(In reply to comment #7)
> (In reply to comment #6)
> > Created an attachment (id=228366) [details] [details]
> > Patch
> 
> LGTM

Thanks for your review.

[Gyuyoung Kim]

Comment on attachment 228366 [details]
Patch

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

> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:57
> +        m_isAccelerated = false;

Looks wrong indentation.

> Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:74
> +    m_viewSize = size;

Is there any problem to update *m_viewSize* with *size* when m_isAccelerated is false ?

[Hyowon Kim]

(In reply to comment #10)
> (From update of attachment 228366 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=228366&action=review
> 
> > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:57
> > +        m_isAccelerated = false;
> 
> Looks wrong indentation.

It looks correct.

> > Source/WebKit/efl/WebCoreSupport/AcceleratedCompositingContextEfl.cpp:74
> > +    m_viewSize = size;
> 
> Is there any problem to update *m_viewSize* with *size* when m_isAccelerated is false ?

No. m_viewSize is used to keep webview size regardless of acceleration.

[Gyuyoung Kim]

Comment on attachment 228366 [details]
Patch

Ok, LGTM.

[WebKit Commit Bot]

Comment on attachment 228366 [details]
Patch

Clearing flags on attachment: 228366

Committed r166637: <http://trac.webkit.org/changeset/166637>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.