Bug 62709 - [EFL] Add GraphicsContext3DEfl for WebGL and accelerated compositing
Summary: [EFL] Add GraphicsContext3DEfl for WebGL and accelerated compositing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 62707 62961
Blocks: 62695 62959
  Show dependency treegraph
 
Reported: 2011-06-15 02:40 PDT by Hyowon Kim
Modified: 2011-09-12 20:11 PDT (History)
11 users (show)

See Also:


Attachments
Patch (86.08 KB, patch)
2011-06-17 05:42 PDT, Hyowon Kim
no flags Details | Formatted Diff | Diff
Patch (32.25 KB, patch)
2011-06-19 22:03 PDT, Hyowon Kim
no flags Details | Formatted Diff | Diff
Patch (32.18 KB, patch)
2011-06-21 22:15 PDT, Hyowon Kim
no flags Details | Formatted Diff | Diff
Patch (32.27 KB, patch)
2011-06-22 19:36 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 2011-06-15 02:40:29 PDT
patch will be added.
Comment 1 Hyowon Kim 2011-06-17 05:42:02 PDT
Created attachment 97589 [details]
Patch
Comment 2 Hyowon Kim 2011-06-17 05:44:45 PDT
(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.
Comment 3 Leandro Pereira 2011-06-17 08:09:11 PDT
Comment on attachment 97589 [details]
Patch

Patch is *huge*; could you please split it into smaller, preferably less than 20kB parts?
Comment 4 Hyowon Kim 2011-06-19 22:03:22 PDT
Created attachment 97745 [details]
Patch
Comment 5 Hyowon Kim 2011-06-19 23:05:02 PDT
(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.
Comment 6 Raphael Kubo da Costa (:rakuco) 2011-06-20 07:06:39 PDT
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?
Comment 7 Hyowon Kim 2011-06-20 19:40:48 PDT
(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.
Comment 8 Hyowon Kim 2011-06-21 22:15:12 PDT
Created attachment 98116 [details]
Patch
Comment 9 Raphael Kubo da Costa (:rakuco) 2011-06-22 07:10:21 PDT
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
Comment 10 Leandro Pereira 2011-06-22 07:44:50 PDT
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.
Comment 11 Hyowon Kim 2011-06-22 19:36:39 PDT
Created attachment 98298 [details]
Patch
Comment 12 Hyowon Kim 2011-06-22 19:38:52 PDT
(In reply to comment #11)
> Created an attachment (id=98298) [details]
> Patch

I updated this patch to reflect the comment #9 and #10.
Comment 13 Hyowon Kim 2011-06-22 20:02:56 PDT
(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 :)
Comment 14 Raphael Kubo da Costa (:rakuco) 2011-06-27 05:17:08 PDT
Informal r+ from my side now that GraphicsContext3DInternal looks OK too.
Comment 15 Eric Seidel (no email) 2011-09-12 15:37:56 PDT
Comment on attachment 98298 [details]
Patch

rs=me.
Comment 16 WebKit Review Bot 2011-09-12 20:11:07 PDT
Comment on attachment 98298 [details]
Patch

Clearing flags on attachment: 98298

Committed r95002: <http://trac.webkit.org/changeset/95002>
Comment 17 WebKit Review Bot 2011-09-12 20:11:13 PDT
All reviewed patches have been landed.  Closing bug.