[PATCH] Android: Port eglretracer for Android
Juha-Pekka Heikkilä
juha-pekka.heikkila at linux.intel.com
Tue Jul 23 07:20:02 PDT 2013
Hi Alexander!
Thanks for looking again at the patch :) Few comments below, I'll later
post a fix.
/Juha-Pekka
On Tue, July 23, 2013 3:10 pm, Alexander Monakov wrote:
> On Tue, 23 Jul 2013, Juha-Pekka Heikkila wrote:
>> diff --git a/dispatch/glproc_egl.cpp b/dispatch/glproc_egl.cpp
>> index 5f93b14..f9e4ac1 100644
>> --- a/dispatch/glproc_egl.cpp
>> +++ b/dispatch/glproc_egl.cpp
>> @@ -26,6 +26,10 @@
>>
>> #include "glproc.hpp"
>>
>> +#if defined __ANDROID__ && defined BUILDEGLRETRACER
>> +#include <waffle.h>
>> +#endif
>> +
>>
>> #if !defined(_WIN32)
>> #ifndef _GNU_SOURCE
>> @@ -137,6 +141,10 @@ _getPublicProcAddress(const char *procName)
>> void *
>> _getPrivateProcAddress(const char *procName)
>> {
>> +#if defined __ANDROID__ && defined BUILDEGLRETRACER
>> + extern int waffle_gl_api;
>> + return waffle_dl_sym(waffle_gl_api, procName);
>> +#else
>
> Why do you need Waffle to handle symbol lookups for Apitrace? As far as I
> understand, you only need the surface+context handling from Waffle, and
> Apitrace's own dynamic loading code should be sufficient. Can you drop
> all
> code that references waffle_gl_api from this patch?
If the intention is to go Waffle way for Android why would I go
sideloading with different APIs?
>
> Furthermore, I think it's wrong that you patch _getPrivateProcAddress but
> not
> _getPublicProcAddress.
>
True, its a silly error.
>
>> diff --git a/retrace/CMakeLists.txt b/retrace/CMakeLists.txt
>> index 913c23e..e77d549 100644
>> --- a/retrace/CMakeLists.txt
>> +++ b/retrace/CMakeLists.txt
>> @@ -39,6 +39,33 @@ add_library (retrace_common STATIC
>> retrace_swizzle.cpp
>> json.cpp
>> )
>> +
>> +if (ANDROID)
>> + message ("-- Looking for Waffle library")
>> +
>> +#########
>> +# In following the name waffle-1 should be fixed to found out properly.
>> + find_path (Waffle_INCLUDE_DIRS
>> + NAMES waffle.h
>> + PATHS "$ENV{OUT}/obj/include/waffle-1"
>> + "$ENV{OUT}/obj/lib"
>> "$ENV{ANDROID_BUILD_TOP}/external/waffle"
>> + NO_CMAKE_FIND_ROOT_PATH
>> + )
>> + if (Waffle_INCLUDE_DIRS)
>> + message ("-- Looking for Waffle library - found")
>> + find_library (Waffle
>> + NAMES "waffle-1"
>> + PATHS "$ENV{OUT}/obj/include/waffle-1" "$ENV{OUT}/obj/lib"
>> + "$ENV{ANDROID_BUILD_TOP}/external/waffle"
>> + REQUIRED NO_CMAKE_FIND_ROOT_PATH NO_DEFAULT_PATH
>> + )
>> + include_directories (${Waffle_INCLUDE_DIRS})
>> + else ()
>> + message ("-- Looking for Waffle library - not found,"
>> + " skipping eglretrace")
>> + endif ()
>> +endif ()
>> +
>
> I would suggest not introducing knowledge of Android repo layout into
> CMakeLists, and instead make it the responsibility of the user to make
> sure
> that the (cross-)compiler can find Waffle's include files and libraries.
> That
> can be arranged with environment variables such as LIBRARY_PATH and CPATH.
I feel this is a bit iffy, mostly because these paths have remained static
since honeycomb (I think). Removing the paths here will add extra hurdle
for compiling this for Android while the paths almost everyone would use
here are the same. Then again, removing the paths here would make Nigel's
suggested alternative way of compiling possible but then everyone need to
give these paths every time.
Anyone compiling eglretracer from this in any case have to have
possibility to compile Waffle for their device somehow somewhere thus I
assume most people using this would have the Android source tree on their
build machine.
>
>> target_link_libraries (retrace_common
>> image
>> common
>> @@ -112,24 +139,45 @@ if (WIN32 OR APPLE OR X11_FOUND)
>> install (TARGETS glretrace RUNTIME DESTINATION bin)
>> endif ()
>>
>> -if (ENABLE_EGL AND X11_FOUND AND NOT WIN32 AND NOT APPLE)
>> - add_executable (eglretrace
>> - glws_egl_xlib.cpp
>> - )
>> +
>> +if (ENABLE_EGL AND X11_FOUND OR (ANDROID AND Waffle_INCLUDE_DIRS)AND
>> NOT WIN32
>> + AND NOT APPLE)
>> + if (ANDROID)
>> + add_definitions(-DBUILDEGLRETRACER)
>> + add_executable (eglretrace
>> + glws_waffle.cpp
>> + )
>> + else ()
>> + add_executable (eglretrace
>> + glws_egl_xlib.cpp
>> + )
>> + endif ()
>
> Please keep the old "if" statement as is and add a new one for
> glws_waffle.cpp.
Yes, I felt stupid writing that but did not realize what is wrong.
>
>>
>> add_dependencies (eglretrace glproc)
>>
>> - target_link_libraries (eglretrace
>> - retrace_common
>> - glretrace_common
>> - glproc_egl
>> - ${X11_X11_LIB}
>> - ${CMAKE_THREAD_LIBS_INIT}
>> - dl
>> - )
>> + if (ANDROID)
>> + target_link_libraries (eglretrace
>> + retrace_common
>> + glretrace_common
>> + glproc_egl
>> + dl
>> + ${Waffle}
>> + )
>> + else ()
>> + target_link_libraries (eglretrace
>> + retrace_common
>> + glretrace_common
>> + glproc_egl
>> + ${X11_X11_LIB}
>> + ${CMAKE_THREAD_LIBS_INIT}
>> + dl
>> + )
>> + endif ()
>>
>> if (${CMAKE_SYSTEM_NAME} MATCHES "Linux")
>> - target_link_libraries (eglretrace rt)
>> + if (NOT ANDROID)
>> + target_link_libraries (eglretrace rt)
>> + endif ()
>> if (READPROC_H_FOUND)
>> target_link_libraries (eglretrace ${proc_LIBRARY})
>> endif ()
>> diff --git a/retrace/glws_waffle.cpp b/retrace/glws_waffle.cpp
>> new file mode 100644
>> index 0000000..f0c44ab
>> --- /dev/null
>> +++ b/retrace/glws_waffle.cpp
>> @@ -0,0 +1,274 @@
>> +#include "os.hpp"
>> +#include "waffle.h"
>> +#include "glws.hpp"
>> +
>> +extern "C" {
>> +int waffle_gl_api = 0;
>> +};
>> +
>> +namespace glws {
>> +
>> +struct waffle_display *dpy;
>> +
>> +class WaffleVisual : public Visual
>> +{
>> +public:
>> + struct waffle_config *config;
>> +
>> + WaffleVisual() :
>> + config(NULL)
>> + {}
>> +
>> + static WaffleVisual* createWaffleVisual(bool doubleBuffer, Profile
>> profile)
>> + {
>
> Can this code go into the constructor instead and the static method be
> removed?
I prefer the two stage constructor so I don't need to start throwing
exceptions. All waffle functions can fail so I take them the C-ish way.
>
>
>> + int config_attrib_list[64], i(0);
>> + WaffleVisual *visual = new WaffleVisual();
>> +
>> + config_attrib_list[i++] = WAFFLE_CONTEXT_API;
>> +
>> + switch (profile) {
>> + case PROFILE_COMPAT:
>> + if(!waffle_display_supports_context_api(dpy,
>> WAFFLE_DL_OPENGL))
>> + {
>> + os::log("%s: !waffle_display_supports_context_api\n",
>> + __FILE__);
>> +
>> + delete visual;
>> + assert(0);
>> + return NULL;
>> + }
>> +
>> + waffle_gl_api = config_attrib_list[i++] = WAFFLE_DL_OPENGL;
>
> WAFFLE_DL_x enum cannot be used in config_attrib_list.
>
True, that is a typo.
>> + break;
>> + case PROFILE_CORE:
>> + os::log("%s: PROFILE_CORE no support\n", __FILE__);
>> + delete visual;
>> + assert(0);
>> + return NULL;
>> + case PROFILE_ES1:
>> + if(!waffle_display_supports_context_api(dpy,
>> +
>> WAFFLE_CONTEXT_OPENGL_ES1))
>> + {
>> + os::log("%s: !waffle_display_supports_context_api\n",
>> + __FILE__);
>> +
>> + delete visual;
>> + assert(0);
>> + return NULL;
>> + }
>> +
>> + config_attrib_list[i++] = WAFFLE_CONTEXT_OPENGL_ES1;
>> + waffle_gl_api = WAFFLE_DL_OPENGL_ES1;
>> + break;
>> + case PROFILE_ES2:
>> + if(!waffle_display_supports_context_api(dpy,
>> +
>> WAFFLE_CONTEXT_OPENGL_ES2))
>> + {
>> + os::log("%s: !waffle_display_supports_context_api\n",
>> + __FILE__);
>> +
>> + delete visual;
>> + assert(0);
>> + return NULL;
>> + }
>> +
>> + config_attrib_list[i++] = WAFFLE_CONTEXT_OPENGL_ES2;
>> + waffle_gl_api = WAFFLE_DL_OPENGL_ES2;
>> + break;
>> + default:
>> + os::log("%s: Unknown context profile\n", __FILE__);
>> + delete visual;
>> + assert(0);
>> + return NULL;
>> + }
>> +
>> + config_attrib_list[i++] = WAFFLE_RED_SIZE;
>> + config_attrib_list[i++] = 8;
>> + config_attrib_list[i++] = WAFFLE_GREEN_SIZE;
>> + config_attrib_list[i++] = 8;
>> + config_attrib_list[i++] = WAFFLE_BLUE_SIZE;
>> + config_attrib_list[i++] = 8;
>> + config_attrib_list[i++] = WAFFLE_DEPTH_SIZE;
>> + config_attrib_list[i++] = 8;
>> + config_attrib_list[i++] = WAFFLE_ALPHA_SIZE;
>> + config_attrib_list[i++] = 8;
>> + config_attrib_list[i++] = WAFFLE_STENCIL_SIZE;
>> + config_attrib_list[i++] = 8;
>> + config_attrib_list[i++] = WAFFLE_DOUBLE_BUFFERED;
>> + config_attrib_list[i++] = doubleBuffer;
>> + config_attrib_list[i++] = 0;
>> +
>> + visual->config = waffle_config_choose(dpy, config_attrib_list);
>> + if (!visual->config)
>> + {
>> + os::log("Error in %s waffle_config_choose(dpy, " \
>> + "config_attrib_list)\n", __FILE__);
>> + delete visual;
>> + assert(0);
>> + return NULL;
>> + }
>> + return visual;
>> + }
>> +
>> + ~WaffleVisual() {
>> + if (this->config != NULL)
>> + {
>> + waffle_config_destroy(this->config);
>> + this->config = NULL;
>> + }
>> + }
>> +};
>> +
>> +class WaffleDrawable : public Drawable
>> +{
>> +public:
>> +
>> + struct waffle_window *window;
>> +
>> + WaffleDrawable(const Visual *vis, int w, int h, bool pbuffer) :
>> + Drawable(vis, w, h, pbuffer)
>> + {
>> + const WaffleVisual *waffleVisual =
>> + static_cast<const WaffleVisual *>(vis);
>> +
>> + window = waffle_window_create(waffleVisual->config, w, h);
>> + }
>> +
>> + void
>> + resize(int w, int h) {
>> + if (w == width && h == height) {
>> + return;
>> + }
>> +
>> + waffle_window_resize(window, w, h);
>> + Drawable::resize(w, h);
>> + }
>> +
>> + void show(void) {
>> + if (visible) {
>> + return;
>> + }
>> +
>> + waffle_window_show(window);
>> + Drawable::show();
>> + }
>> +
>> + void swapBuffers(void) {
>> + waffle_window_swap_buffers(window);
>> + }
>> +};
>> +
>> +class WaffleContext : public Context
>> +{
>> +public:
>> + struct waffle_context *context;
>> +
>> + WaffleContext(const Visual *vis, Profile prof) :
>> + Context(vis, prof),
>> + context(NULL)
>> + {}
>> +
>> + static WaffleContext* createWaffleContext(const Visual *vis,
>> Profile prof)
>> + {
>
> Likewise: can this go into the constructor?
As above, I prefer two stage constructor here because of Waffle.
>
>> + WaffleContext *ctx = new WaffleContext(vis, prof);
>> + const WaffleVisual *waffleVisual =
>> + static_cast<const WaffleVisual *>(vis);
>> +
>> + ctx->context = waffle_context_create(waffleVisual->config,
>> NULL);
>> + if (!ctx->context)
>> + {
>> + os::log("Error in %s waffle_context_create(config,
>> NULL)\n",
>> + __FILE__);
>> +
>> + assert(0);
>> + return NULL;
>> + }
>> + return ctx;
>> + }
>> +
>> + ~WaffleContext() {
>> + if( context != NULL )
>> + {
>> + waffle_context_destroy(context);
>> + context = NULL;
>> + }
>> + }
>> +};
>> +
>> +/*
>> + With waffle there is not too many events to look for..
>> + */
>> +bool
>> +processEvents(void) {
>> + return true;
>> +}
>> +
>> +void
>> +init(void) {
>> + int i;
>> + int waffle_init_attrib_list[3];
>> +
>> + i = 0;
>> + waffle_init_attrib_list[i++] = WAFFLE_PLATFORM;
>> + waffle_init_attrib_list[i++] = WAFFLE_PLATFORM_ANDROID;
>> + waffle_init_attrib_list[i++] = WAFFLE_NONE;
>> +
>> + waffle_init(waffle_init_attrib_list);
>> +
>> + dpy = waffle_display_connect(NULL);
>> + if (!dpy)
>> + {
>> + os::log("No display!!\n");
>> + assert(0);
>> + }
>> +}
>> +
>> +void
>> +cleanup(void) {
>> + waffle_display_disconnect(dpy);
>> +}
>> +
>> +Visual *
>> +createVisual(bool doubleBuffer, Profile profile) {
>> + return WaffleVisual::createWaffleVisual(doubleBuffer, profile);
>> +}
>> +
>> +Drawable *
>> +createDrawable(const Visual *visual, int width, int height, bool
>> pbuffer)
>> +{
>> + return new WaffleDrawable(visual, width, height, pbuffer);
>> +}
>> +
>> +Context *
>> +createContext(const Visual *visual, Context *shareContext, Profile
>> profile,
>> + bool debug)
>> +{
>> + return WaffleContext::createWaffleContext(visual, profile );
>> +}
>> +
>> +bool
>> +makeCurrent(Drawable *drawable, Context *context)
>> +{
>> + if (!drawable || !context) {
>> + /*
>> + Following cause inside waffle
>> + eglMakeCurrent(dpy, NULL, NULL, NULL) which equal to
>> + eglMakeCurrent(eglDisplay, EGL_NO_SURFACE, EGL_NO_SURFACE, \
>> + EGL_NO_CONTEXT)
>> + Lets hope things stay that way...
>> + */
>
> The comment basically says that Waffle's make_current(NULL, NULL) has a
> totally reasonable and expected behavior. Combined with the lack of code
> comments in the rest of the file, this creates a somewhat comical effect,
> at
> least to me. :) No offence meant, just pointing out the imbalance.
No offence taken :) The thing here is that..
waffle_make_current(dpy, NULL, NULL);
..look really straightforward as if would just do..
eglMakeCurrent(dpy, NULL, NULL, NULL);
..but it does more than that and is platform dependant. This is
undocumented behaviour within Waffle and looking at the Waffle code I do
think it should be underlined here is used something which was not
promised.
The code which happen with waffle_make_current you can see here:
http://cgit.freedesktop.org/~chadversary/waffle/tree/src/waffle/api/waffle_gl_misc.c#n80
I see there enough code where the behaviour can be changed so that
waffle_make_current(dpy, NULL, NULL) used here would need to change. That
is until it is documented into api waffle_make_current(dpy, NULL, NULL)
will do eglMakeCurrent(dpy, NULL, NULL, NULL).
More information about the apitrace
mailing list