Bug 73111 – [Texmap][EFL] Accelerated compositing support using TextureMapper on EFL port

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Hyowon Kim]

WebKit-EFL will support accelerated compositing using TextureMapper which is a cross-platform implementation based on OpenGL (ES).

[Hyowon Kim]

Created attachment 116564 [details]
This patch makes TextureMapper build on EFL port.

[Noam Rosenthal]

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.

[Raphael Kubo da Costa (:rakuco)]

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.

[Gyuyoung Kim]

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

[Noam Rosenthal]

Comment on attachment 116564 [details]
This patch makes TextureMapper build on EFL port.

Seems to not compile.

[Gyuyoung Kim]

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?

[Hyowon Kim]

Created attachment 116716 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 116716 [details]
Patch

Attachment 116716 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10679112

[Gyuyoung Kim]

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 ?

[Tomasz Morawski]

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)

[Hyowon Kim]

Created attachment 116728 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 116728 [details]
Patch

Attachment 116728 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10668682

[Tomasz Morawski]

(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.

[Raphael Kubo da Costa (:rakuco)]

(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.

[Tomasz Morawski]

(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

[Antonio Gomes]

(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.

[Hyowon Kim]

(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.

[Hyowon Kim]

Created attachment 116901 [details]
Patch

[Hyowon Kim]

(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!

[Noam Rosenthal]

Comment on attachment 116901 [details]
Patch

LGTM. Please have a committer from EFL commit when you can watch your bots/EWS.

[WebKit Review Bot]

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

[Hyowon Kim]

Created attachment 116920 [details]
Patch

[WebKit Review Bot]

Comment on attachment 116920 [details]
Patch

Clearing flags on attachment: 116920

Committed r101347: <http://trac.webkit.org/changeset/101347>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Raphael Kubo da Costa (:rakuco)]

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.

[Raphael Kubo da Costa (:rakuco)]

Created https://bugs.webkit.org/show_bug.cgi?id=73580 manually, as the rollout had conflicts and sheriffbot failed.

[Dominik Röttsches (drott)]

Hyowon, when would you have time to continue upstreaming on this topic?

[Kenneth Rohde Christiansen]

(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

[Kenneth Rohde Christiansen]

(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.

[Kenneth Rohde Christiansen]

Created attachment 165850 [details]
Patch

[Hyowon Kim]

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

Unnecessary files :) Thanks.

[WebKit Review Bot]

Comment on attachment 165850 [details]
Patch

Clearing flags on attachment: 165850

Committed r129720: <http://trac.webkit.org/changeset/129720>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.