GNOME Bugzilla – Bug 701706
gst-omx testegl.c example not compatible with gstreamer 1.x
Last modified: 2014-07-23 08:26:15 UTC
testegl.c example in git master currently not compatible with git master gstreamer.
Created attachment 246137 [details] [review] testegl.c patch for 1.x support
Created attachment 246138 [details] [review] patch for correct library name
Created attachment 246139 [details] [review] patch for new working mode of testegl example
Created attachment 246148 [details] [review] format patch
Created attachment 246154 [details] [review] testegl.c patch for 1.x support
Created attachment 246155 [details] [review] examples/egl/Makefile.am file patch for linking
Created attachment 246156 [details] [review] testegl header file patch for new mode
Created attachment 246157 [details] [review] patch for enabling examples building by default
Review of attachment 246155 [details] [review]: ::: examples/egl/Makefile.am @@ +7,3 @@ testegl_LDADD = \ $(GST_PLUGINS_BASE_LIBS) \ + -lgstapp-1.0 -lgstvideo-1.0 \ Should be @GST_API_VERSION@
Review of attachment 246154 [details] [review]: This patch seems to include many unrelated changes to porting. Please clean that up, afterwards I'll do a real review :) ::: examples/egl/testegl.c @@ +137,3 @@ + OMX_EGL_EXAMPLE_VIDEO_CUBE, + OMX_EGL_EXAMPLE_VIDEO_BACKGROUND, + /* .. */ Like this @@ +250,3 @@ * ***********************************************************/ + static void init_ogl (APP_STATE_T * state) Indention broken @@ +1022,2 @@ + g_mutex_lock (state->lock); + GST_DEBUG ("\neglglessink-allocate-eglimage %p ", query); No \n here, and use GST_PTR_FORMAT to let it print the query
Review of attachment 246156 [details] [review]: Please put all function changes in a single commit. Every commit should be compileable and useable, so include this here in the commit that adds the new mode
Created attachment 246226 [details] [review] examples/egl/Makefile.am file patch for linking Hardcoded api version replaced by @GST_API_VERSION@.
Created attachment 246227 [details] [review] testegl.c patch for 1.x support
Review of attachment 246227 [details] [review]: Just one minor comment, and you should squash that together with the Makefile.am patch and only keep the new mode as a separate patch. otherwise looks good ::: examples/egl/testegl.c @@ +4,3 @@ Copyright (C) 2013, Fluendo S.A. @author: Josep Torra <josep@fluendo.com> + @author: Ilya Smelykh <ilya@videoexpertsgroup.com> You might want to add a new Copyright line here as I assume you're not working for Fluendo ;)
Created attachment 246402 [details] [review] testegl.c patch for 1.x support
commit 4593918ea46d55c6fb97e98fbe362bbe3c261d66 Author: Ilya Smelykh <ilya@videoexpertsgroup.com> Date: Fri Jun 7 12:39:18 2013 +0700 examples: testegl example port to 1.x https://bugzilla.gnome.org/show_bug.cgi?id=701706