Bug 77308 – [EFL] Enable WebGL with glx backend

• Component: WebKit EFL
• Status: RESOLVED FIXED

[ChangSeok Oh]

This bug is for enabling WebGL with glx backend for WebKitEfl.

[ChangSeok Oh]

Created attachment 124554 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Does this patch have any relation with the one in bug 62961?

[Igor Trindade Oliveira]

you should remove fast/canvas/webgl from Skipped file.

[ChangSeok Oh]

(In reply to comment #2)
> Does this patch have any relation with the one in bug 62961?

A little. I also checked the patch, but it looks too old and is not valid anymore.
If someone wants to implement WebGL with EVAS_GL backend like the patch, it's a better idea to add a new option to choose gl backend in my opinion.

[Raphael Kubo da Costa (:rakuco)]

Does it mean Samsung (or anyone else, for that matter) is not working on that approach anymore and I can close that bug report?

[ChangSeok Oh]

(In reply to comment #5)
> Does it mean Samsung (or anyone else, for that matter) is not working on that approach anymore and I can close that bug report?

No, it doesn't. Using evasgl is still a valid approach for WebGL and it could be one option to choose. I believe Samsung(or anyone else) is going to contribute it near soon. 
But on my side, current implementation of GraphicsContext3DEfl has something wrong. First of all, GraphicsContext3DInternal is not a valid class name. Second, OpenGL API wrapper is useless for supporting WebGL if we follow the way of other ports.
Anyway I think the patch for bug 62961 is needed to revise.
This patch is just an another option that we can choose to support WebGL.

[ChangSeok Oh]

(In reply to comment #3)
> you should remove fast/canvas/webgl from Skipped file.

Which file do you mean? In here, LayoutTests/platform/efl/Skipped?

[Igor Trindade Oliveira]

yup
(In reply to comment #7)
> (In reply to comment #3)
> > you should remove fast/canvas/webgl from Skipped file.
> 
> Which file do you mean? In here, LayoutTests/platform/efl/Skipped?

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #3)
> you should remove fast/canvas/webgl from Skipped file.

And please make sure the tests are passing :)

[ChangSeok Oh]

Created attachment 124747 [details]
Patch

[WebKit Review Bot]

Attachment 124747 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 124747 [details]
Patch

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

You should send a test version of the patch setting ENABLE_WEBGL to ON so we can check the build passes.

> Source/WebCore/CMakeLists.txt:2209
> +        platform/graphics/GraphicsContext3D.cpp

Why? It's not sorted anymore.

> Source/WebKit/efl/ewk/ewk_view.cpp:651
> +    priv->pageSettings->setWebGLEnabled(true);

What happens if you pass true here but have ENABLE_WEBGL set to OFF (the default configuration)?

[ChangSeok Oh]

Comment on attachment 124747 [details]
Patch

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

Thank you for review. :)
AFAIK, WebGL has remained experimental. We usually disable that kind of feature as the default configuration. Same situation in GTK, MAC port and build-webkit.
In addition, above style alarm looks false... :p

>> Source/WebCore/CMakeLists.txt:2209
>> +        platform/graphics/GraphicsContext3D.cpp
> 
> Why? It's not sorted anymore.

Sorry, I couldn't get the point. 'p' < 'r', right?

>> Source/WebKit/efl/ewk/ewk_view.cpp:651
>> +    priv->pageSettings->setWebGLEnabled(true);
> 
> What happens if you pass true here but have ENABLE_WEBGL set to OFF (the default configuration)?

In that case, we can't create webgl context, so that we wouldn't see any webgl image.

[Martin Robinson]

Comment on attachment 124747 [details]
Patch

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

Just a couple tiny style nits. Please fix them before landing the patch.

> Source/WebCore/platform/graphics/efl/DrawingBufferEfl.cpp:40
> +DrawingBuffer::DrawingBuffer(GraphicsContext3D* context,
> +                             const IntSize& size,
> +                             bool multisampleExtensionSupported,
> +                             bool packedDepthStencilExtensionSupported,
> +                             bool separateBackingTexture)

In WebKit we usually keep parameter lists on one line.

> Source/WebCore/platform/graphics/efl/DrawingBufferEfl.cpp:66
> +    // create a texture to render into

This comment should start with a capital letter and end with a period.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 124747 [details]
Patch

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

>>> Source/WebCore/CMakeLists.txt:2209
>>> +        platform/graphics/GraphicsContext3D.cpp
>> 
>> Why? It's not sorted anymore.
> 
> Sorry, I couldn't get the point. 'p' < 'r', right?

You need to take casing into account.

>>> Source/WebKit/efl/ewk/ewk_view.cpp:651
>>> +    priv->pageSettings->setWebGLEnabled(true);
>> 
>> What happens if you pass true here but have ENABLE_WEBGL set to OFF (the default configuration)?
> 
> In that case, we can't create webgl context, so that we wouldn't see any webgl image.

So enabling WebGL in the source code even if WebGL is disabled in the build won't do any harm?

[Raphael Kubo da Costa (:rakuco)]

Let me repeat that you should send a test version of the patch setting ENABLE_WEBGL to ON so we can check the build passes. By the way, have you checked that the layout tests you have enabled pass?

[Martin Robinson]

(In reply to comment #16)
> Let me repeat that you should send a test version of the patch setting ENABLE_WEBGL to ON so we can check the build passes. By the way, have you checked that the layout tests you have enabled pass?

If the feature is disabled during build, the test script should be smart enough to not run the tests.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #17)
> If the feature is disabled during build, the test script should be smart enough to not run the tests.

I meant running the tests locally (there's no bot running EFL layout tests yet).

[ChangSeok Oh]

(In reply to comment #16)
> Let me repeat that you should send a test version of the patch setting ENABLE_WEBGL to ON so we can check the build passes. 

Hm.. I'm a little confused. Do you want that I send you the patch setting WEBGL ON personally and leave WEBGL OFF as the default? Or we should set ENABLE_WEBGL ON as the default though the feature is experimental?

> By the way, have you checked that the layout tests you have enabled pass?
No. I haven't yet. Let me do it after updating the patch.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #19)
> Hm.. I'm a little confused. Do you want that I send you the patch setting WEBGL ON personally and leave WEBGL OFF as the default? Or we should set ENABLE_WEBGL ON as the default though the feature is experimental?

The former -- just send the patch with the default toggled _without_ r? and cq? and manually ask the EWS bots to run, so that the EFL bot tries to build the patch with the enabled code path and we can check it works fine.

[ChangSeok Oh]

Comment on attachment 124747 [details]
Patch

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

Thank you all.

>>>> Source/WebCore/CMakeLists.txt:2209
>>>> +        platform/graphics/GraphicsContext3D.cpp
>>> 
>>> Why? It's not sorted anymore.
>> 
>> Sorry, I couldn't get the point. 'p' < 'r', right?
> 
> You need to take casing into account.

Excuse me, Could you tell me in detail? Do you want my explanation why I sorted files to build in ChangeLog? or Want to relocate GraphicsContext3D.cpp before DrawingBuffer.cpp?

>> Source/WebCore/platform/graphics/efl/DrawingBufferEfl.cpp:40
>> +                             bool separateBackingTexture)
> 
> In WebKit we usually keep parameter lists on one line.

Done.
But if you think DrawingBufferGtk.cpp is also needed to modify like this, let me know. :)

>> Source/WebCore/platform/graphics/efl/DrawingBufferEfl.cpp:66
>> +    // create a texture to render into
> 
> This comment should start with a capital letter and end with a period.

ditto.

>>>> Source/WebKit/efl/ewk/ewk_view.cpp:651
>>>> +    priv->pageSettings->setWebGLEnabled(true);
>>> 
>>> What happens if you pass true here but have ENABLE_WEBGL set to OFF (the default configuration)?
>> 
>> In that case, we can't create webgl context, so that we wouldn't see any webgl image.
> 
> So enabling WebGL in the source code even if WebGL is disabled in the build won't do any harm?

Right. No problem to build and run.

[ChangSeok Oh]

(In reply to comment #19)
> (In reply to comment #16)
> > Let me repeat that you should send a test version of the patch setting ENABLE_WEBGL to ON so we can check the build passes. 
> 
> Hm.. I'm a little confused. Do you want that I send you the patch setting WEBGL ON personally and leave WEBGL OFF as the default? Or we should set ENABLE_WEBGL ON as the default though the feature is experimental?
> 
> > By the way, have you checked that the layout tests you have enabled pass?
> No. I haven't yet. Let me do it after updating the patch.

I've run DRT locally, but nothing special regarding webgl test cases.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #21)
> >> Sorry, I couldn't get the point. 'p' < 'r', right?
> > 
> > You need to take casing into account.
> 
> Excuse me, Could you tell me in detail? Do you want my explanation why I sorted files to build in ChangeLog? or Want to relocate GraphicsContext3D.cpp before DrawingBuffer.cpp?

You just need to move GraphicsContext3D.cpp back to its original position in CMakeLists.txt: 'p' < 'r', but 'G' < 'g'.

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #22)
> I've run DRT locally, but nothing special regarding webgl test cases.

Do you mean the webgl tests you have removed from the skipped list are all passing? If so, OK from my side.

[ChangSeok Oh]

Created attachment 125482 [details]
Patch

[ChangSeok Oh]

Created attachment 125488 [details]
Patch

[ChangSeok Oh]

(In reply to comment #23)
> (In reply to comment #21)
> > >> Sorry, I couldn't get the point. 'p' < 'r', right?
> > > 
> > > You need to take casing into account.
> > 
> > Excuse me, Could you tell me in detail? Do you want my explanation why I sorted files to build in ChangeLog? or Want to relocate GraphicsContext3D.cpp before DrawingBuffer.cpp?
> 
> You just need to move GraphicsContext3D.cpp back to its original position in CMakeLists.txt: 'p' < 'r', but 'G' < 'g'.

O.K. I see. I was confused, since following lines in OptionsEfl.cmake. I have thought we don't care of uppercase/lowcase.
>html/MediaFragmentURIParser.cpp
>html/shadow/MediaControlElements.cpp
>html/TimeRanges.cpp

 I also fixed it. :)

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 125488 [details]
Patch

Assuming all the raised issues have been fixed, cq+'ing the patch.

[WebKit Review Bot]

Comment on attachment 125488 [details]
Patch

Clearing flags on attachment: 125488

Committed r106772: <http://trac.webkit.org/changeset/106772>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.