[EFL] Initial implementation of GraphicsContext3DPrivate

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Hyowon Kim]

This patch adds GraphicsContext3DInternal implementation for EFL port.

[Hyowon Kim]

Created attachment 97734 [details]
Patch

[Ryuan Choi]

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

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:61
> +    m_evasGL = ewk_view_evas_gl_create(ewkView);

Should we call ewk api to create evas GL?

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:83
> +    m_context = evas_gl_context_create(m_evasGL, shareContext);
> +    if (!m_context)
> +        return false;
> +
> +    if (!createSurface(ewkView, bRenderDirectlyToEvasGLObject))
> +        return false;

If we can fail to create here, I think that we should create evas gl.
Is it right?

[Gyuyoung Kim]

I think this patch is too big. Is it possible to split this patch into smaller one ?

[Hyowon Kim]

(In reply to comment #3)
> I think this patch is too big. Is it possible to split this patch into smaller one ?

I have already heard the same comment about the size of my patch from Leandro.
(Bug 62709)
GraphicsContext3DInternal is big because it contains wrapper functions to all OpenGL(ES) functions. I have no idea how to split it.

[Gyuyoung Kim]

(In reply to comment #4)
> (In reply to comment #3)
> > I think this patch is too big. Is it possible to split this patch into smaller one ?
> 
> I have already heard the same comment about the size of my patch from Leandro.
> (Bug 62709)
> GraphicsContext3DInternal is big because it contains wrapper functions to all OpenGL(ES) functions. I have no idea how to split it.

I'm not sure if my case can be adjusted your patch. Anyway, when I have a huge patch, I make a patch which has dummy functions first. Then, I make next patches which implement the dummy functions step by step.

[Ryuan Choi]

(In reply to comment #2)
> View in context: https://bugs.webkit.org/attachment.cgi?id=97734&action=review
> 
> > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:61
> > +    m_evasGL = ewk_view_evas_gl_create(ewkView);
> 
> Should we call ewk api to create evas GL?
> 
> > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:83
> > +    m_context = evas_gl_context_create(m_evasGL, shareContext);
> > +    if (!m_context)
> > +        return false;
> > +
> > +    if (!createSurface(ewkView, bRenderDirectlyToEvasGLObject))
> > +        return false;
> 
> If we can fail to create here, I think that we should create evas gl.
> Is it right?

s/create evas gl/clear evas gl/
anyway, this is my mistake after discussed. please ignore it.

[Hyowon Kim]

Created attachment 97769 [details]
Patch

[Hyowon Kim]

(In reply to comment #6)
> (In reply to comment #2)
> > View in context: https://bugs.webkit.org/attachment.cgi?id=97734&action=review
> > 
> > > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:61
> > > +    m_evasGL = ewk_view_evas_gl_create(ewkView);
> > 
> > Should we call ewk api to create evas GL?

I didn't know about evas_object_evas_get().
I have replaced the following code 1 with the code 2.

(1) m_evasGL = ewk_view_evas_gl_create(ewkView);

(2) Evas* evas = evas_object_evas_get(ewkView);
    m_evasGL = evas_gl_create(evas);

Thank you very very much :)

> > > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:83
> > > +    m_context = evas_gl_context_create(m_evasGL, shareContext);
> > > +    if (!m_context)
> > > +        return false;
> > > +
> > > +    if (!createSurface(ewkView, bRenderDirectlyToEvasGLObject))
> > > +        return false;
> > 
> > If we can fail to create here, I think that we should create evas gl.
> > Is it right?
> 
> s/create evas gl/clear evas gl/
> anyway, this is my mistake after discussed. please ignore it.

[Ryuan Choi]

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

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:59
> +    Evas_Object* ewkView = static_cast<Evas_Object*>(hostWindow->platformPageClient());

how do you think view instead of ewkView.

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:70
> +    Evas_GL_Context* shareContext = 0;

shareContext or sharedContext? Which one is right?

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:74
> +        GraphicsContext3D* compositingContext = ewk_view_accelerated_compositing_context_get(ewkView);

Is it public api?
If not, does we need to guard this to ACCELEARTED_COMPOSITING macro?

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:89
> +bool GraphicsContext3DInternal::createSurface(Evas_Object* ewkView, bool renderDirectlyToEvasGLObject)

If createSurface is only for initialize, does we need to separate it?

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:98
> +    evas_object_geometry_get(ewkView, &x, &y, &w, &h);

I think that we can move this into if (renderDirectlyToEvasGLObject).
And evas_object_geometry_get will clear values to 0 although it failed.

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:100
> +    int surfaceWidth = 0, surfaceHeight = 0;

I prefered these meaningful name, but can we share this with w, h.

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:106
> +    } else
> +        surfaceWidth = surfaceHeight = 1;

How about initializing above values to 1 instead of else statement.

Other codes looks just bypassed from WebKit's to evas_gl_api's.

[Hyowon Kim]

Created attachment 97781 [details]
Patch

[Hyowon Kim]

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

I updated this patch to reflect the comment #9 From Ryuan Choi. Thanks again Ryuan.

[Raphael Kubo da Costa (:rakuco)]

Informal r- from my side:

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:2
> +    Copyright (C) 2009-2011 Samsung Electronics

This file is new, so the copyright should start in 2011.

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:57
> +bool GraphicsContext3DInternal::initialize(GraphicsContext3D::Attributes attrs, HostWindow* hostWindow, bool bRenderDirectlyToEvasGLObject)

I'd rather if instead of having a public constructor and this method you could have a static create() method which took care of creating and initializing the object automatically.

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:144
> +void GraphicsContext3DInternal::bindAttribLocation(Platform3DObject program, GC3Duint index, const char* name)

I think it makes more sense to make name a String, and call .utf8().data() here.

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:394
> +    char* name = static_cast<char*>(malloc(maxNameLength * sizeof(char)));

sizeof(char) is always 1.

Manually managing the memory allocated here should not be necessary. You can use, for example, an OwnArrayPtr: OwnArrayPtr<char> name = adoptArrayPtr(new char[maxNameLength]);

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:431
> +    char* name = static_cast<char*>(malloc(maxNameLength * sizeof(char)));

Ditto.

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:486
> +        ListHashSet<GC3Denum>::iterator iter = m_syntheticErrors.begin();

I think it's clearer to do

  GC3Denum err = m_syntheticErrors.first();
  m_syntheticErrors.remove(m_syntheticErrors.begin());
  return err;

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:545
> +    char* log = static_cast<char*>(malloc(logLength * sizeof(char)));

Same comment about manual memory management.

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:579
> +    char* log = static_cast<char*>(malloc(logLength * sizeof(char)));

Ditto.

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:601
> +    char* log = static_cast<char*>(malloc(logLength * sizeof(char)));

Ditto.

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:1113
> +    return 0;

Missing notImplemented()?

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.h:2
> +    Copyright (C) 2009-2011 Samsung Electronics

This file is new, so the copyright should start in 2011.

[Gyuyoung Kim]

Comment on attachment 97781 [details]
Patch

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

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:2
> +    Copyright (C) 2009-2011 Samsung Electronics

Remvoe 2009. Let's only use 2011.

[Hyowon Kim]

Created attachment 98115 [details]
Patch

[Hyowon Kim]

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

I updated this patch to reflect the comment #12 from Raphael.
Raphael, thank you for your interest and comments.

[Hyowon Kim]

(In reply to comment #12)
> Informal r- from my side:
> 
> > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:57
> > +bool GraphicsContext3DInternal::initialize(GraphicsContext3D::Attributes attrs, HostWindow* hostWindow, bool bRenderDirectlyToEvasGLObject)
> 
> I'd rather if instead of having a public constructor and this method you could have a static create() method which took care of creating and initializing the object automatically.
> 

GraphicsContext3DInternal::initialize() calls many Evas_GL's create functions. 
If one of them fails, initialize() returns false, and then the GraphicsContext3DInternal instance will be destroyed.

According to your comment about a static create() function, I made the following codes.
static PassOwnPtr<GraphicsContext3DInternal> GraphicsContext3DInternal::create(a, b, c)
{
     OwnPtr<GraphicsContext3DInternal> internal = adoptPtr(new GraphicsContext3DInternal());
     if (!internal->initialize(a, b, c))
        return 0;
     return internal.release();    
}

But it couldn't return 0 due to the following compile error.

error: conversion from 'int' to non-scalar type 'WTF::PassOwnPtr<WebCore::GraphicsContext3DInternal>' requested

So creating and initailizing a instance at once in the static GraphicsContext3DInternal::create() function seems to be difficult because of its return type.

GraphicsContext3DInternal instances are created only in GraphicsContext3D::create(), so I think that current implementation might be good.

[Gyuyoung Kim]

Comment on attachment 98115 [details]
Patch

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

When you define functions, you need to adhere consistency in parameter name. AFAIK, we have to adhere WebKit coding style rule in WebCore,

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.h:57
> +    void blendFuncSeparate(GC3Denum srcRGB, GC3Denum dstRGB, GC3Denum srcAlpha, GC3Denum dstAlpha);

you use "A" upper case in srcAlpha.

> Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.h:70
> +    void copyTexImage2D(GC3Denum target, GC3Dint level, GC3Denum internalformat, GC3Dint x, GC3Dint y, GC3Dsizei width, GC3Dsizei height, GC3Dint border);

In internalformat, you don't use upper case for "f".

[Hyowon Kim]

(In reply to comment #16)
> (In reply to comment #12)
> > Informal r- from my side:
> > 
> > > Source/WebCore/platform/graphics/efl/GraphicsContext3DInternal.cpp:57
> > > +bool GraphicsContext3DInternal::initialize(GraphicsContext3D::Attributes attrs, HostWindow* hostWindow, bool bRenderDirectlyToEvasGLObject)
> > 
> > I'd rather if instead of having a public constructor and this method you could have a static create() method which took care of creating and initializing the object automatically.
> > 
> GraphicsContext3DInternal::initialize() calls many Evas_GL's create functions. 
> If one of them fails, initialize() returns false, and then the GraphicsContext3DInternal instance will be destroyed.
> According to your comment about a static create() function, I made the following codes.
> static PassOwnPtr<GraphicsContext3DInternal> GraphicsContext3DInternal::create(a, b, c)
> {
>      OwnPtr<GraphicsContext3DInternal> internal = adoptPtr(new GraphicsContext3DInternal());
>      if (!internal->initialize(a, b, c))
>         return 0;
>      return internal.release();    
> }
> But it couldn't return 0 due to the following compile error.
> error: conversion from 'int' to non-scalar type 'WTF::PassOwnPtr<WebCore::GraphicsContext3DInternal>' requested
> So creating and initailizing a instance at once in the static GraphicsContext3DInternal::create() function seems to be difficult because of its return type.
> GraphicsContext3DInternal instances are created only in GraphicsContext3D::create(), so I think that current implementation might be good.

Sorry, I should have used "return nullptr;" instead of 0!!!

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #18)
> Sorry, I should have used "return nullptr;" instead of 0!!!

I'm glad you've figured it out :) I'm looking forward to another patch.

[Hyowon Kim]

Created attachment 98304 [details]
Patch

[Hyowon Kim]

(In reply to comment #19)
> (In reply to comment #18)
> > Sorry, I should have used "return nullptr;" instead of 0!!!
> 
> I'm glad you've figured it out :) I'm looking forward to another patch.

You are so kind :) I updated this patch to reflect your comment about using the static create function.

[Raphael Kubo da Costa (:rakuco)]

It looks good to me now; informal r+ from my side.

[Igor Trindade Oliveira]

How are you testing this code? are you enabling webgl in build-webkit?

[Hyowon Kim]

Created attachment 128908 [details]
Patch

[Hyowon Kim]

(In reply to comment #23)
> How are you testing this code? are you enabling webgl in build-webkit?

I'm sorry for the delay in my response.
I've actually finished the implementation about accelerated compositing using texmap and GC3D already.
I will summit another patch soon, in order to add GC3D files to webkit-efl build when ACCELERATED_COMPOSITING is used.
Thanks for your interest.

[WebKit Review Bot]

Comment on attachment 128908 [details]
Patch

Attachment 128908 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11633543

New failing tests:
inspector/protocol/console-agent.html

[Igor Trindade Oliveira]

i think we are already using GTK GraphicsContext3DInternal: https://bugs.webkit.org/show_bug.cgi?id=77296

I think you guys should try to synchronize what you are doing.


(In reply to comment #26)
> (From update of attachment 128908 [details])
> Attachment 128908 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11633543
> 
> New failing tests:
> inspector/protocol/console-agent.html

[Hyowon Kim]

Created attachment 128939 [details]
re-upload

[Hyowon Kim]

Created attachment 128947 [details]
re-upload2

[Hyowon Kim]

(In reply to comment #27)
> i think we are already using GTK GraphicsContext3DInternal: https://bugs.webkit.org/show_bug.cgi?id=77296
> 
> I think you guys should try to synchronize what you are doing.
> 

Bug 77296 makes it shareable to use GC3DPrivate with GLX backend.

But, this patch is a EFL platform specific using Evas_GL backend.
(As you know, the initialization of GLX is a lot different from Evas_GL's.)

Evas_GL could make it easy to render to Evas 
and provide options to take advantage of performance gains.

And I talked with 'ChangSeok Oh' who's the author of Bug 77296.
He agreed that GC3DPrivate in efl port should be seperated from it in glx.

Thanks!

[WebKit Review Bot]

Comment on attachment 128947 [details]
re-upload2

Rejecting attachment 128947 [details] from commit-queue.

New failing tests:
inspector/protocol/console-agent.html
Full output: http://queues.webkit.org/results/11634005

[Ryuan Choi]

Comment on attachment 128947 [details]
re-upload2

Try cq+ once more because I think that complained message is not related to this bug.

[WebKit Review Bot]

Comment on attachment 128947 [details]
re-upload2

Clearing flags on attachment: 128947

Committed r109061: <http://trac.webkit.org/changeset/109061>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.