Bug 130969 - [EFL] Composite all layers into one evas_object using TextureMapper.
Summary: [EFL] Composite all layers into one evas_object using TextureMapper.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Linux
: P2 Normal
Assignee: Hyowon Kim
URL:
Keywords:
Depends on:
Blocks: 79766
  Show dependency treegraph
 
Reported: 2014-03-31 07:54 PDT by Hyowon Kim
Modified: 2014-04-30 04:39 PDT (History)
7 users (show)

See Also:


Attachments
Patch (33.07 KB, patch)
2014-03-31 08:15 PDT, Hyowon Kim
no flags Details | Formatted Diff | Diff
Patch (34.93 KB, patch)
2014-04-01 20:22 PDT, Hyowon Kim
no flags Details | Formatted Diff | Diff
Patch (35.38 KB, patch)
2014-04-01 23:29 PDT, Hyowon Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hyowon Kim 2014-03-31 07:54:01 PDT
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.
Comment 1 Hyowon Kim 2014-03-31 08:15:58 PDT
Created attachment 228179 [details]
Patch
Comment 2 Ryuan Choi 2014-03-31 15:35:00 PDT
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.
Comment 3 Hyowon Kim 2014-04-01 20:22:32 PDT
Created attachment 228359 [details]
Patch
Comment 4 Ryuan Choi 2014-04-01 20:42:20 PDT
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 ?
Comment 5 Hyowon Kim 2014-04-01 23:26:02 PDT
(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.
Comment 6 Hyowon Kim 2014-04-01 23:29:38 PDT
Created attachment 228366 [details]
Patch
Comment 7 Ryuan Choi 2014-04-01 23:39:25 PDT
(In reply to comment #6)
> Created an attachment (id=228366) [details]
> Patch

LGTM
Comment 8 Hyowon Kim 2014-04-02 00:11:47 PDT
(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.
Comment 9 Hyowon Kim 2014-04-02 00:12:26 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > Created an attachment (id=228366) [details] [details]
> > Patch
> 
> LGTM

Thanks for your review.
Comment 10 Gyuyoung Kim 2014-04-02 00:33:22 PDT
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 ?
Comment 11 Hyowon Kim 2014-04-02 00:54:40 PDT
(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.
Comment 12 Gyuyoung Kim 2014-04-02 00:57:08 PDT
Comment on attachment 228366 [details]
Patch

Ok, LGTM.
Comment 13 WebKit Commit Bot 2014-04-02 02:24:48 PDT
Comment on attachment 228366 [details]
Patch

Clearing flags on attachment: 228366

Committed r166637: <http://trac.webkit.org/changeset/166637>
Comment 14 WebKit Commit Bot 2014-04-02 02:24:54 PDT
All reviewed patches have been landed.  Closing bug.