Bug 62961

Summary: [EFL] Initial implementation of GraphicsContext3DPrivate
Product: WebKit Reporter: Hyowon Kim <hw1008.kim>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, gyuyoung.kim, gyuyoung.kim, igor.oliveira, kenneth, kevin.cs.oh, leandro, lucas.de.marchi, rakuco, ryuan.choi, sangseok.lim, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 62695, 62709, 79759, 79766, 80211    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
noam: review+, webkit.review.bot: commit-queue-
re-upload
none
re-upload2 none

Description Hyowon Kim 2011-06-19 20:05:21 PDT
This patch adds GraphicsContext3DInternal implementation for EFL port.
Comment 1 Hyowon Kim 2011-06-19 20:10:24 PDT
Created attachment 97734 [details]
Patch
Comment 2 Ryuan Choi 2011-06-19 21:21:32 PDT
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?
Comment 3 Gyuyoung Kim 2011-06-19 21:24:58 PDT
I think this patch is too big. Is it possible to split this patch into smaller one ?
Comment 4 Hyowon Kim 2011-06-19 23:21:15 PDT
(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.
Comment 5 Gyuyoung Kim 2011-06-19 23:32:37 PDT
(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.
Comment 6 Ryuan Choi 2011-06-20 00:31:36 PDT
(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.
Comment 7 Hyowon Kim 2011-06-20 02:51:43 PDT
Created attachment 97769 [details]
Patch
Comment 8 Hyowon Kim 2011-06-20 02:53:53 PDT
(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.
Comment 9 Ryuan Choi 2011-06-20 03:45:19 PDT
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.
Comment 10 Hyowon Kim 2011-06-20 05:10:02 PDT
Created attachment 97781 [details]
Patch
Comment 11 Hyowon Kim 2011-06-20 05:12:38 PDT
(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.
Comment 12 Raphael Kubo da Costa (:rakuco) 2011-06-20 07:01:11 PDT
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.
Comment 13 Gyuyoung Kim 2011-06-21 19:33:42 PDT
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.
Comment 14 Hyowon Kim 2011-06-21 21:53:00 PDT
Created attachment 98115 [details]
Patch
Comment 15 Hyowon Kim 2011-06-21 22:03:34 PDT
(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.
Comment 16 Hyowon Kim 2011-06-21 22:07:21 PDT
(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.
Comment 17 Gyuyoung Kim 2011-06-21 22:40:10 PDT
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".
Comment 18 Hyowon Kim 2011-06-22 04:18:26 PDT
(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!!!
Comment 19 Raphael Kubo da Costa (:rakuco) 2011-06-22 07:05:45 PDT
(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.
Comment 20 Hyowon Kim 2011-06-22 20:10:06 PDT
Created attachment 98304 [details]
Patch
Comment 21 Hyowon Kim 2011-06-22 20:11:32 PDT
(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.
Comment 22 Raphael Kubo da Costa (:rakuco) 2011-06-27 05:15:47 PDT
It looks good to me now; informal r+ from my side.
Comment 23 Igor Trindade Oliveira 2012-01-30 12:43:35 PST
How are you testing this code? are you enabling webgl in build-webkit?
Comment 24 Hyowon Kim 2012-02-25 23:59:17 PST
Created attachment 128908 [details]
Patch
Comment 25 Hyowon Kim 2012-02-26 00:22:03 PST
(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.
Comment 26 WebKit Review Bot 2012-02-26 00:50:54 PST
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
Comment 27 Igor Trindade Oliveira 2012-02-26 06:52:30 PST
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
Comment 28 Hyowon Kim 2012-02-26 18:50:50 PST
Created attachment 128939 [details]
re-upload
Comment 29 Hyowon Kim 2012-02-26 21:16:43 PST
Created attachment 128947 [details]
re-upload2
Comment 30 Hyowon Kim 2012-02-26 22:53:28 PST
(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!
Comment 31 WebKit Review Bot 2012-02-27 01:30:59 PST
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
Comment 32 Ryuan Choi 2012-02-27 18:08:48 PST
Comment on attachment 128947 [details]
re-upload2

Try cq+ once more because I think that complained message is not related to this bug.
Comment 33 WebKit Review Bot 2012-02-27 19:01:25 PST
Comment on attachment 128947 [details]
re-upload2

Clearing flags on attachment: 128947

Committed r109061: <http://trac.webkit.org/changeset/109061>
Comment 34 WebKit Review Bot 2012-02-27 19:01:34 PST
All reviewed patches have been landed.  Closing bug.