Bug 77308

Summary: [EFL] Enable WebGL with glx backend
Product: WebKit Reporter: ChangSeok Oh <kevin.cs.oh>
Component: WebKit EFLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: gyuyoung.kim, igor.oliveira, kbr, lucas.de.marchi, mrobinson, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 77219    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description ChangSeok Oh 2012-01-29 17:57:27 PST
This bug is for enabling WebGL with glx backend for WebKitEfl.
Comment 1 ChangSeok Oh 2012-01-30 08:23:57 PST
Created attachment 124554 [details]
Patch
Comment 2 Raphael Kubo da Costa (:rakuco) 2012-01-30 08:59:15 PST
Does this patch have any relation with the one in bug 62961?
Comment 3 Igor Trindade Oliveira 2012-01-30 13:31:25 PST
you should remove fast/canvas/webgl from Skipped file.
Comment 4 ChangSeok Oh 2012-01-30 21:28:19 PST
(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.
Comment 5 Raphael Kubo da Costa (:rakuco) 2012-01-31 05:11:46 PST
Does it mean Samsung (or anyone else, for that matter) is not working on that approach anymore and I can close that bug report?
Comment 6 ChangSeok Oh 2012-01-31 07:38:11 PST
(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.
Comment 7 ChangSeok Oh 2012-01-31 08:35:58 PST
(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?
Comment 8 Igor Trindade Oliveira 2012-01-31 08:42:07 PST
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?
Comment 9 Raphael Kubo da Costa (:rakuco) 2012-01-31 08:51:34 PST
(In reply to comment #3)
> you should remove fast/canvas/webgl from Skipped file.

And please make sure the tests are passing :)
Comment 10 ChangSeok Oh 2012-01-31 08:54:18 PST
Created attachment 124747 [details]
Patch
Comment 11 WebKit Review Bot 2012-01-31 08:57:53 PST
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.
Comment 12 Raphael Kubo da Costa (:rakuco) 2012-01-31 09:17:30 PST
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)?
Comment 13 ChangSeok Oh 2012-01-31 18:18:39 PST
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.
Comment 14 Martin Robinson 2012-02-02 08:02:54 PST
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.
Comment 15 Raphael Kubo da Costa (:rakuco) 2012-02-02 08:47:21 PST
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?
Comment 16 Raphael Kubo da Costa (:rakuco) 2012-02-02 08:48:18 PST
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?
Comment 17 Martin Robinson 2012-02-02 09:12:52 PST
(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.
Comment 18 Raphael Kubo da Costa (:rakuco) 2012-02-02 09:25:51 PST
(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).
Comment 19 ChangSeok Oh 2012-02-02 10:07:28 PST
(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.
Comment 20 Raphael Kubo da Costa (:rakuco) 2012-02-02 20:24:30 PST
(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.
Comment 21 ChangSeok Oh 2012-02-02 21:24:26 PST
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.
Comment 22 ChangSeok Oh 2012-02-02 22:18:39 PST
(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.
Comment 23 Raphael Kubo da Costa (:rakuco) 2012-02-03 18:48:32 PST
(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'.
Comment 24 Raphael Kubo da Costa (:rakuco) 2012-02-03 18:49:39 PST
(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.
Comment 25 ChangSeok Oh 2012-02-03 23:48:11 PST
Created attachment 125482 [details]
Patch
Comment 26 ChangSeok Oh 2012-02-04 02:32:14 PST
Created attachment 125488 [details]
Patch
Comment 27 ChangSeok Oh 2012-02-04 02:40:24 PST
(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. :)
Comment 28 Raphael Kubo da Costa (:rakuco) 2012-02-05 18:40:50 PST
Comment on attachment 125488 [details]
Patch

Assuming all the raised issues have been fixed, cq+'ing the patch.
Comment 29 WebKit Review Bot 2012-02-05 20:01:56 PST
Comment on attachment 125488 [details]
Patch

Clearing flags on attachment: 125488

Committed r106772: <http://trac.webkit.org/changeset/106772>
Comment 30 WebKit Review Bot 2012-02-05 20:02:03 PST
All reviewed patches have been landed.  Closing bug.