GNOME Bugzilla – Bug 693230
Port to GStreamer 1.0
Last modified: 2013-02-17 19:03:53 UTC
Reference: https://mail.gnome.org/archives/frogr-list/2013-February/msg00001.html Your wish be my command :
Created attachment 235270 [details] [review] Port to GStreamer 1.0 The patch is build tested and frogr starts up fine with it. I don't have means of actually testing the functionality (so far no flickr account :) ) Please let me know if this works.. and if it does, let's move forward. (with GNOME 3.6, gstreamer 1.0 can be considered a given)
There are two more things that need changing: #define CAPS "video/x-raw-rgb,width=160,pixel-aspect-ratio=1/1,bpp=(int)24,depth=(int)24,endianness=(int)4321,red_mask=(int)0xff0000, green_mask=(int)0x00ff00, blue_mask=(int)0x0000ff" to #define CAPS "video/x-raw,format=RGB,width=160,pixel-aspect-ratio=1/1" and descr = g_strdup_printf ("uridecodebin uri=%s ! ffmpegcolorspace ! videoscale ! " to descr = g_strdup_printf ("uridecodebin uri=%s ! videoconvert ! videoscale ! " The code can be simplified further btw using the gdkpixbufsink element from -good: descr = g_strdup_printf ("uridecodebin uri=%s ! videoconvert ! videoscale " " ! " CAPS " ! gdkpixbufsink name=sink", file_uri); then you can replace all the code from http://git.gnome.org/browse/frogr/tree/src/frogr-util.c?id=ec7869cca6d348f02266bba8442ce8c53369426a#n442 to http://git.gnome.org/browse/frogr/tree/src/frogr-util.c?id=ec7869cca6d348f02266bba8442ce8c53369426a#n475 with a simple g_object_get (sink, "last-pixbuf", &pixbuf, NULL); On a sidenote, there seem to be lots of error return NULLs where it should probably shut down and unref the pipeline instead of just returning.
One two more things: - gst_pad_get_current_caps() returns caps with a reference, you need to gst_caps_unref() them somewhere later - does gdk_pixbuf_new_from_data() make a copy of the pixel data? If not, you should probably make a copy with g_memdup (info.data, info.size) first and then pass g_free as GDestroyNotifyFunction and copied_data as destroy_fn_data. (For the case where you don't opt for the gdkpixbufsink solution mentioned above, otherwise it's moot of course).
Hi Dominique, First of all, sorry for the late reply. Haven't checked bugzilla mail for a while, and this has been a pleasant surprise! :) (In reply to comment #1) > Created an attachment (id=235270) [details] [review] > Port to GStreamer 1.0 > > The patch is build tested and frogr starts up fine with it. Thanks! > I don't have means of actually testing the functionality (so far no flickr > account :) ) > > Please let me know if this works.. and if it does, let's move forward. > (with GNOME 3.6, gstreamer 1.0 can be considered a given) I can't test it right now and will probably not be able to do it until next Wednesday (I'll be off since this afternoon until then), but I promise I'll do it as soon as possible. Most likely, next Wednesday as soon as I arrive at home again. In the meanwhile, I'd just ask you to consider Tim-Philipp's comments with regard to the patch, since he's a gstreamer developer and his knowledge is infinitely more valuable than mine on this thing (after all, I have no clue about gst and just copied the snapshot.c example from gst 0.10 to make this work :P) Thanks to both the two of you!
(In reply to comment #2) > There are two more things that need changing: > > #define CAPS > "video/x-raw-rgb,width=160,pixel-aspect-ratio=1/1,bpp=(int)24,depth=(int)24,endianness=(int)4321,red_mask=(int)0xff0000, > green_mask=(int)0x00ff00, blue_mask=(int)0x0000ff" > to > #define CAPS "video/x-raw,format=RGB,width=160,pixel-aspect-ratio=1/1" > > > and > > descr = g_strdup_printf ("uridecodebin uri=%s ! ffmpegcolorspace ! videoscale ! > " > to > descr = g_strdup_printf ("uridecodebin uri=%s ! videoconvert ! videoscale ! " Indeed.. those soft changes are always a little bit nasty to catch (a small script to look for known issues would be cool addition). Will incorporate those into the patch. > The code can be simplified further btw using the gdkpixbufsink element from > -good: > > descr = g_strdup_printf ("uridecodebin uri=%s ! videoconvert ! videoscale " > " ! " CAPS " ! gdkpixbufsink name=sink", file_uri); > then you can replace all the code from > http://git.gnome.org/browse/frogr/tree/src/frogr-util.c?id=ec7869cca6d348f02266bba8442ce8c53369426a#n442 > to > http://git.gnome.org/browse/frogr/tree/src/frogr-util.c?id=ec7869cca6d348f02266bba8442ce8c53369426a#n475 > > with a simple > > g_object_get (sink, "last-pixbuf", &pixbuf, NULL); Simplifaction is surely a good thing.. but I'm in a similar position to Mario: not really knowing Gst, I apply simple recipes from the porting hand book :) I'll try to add those changes as well.. the most difficult for me in this case is not having test capabilities (no flickr account)... maybe time to change this :) @Mario: don't worry about the timing... you deserve some good time off!
(In reply to comment #3) > One two more things: > > - gst_pad_get_current_caps() returns caps with > a reference, you need to gst_caps_unref() them > somewhere later Taking your simplification part into account: this seems actually no longer to matter: it's part of the dropped code after the change to the gdkpixbufsink... > - does gdk_pixbuf_new_from_data() make a copy > of the pixel data? If not, you should probably > make a copy with g_memdup (info.data, info.size) > first and then pass g_free as GDestroyNotifyFunction > and copied_data as destroy_fn_data. (For the case > where you don't opt for the gdkpixbufsink solution > mentioned above, otherwise it's moot of course). replacing almost 40 lines with 1 :) sounds like a good thing...
Created attachment 235535 [details] [review] Port to GStreamer 1.0
Attached a new patch for review... Again, I build tested and this time even 'work tested' a little bit: - Frogr starts - Connected to a (newly created) flickr accoount - Picked a Video: a thumbnail is shown (this is to see that the entire simplification works... hope I got it right when that code should run) - Added a picture to the set - Initiated an upload... ==> all this went smooth. Marioa, Tim-Philipp, please have a look at the new patch. Let me know if I missed some stuff, mis-understood some things or otherwise completely screwed up.
Review of attachment 235535 [details] [review]: Looks good to me, apart from one minor thing I forgot about. ::: src/frogr-util.c @@ -440,3 +440,3 @@ GST_SEEK_FLAG_KEY_UNIT | GST_SEEK_FLAG_FLUSH, position); - /* get the preroll buffer from appsink, this block untils appsink really prerolls */ + g_object_get (sink, "last-pixbuf", &pixbuf, NULL); Sorry, I failed to mention this before, but there is one more thing needed: between the gst_element_seek_simple() and the g_object_get() we need to wait for the pipeline to re-preroll, i.e. wait for a buffer from the new position to get to the sink. We can do that with something like: #define PREROLL_TIMEOUT (5*GST_SECOND) GstStateChangeReturn sret; sret = gst_element_get_state (pipeline, NULL, NULL, PREROLL_TIMEOUT); if (sret != GST_STATE_CHANGE_SUCCESS) goto error;
Created attachment 235538 [details] [review] Port to GStreamer 1.0
Then it should be something like this I guess...
Looks good to me (though you might want to move the variable declaration somewhere to the beginning of the block to avoid compiler warnings in some cases).
Didn't see any warnings while building this, with gcc 4.7 (which is one of the stricter compilers around, no?) Anything I can test to ensure this won't break ?
Review of attachment 235538 [details] [review]: Thanks a lot for the patch, Dominique! The patch looks good to me overall. Still there are some things (unrelated to gstreamer, of course, I have no clue about that!) that I'd like to point out before getting this in. See below... ::: src/frogr-util.c @@ +440,3 @@ GST_SEEK_FLAG_KEY_UNIT | GST_SEEK_FLAG_FLUSH, position); + #define PREROLL_TIMEOUT (5*GST_SECOND) Could you move this macro definition up in the file? (probably after #define CAPS) @@ +444,1 @@ + GstStateChangeReturn sret; Please move this declaration to the beginning of the block, either after the declaration of ret or even in the same line (comma separated) Also, please remove the declarations of the following variables now that you're not using it anymore: buffer, format, width, height and res. Otherwise it will complain while building if you used ./configure --enable-debug=yes (which is more strict) @@ +452,2 @@ /* cleanup and exit */ +error: I have no problems with goto statements in general, but in this very specific case it seems you don't really need it as you are using the error label from this place only and, on top of that, there's only one stament that would be executed if sret == GST_STATE_SUCCESS. I would rewrite this as: [...] if (sret == GST_STATE_CHANGE_SUCCESS) g_object_get (sink, "last-pixbuf", &pixbuf, NULL); gst_element_set_state (pipeline, GST_STATE_NULL); gst_object_unref (pipeline); return pixbuf; }
Sorry for the duplicated comment. Not sure what happened... Anyway, a minor doubt I have now is how I can make this patch to work since frogr is not able to load any thumbnail for me, and I guess it's due to me not having the proper gst plugin installed after the changes done in the pipeline, or something like that? I have the following packages installed: gstreamer1-plugins-bad-free-1.0.5-1.fc18.x86_64 gstreamer1-plugins-bad-free-extras-1.0.5-1.fc18.x86_64 gstreamer1-plugins-base-1.0.5-2.fc18.x86_64 gstreamer1-plugins-good-1.0.5-1.fc18.x86_64 gstreamer1-plugins-good-extras-1.0.5-1.fc18.x86_64 gstreamer1-plugins-ugly-1.0.2-2.fc18.x86_64 Again, thanks!
(In reply to comment #15) > Review of attachment 235538 [details] [review]: > > Thanks a lot for the patch, Dominique! The patch looks good to me overall. > Still there are some things (unrelated to gstreamer, of course, I have no clue > about that!) that I'd like to point out before getting this in. See below... Thanks for your time reviewing it over and over :) > > ::: src/frogr-util.c > @@ +440,3 @@ > GST_SEEK_FLAG_KEY_UNIT | GST_SEEK_FLAG_FLUSH, position); > > + #define PREROLL_TIMEOUT (5*GST_SECOND) > > Could you move this macro definition up in the file? (probably after #define > CAPS) Sure... > > @@ +444,1 @@ > + GstStateChangeReturn sret; > > Please move this declaration to the beginning of the block, either after the > declaration of ret or even in the same line (comma separated) > > Also, please remove the declarations of the following variables now that you're > not using it anymore: buffer, format, width, height and res. > > Otherwise it will complain while building if you used ./configure > --enable-debug=yes (which is more strict) Will make sure to build it with --enable-debug... thanks for the pointer. And moving it up is no issue of course. > > @@ +452,2 @@ > /* cleanup and exit */ > +error: > > I have no problems with goto statements in general, but in this very specific > case it seems you don't really need it as you are using the error label from > this place only and, on top of that, there's only one stament that would be > executed if sret == GST_STATE_SUCCESS. Ok.. I think the 'goto' in this case is just a 'handy' thing to not complicate the code too much in the future... no idea what else all will come after the error has happened. But no problem, I can rewrite this to do the next things conditionally instead. Patch should come later today or on the Weekend.
> Anyway, a minor doubt I have now is how I can make this patch to work since > frogr is not able to load any thumbnail for me, and I guess it's due to me not > having the proper gst plugin installed after the changes done in the pipeline, > or something like that? You may also want gst-libav (gst-ffmpeg replacement) for video codecs, and possibly the nonfree -bad if there is one, for additional demuxers (though less critical, base/good/ugly cover many already).
This is the error I'm getting when I launch frogr with G_MESSAGES_DEBUG=all: 'Could not construct pipeline: no element "gdkpixbufsink"' I googled a bit and it seems this plugin (which seems was written by you, Tim :)) should be provided by gst-plugins-good > 1.0.2. I have installed gstreamer1-plugins-good 1.0.5 in my FC18, and this is what I get when I run the following command: $ rpm -ql gstreamer1-plugins-good-1.0.5-1.fc18.x86_64 | grep pixbuf /usr/share/gtk-doc/html/gst-plugins-good-plugins-1.0/gst-plugins-good-plugins-gdkpixbufoverlay.html /usr/share/gtk-doc/html/gst-plugins-good-plugins-1.0/gst-plugins-good-plugins-gdkpixbufsink.html /usr/share/gtk-doc/html/gst-plugins-good-plugins-1.0/gst-plugins-good-plugins-plugin-gdkpixbuf.html Curiously enough, it's mentioned in the documentation for the package, but there's not a single .so file in the package providing that plugin, even if there are many others: $ rpm -ql gstreamer1-plugins-good-1.0.5-1.fc18.x86_64 | grep "libgst.*\.so" | wc -l 60 Maybe it's a bug in the package for fedora?
Sounds like a packaging bug indeed. The debian package for 1.0.5 does contain it.
(In reply to comment #20) > Sounds like a packaging bug indeed. The debian package for 1.0.5 does contain > it. Reported: https://bugzilla.redhat.com/show_bug.cgi?id=911783
(In reply to comment #21) > (In reply to comment #20) > > Sounds like a packaging bug indeed. The debian package for 1.0.5 does contain > > it. > > Reported: > https://bugzilla.redhat.com/show_bug.cgi?id=911783 Already fixed in gstreamer1-plugins-good-1.0.5-3.fc18, present in fedora's updates-testing repository. Cool. Also, I tested the patch again and it works perfectly now so I have no more objections to it. I will be more than happy than making a final review and approving it as soon as Dominique uploads a new version addressing those comments of mine, unless you (Tim) has any other comment to make on it.
Created attachment 236360 [details] [review] Port to GStreamer 1.0
That one should now hopefully address everything... I built this code now with configure --enable-debug and there are no new warnings appearing. If I missed something, please let me know...
Review of attachment 236360 [details] [review]: (In reply to comment #24) > That one should now hopefully address everything... I built this code now with > configure --enable-debug and there are no new warnings appearing. > > If I missed something, please let me know... It's perfect! Please feel free to commit it when you want. Thanks!
Comment on attachment 236360 [details] [review] Port to GStreamer 1.0 Thanks for your patience in reviewing this patch... It's been committed in http://git.gnome.org/browse/frogr/commit/?id=f63040
(In reply to comment #26) > (From update of attachment 236360 [details] [review]) > Thanks for your patience in reviewing this patch... > > It's been committed in http://git.gnome.org/browse/frogr/commit/?id=f63040 Thanks to you for making it real!