Bug 73111

Summary: [Texmap][EFL] Accelerated compositing support using TextureMapper on EFL port
Product: WebKit Reporter: Hyowon Kim <hw1008.kim>
Component: WebKit EFLAssignee: Kenneth Rohde Christiansen <kenneth>
Status: RESOLVED FIXED    
Severity: Normal CC: dbates, d-r, gyuyoung.kim, gyuyoung.kim, hausmann, kenneth, lucas.de.marchi, noam, rakuco, ryuan.choi, t.morawski, tonikitoo, webkit.review.bot, yael
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Linux   
Attachments:
Description Flags
This patch makes TextureMapper build on EFL port.
none
Patch
none
Patch
none
Patch
noam: review+, webkit.review.bot: commit-queue-
Patch
none
Patch none

Description Hyowon Kim 2011-11-24 22:27:29 PST
WebKit-EFL will support accelerated compositing using TextureMapper which is a cross-platform implementation based on OpenGL (ES).
Comment 1 Hyowon Kim 2011-11-24 22:53:47 PST
Created attachment 116564 [details]
This patch makes TextureMapper build on EFL port.
Comment 2 Noam Rosenthal 2011-11-25 02:20:21 PST
Comment on attachment 116564 [details]
This patch makes TextureMapper build on EFL port.

The changes to TextureMapper are OK with me.
I don't mind reviewing this whole patch, but I'm not well versed in the EFL port to know if that is welcome.
Comment 3 Raphael Kubo da Costa (:rakuco) 2011-11-25 04:43:01 PST
Comment on attachment 116564 [details]
This patch makes TextureMapper build on EFL port.

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

LGTM aside from the nitpicks below.

> ChangeLog:10
> +        No new tests, this is a buildsystem changes.

changes -> change.

> ChangeLog:12
> +        * Source/cmake/OptionsEfl.cmake:

I'd move the "Add WTF_USE_TEXTURE_MAPPER [...]" paragraph here.

> Source/WebCore/ChangeLog:17
> +        * platform/graphics/efl/GraphicsLayerEfl.cpp: Removed.
> +        * platform/graphics/efl/GraphicsLayerEfl.h: Removed.

It's weird that these files do not show up in the patch.
Comment 4 Gyuyoung Kim 2011-11-25 04:58:30 PST
Comment on attachment 116564 [details]
This patch makes TextureMapper build on EFL port.

Attachment 116564 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10646401
Comment 5 Noam Rosenthal 2011-11-27 10:15:51 PST
Comment on attachment 116564 [details]
This patch makes TextureMapper build on EFL port.

Seems to not compile.
Comment 6 Gyuyoung Kim 2011-11-27 17:59:56 PST
Comment on attachment 116564 [details]
This patch makes TextureMapper build on EFL port.

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

It looks this patch needs to use new GL library. But, it looks this patch misses library dependency. In order to avoid build break in EWS, we have to install needed library to EWS server first. Please add gl library dependency to cmake files, and then we have to install gl library to EWS and build bot server together.

/mnt/eflews/git/webkit/Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:46: error: variable or field 'glUniform1f' declared void
/mnt/eflews/git/webkit/Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:46: error: 'GLint' was not declared in this scope
/mnt/eflews/git/webkit/Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:46: error: 'GLfloat' was not declared in this scope
/mnt/eflews/git/webkit/Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:47: error: variable or field 'glUniform1i' declared void
/mnt/eflews/git/webkit/Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:47: error: 'GLint' was not declared in this scope
/mnt/eflews/git/webkit/Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:47: error: 'GLint' was not declared in this scope
/mnt/eflews/git/webkit/Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:48: error: variable or field 'glVertexAttribPointer' declared void
/mnt/eflews/git/webkit/Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:48: error: 'GLuint' was not declared in this scope
/mnt/eflews/git/webkit/Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:48: error: 'GLint' was not declared in this scope
/mnt/eflews/git/webkit/Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:48: error: 'GLenum' was not declared in this scope
/mnt/eflews/git/webkit/Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:48: error: 'GLboolean' was not declared in this scope
/mnt/eflews/git/webkit/Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:48: error: 'GLsizei' was not declared in this scope
/mnt/eflews/git/webkit/Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:48: error: expected primary-expression before 'const'
/mnt/eflews/git/webkit/Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:49: error: variable or field 'glUniform4f' declared void
/mnt/eflews/git/webkit/Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:49: error: 'GLint' was not declared in t

> Source/WebCore/ChangeLog:11
> +        No new tests. Covered by existing tests.

Would you mention existing test cases you tested?
Comment 7 Hyowon Kim 2011-11-28 02:13:53 PST
Created attachment 116716 [details]
Patch
Comment 8 Gyuyoung Kim 2011-11-28 02:19:05 PST
Comment on attachment 116716 [details]
Patch

Attachment 116716 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10679112
Comment 9 Gyuyoung Kim 2011-11-28 02:41:30 PST
Comment on attachment 116716 [details]
Patch

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

Ok. The needed gl library is just installed on EWS server. :-)  EWS build error will not occur in next submit.

> Source/cmake/OptionsEfl.cmake:39
> +FIND_PACKAGE(GL REQUIRED)

I wonder if we don't need to check GL version. Is old GL library Ok ?
Comment 10 Tomasz Morawski 2011-11-28 02:43:14 PST
Hi,
It still does not compile due to that there is missing FindOpenGL.cmake file. What about adding something like this:


Source/cmake/FindOpenGL.cmake
include(LibFindMacros)

libfind_pkg_check_modules(OpenGL_PKGCONF gl)

find_path(OpenGL_INCLUDE_DIR
  NAMES GL/gl.h
  PATHS ${OpenGL_PKGCONF_INCLUDE_DIRS}
)

find_library(OpenGL_LIBRARY
  NAMES GL
  PATHS ${OpenGL_PKGCONF_LIBRARY_DIRS}
)

set(OpenGL_PROCESS_INCLUDES OpenGL_INCLUDE_DIR)
set(OpenGL_PROCESS_LIBS OpenGL_LIBRARY)
libfind_process(OpenGL)


It could be used as:

FIND_PACKAGE(OpenGL REQUIRED)
Comment 11 Hyowon Kim 2011-11-28 03:56:45 PST
Created attachment 116728 [details]
Patch
Comment 12 Gyuyoung Kim 2011-11-28 04:07:02 PST
Comment on attachment 116728 [details]
Patch

Attachment 116728 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10668682
Comment 13 Tomasz Morawski 2011-11-28 04:27:36 PST
(In reply to comment #11)
> Created an attachment (id=116728) [details]
> Patch

The ${GL_LIBRARY} is not valid now. Please update it according to my FindOpenGL file that I have posted in comment 10.
Comment 14 Raphael Kubo da Costa (:rakuco) 2011-11-28 05:18:13 PST
(In reply to comment #13)
> (In reply to comment #11)
> > Created an attachment (id=116728) [details] [details]
> > Patch
> 
> The ${GL_LIBRARY} is not valid now. Please update it according to my FindOpenGL file that I have posted in comment 10.

Please don't add FindOpenGL.cmake, CMake already has one. See CMake's FindOpenGL.cmake and adjust the buildsystem accordingly.
Comment 15 Tomasz Morawski 2011-11-28 05:26:54 PST
(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #11)
> > > Created an attachment (id=116728) [details] [details] [details]
> > > Patch
> > 
> > The ${GL_LIBRARY} is not valid now. Please update it according to my FindOpenGL file that I have posted in comment 10.
> 
> Please don't add FindOpenGL.cmake, CMake already has one. See CMake's FindOpenGL.cmake and adjust the buildsystem accordingly.

It should be good if you change gl's varables name to:
OPENGL_INCLUDE_DIR  - the GL include directory
OPENGL_LIBRARIES -  Link these to use OpenGL and GLU
Comment 16 Antonio Gomes 2011-11-28 08:41:09 PST
(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #11)
> > > Created an attachment (id=116728) [details] [details] [details]
> > > Patch
> > 
> > The ${GL_LIBRARY} is not valid now. Please update it according to my FindOpenGL file that I have posted in comment 10.
> 
> Please don't add FindOpenGL.cmake, CMake already has one. See CMake's FindOpenGL.cmake and adjust the buildsystem accordingly.

+dbates.
Comment 17 Hyowon Kim 2011-11-28 22:26:35 PST
(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #11)
> > > Created an attachment (id=116728) [details] [details] [details]
> > > Patch
> > 
> > The ${GL_LIBRARY} is not valid now. Please update it according to my FindOpenGL file that I have posted in comment 10.
> 
> Please don't add FindOpenGL.cmake, CMake already has one. See CMake's FindOpenGL.cmake and adjust the buildsystem accordingly.

I found that CMake has support for finding the OpenGL package. (/usr/share/cmake-2.8/Moduels/Find*.cmake)
Thanks for letting me know.
Comment 18 Hyowon Kim 2011-11-28 22:58:14 PST
Created attachment 116901 [details]
Patch
Comment 19 Hyowon Kim 2011-11-28 23:23:02 PST
(In reply to comment #2)
> (From update of attachment 116564 [details])
> The changes to TextureMapper are OK with me.
> I don't mind reviewing this whole patch, but I'm not well versed in the EFL port to know if that is welcome.

Your review can help me a lot~! Thanks!
Comment 20 Noam Rosenthal 2011-11-28 23:27:53 PST
Comment on attachment 116901 [details]
Patch

LGTM. Please have a committer from EFL commit when you can watch your bots/EWS.
Comment 21 WebKit Review Bot 2011-11-29 00:51:51 PST
Comment on attachment 116901 [details]
Patch

Rejecting attachment 116901 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
hird_party/libwebp --revision 111575 --non-interactive --force --accept theirs-conflict --ignore-externals returned non-zero exit status 1 in /mnt/git/webkit-commit-queue/Source/WebKit/chromium
Error: 'depot_tools/gclient sync --force --reset --delete_unversioned_trees' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 109.
Re-trying 'depot_tools/gclient sync --force --reset --delete_unversioned_trees'
No such file or directory at Tools/Scripts/update-webkit line 112.

Full output: http://queues.webkit.org/results/10679563
Comment 22 Hyowon Kim 2011-11-29 01:29:57 PST
Created attachment 116920 [details]
Patch
Comment 23 WebKit Review Bot 2011-11-29 02:47:47 PST
Comment on attachment 116920 [details]
Patch

Clearing flags on attachment: 116920

Committed r101347: <http://trac.webkit.org/changeset/101347>
Comment 24 WebKit Review Bot 2011-11-29 02:47:54 PST
All reviewed patches have been landed.  Closing bug.
Comment 25 Raphael Kubo da Costa (:rakuco) 2011-12-01 08:50:24 PST
Hmm, I wonder why the EWS did not fail with this patch (perhaps it fails when SHARED_CORE=On?).

The build always fails here when linking the Tools programs (EWebLauncher, DumpRenderTree and ImageDiff):
../../../Source/WebCore/libwebcore_efl.so.0.1.0: undefined reference to `WebCore::uidForImage(WebCore::Image*)'
../../../Source/WebCore/libwebcore_efl.so.0.1.0: undefined reference to `WebCore::BGRA32PremultimpliedBuffer::create()'
collect2: ld returned 1 exit status

And indeed, these two are only implemented by TextureMapperQt.cpp. I think the patch as-is should be rolled out.
Comment 26 Raphael Kubo da Costa (:rakuco) 2011-12-01 12:14:58 PST
Created https://bugs.webkit.org/show_bug.cgi?id=73580 manually, as the rollout had conflicts and sheriffbot failed.
Comment 27 Dominik Röttsches (drott) 2012-09-26 04:50:30 PDT
Hyowon, when would you have time to continue upstreaming on this topic?
Comment 28 Kenneth Rohde Christiansen 2012-09-26 06:39:47 PDT
(In reply to comment #27)
> Hyowon, when would you have time to continue upstreaming on this topic?

This patch should apply without any problems now, none of those methods exists anymore - reopen
Comment 29 Kenneth Rohde Christiansen 2012-09-26 08:02:30 PDT
(In reply to comment #28)
> (In reply to comment #27)
> > Hyowon, when would you have time to continue upstreaming on this topic?
> 
> This patch should apply without any problems now, none of those methods exists anymore - reopen

This patch more or less seems to be in trunk, minus that the old files are also still there.
Comment 30 Kenneth Rohde Christiansen 2012-09-26 12:15:34 PDT
Created attachment 165850 [details]
Patch
Comment 31 Hyowon Kim 2012-09-26 18:03:14 PDT
(In reply to comment #30)
> Created an attachment (id=165850) [details]
> Patch

Unnecessary files :) Thanks.
Comment 32 WebKit Review Bot 2012-09-26 18:20:28 PDT
Comment on attachment 165850 [details]
Patch

Clearing flags on attachment: 165850

Committed r129720: <http://trac.webkit.org/changeset/129720>
Comment 33 WebKit Review Bot 2012-09-26 18:20:33 PDT
All reviewed patches have been landed.  Closing bug.