Bug 62709 – [EFL] Add GraphicsContext3DEfl for WebGL and accelerated compositing

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Hyowon Kim]

patch will be added.

[Hyowon Kim]

Created attachment 97589 [details]
Patch

[Hyowon Kim]

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

This patch adds the GraphicsContext3DInternal class using Evas_GL APIs.
Evas_GL APIs allow to use OpenGL to render to specially set up evas_image_objects which act as render target surfaces.
GraphicsContext3D in Efl port will be used for accelerated compositing as well as WebGL.

[Leandro Pereira]

Comment on attachment 97589 [details]
Patch

Patch is *huge*; could you please split it into smaller, preferably less than 20kB parts?

[Hyowon Kim]

Created attachment 97745 [details]
Patch

[Hyowon Kim]

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

The patch has split into three bugs.
This patch contains only GraphicsContext3DEfl.cpp.

- GraphicsContext3D.h : Bug 62959
- GraphicsContext3DInternal.h/cpp : Bug 62961

I'm looking forward to the review.

[Raphael Kubo da Costa (:rakuco)]

Reviewing this depends on changes made to bug 62961, so there's not much to say about this one.

Is there going to be another patch plugging these new files into the build system?

[Hyowon Kim]

(In reply to comment #6)
> Reviewing this depends on changes made to bug 62961, so there's not much to say about this one.
> 
> Is there going to be another patch plugging these new files into the build system?

Yes, I will upload another patch for adding new files to build-script files. Thanks.

[Hyowon Kim]

Created attachment 98116 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

It looks mostly OK, it just needs to be updated when the final version of GraphicsContext3DInternal is approved.

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:29
> +#include <wtf/text/CString.h>

I don't think you need this header after you pass a String directly to Graphics

[Leandro Pereira]

Comment on attachment 98116 [details]
Patch

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

This looks like a huge wrapper class; isn't it possible to share most of this code with other ports, as there are few references to Evas and such?

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:35
> +    bool renderDirectlyToEvasGLObject = (renderStyle == RenderDirectlyToHostWindow) ? true : false;

No need to use a ternary operator here; comparisons already returns a boolean.

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:142
> +    m_internal->bufferData(target, size, /* data */ 0, usage);

Is this comment really needed?

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:167
> +    m_internal->clearColor(r, g, b, a);

I'd use more descriptive names here.

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:177
> +    m_internal->clearStencil(s);

Ditto.

[Hyowon Kim]

Created attachment 98298 [details]
Patch

[Hyowon Kim]

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

I updated this patch to reflect the comment #9 and #10.

[Hyowon Kim]

(In reply to comment #10)
> (From update of attachment 98116 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98116&action=review
> 
> This looks like a huge wrapper class; isn't it possible to share most of this code with other ports, as there are few references to Evas and such?

Yes, most of this code are sharable.

Source/WebCore/platform/graphics/GraphicsContext3D.h:934

    // FIXME: ideally this would be used on all platforms.
#if PLATFORM(CHROMIUM) || PLATFORM(QT) || PLATFORM(GTK)
    friend class GraphicsContext3DInternal;
    OwnPtr<GraphicsContext3DInternal> m_internal;
#endif

Chromium & QT already have their own GraphicsContext3DInternal class.
How can you make it common? I have no concrete plans yet.

Thank you for your comment :)

[Raphael Kubo da Costa (:rakuco)]

Informal r+ from my side now that GraphicsContext3DInternal looks OK too.

[Eric Seidel (no email)]

Comment on attachment 98298 [details]
Patch

rs=me.

[WebKit Review Bot]

Comment on attachment 98298 [details]
Patch

Clearing flags on attachment: 98298

Committed r95002: <http://trac.webkit.org/changeset/95002>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.