Created attachment 3477 GStreamer thumbnailing support The ffmpeg thumbnailer may not be suitable for everyone (codecs, hw acceleration, legal, etc) so I'm attaching a GStreamer-based thumbnailer. It uses a playbin with NULL sinks to do the decoding, and grabs a number of frames checking that they have interesting content. This patch is still a work in progress because the error paths leak, but I thought I'd submit it now to get an early review.
Yeah, the ffmpeg thumbnailer doesn't even load on my machine due to some policy in Fedora. So I'm happy to see someone working on a GStreamer-based thumbnailer for video files. Before I go into detail with my comments, let me note that I've just created a coding style document for tumbler which I think is worth looking at in order to maintain a consistent style: http://git.xfce.org/apps/tumbler/plain/CODING_STYLE So here's a quick review of your code (everything not mentioned is fine): * I am missing source code comments and assertions. Also, there is a mixture of different coding styles being used in your patch. Please have a look at the coding style document I mentioned and fix those inconsistencies. * gst-helper.c uses LGPL 2.1, not LGPL 2 or later. I wonder if that is a problem. I can ask a friend from Debian about this though. He's always after us concerning licensing issues. ;) * There is a function taken from Brickley in gst-thumbnailer.c, I suppose this was also licensed under LGPL 2.1, not LGPL 2 or later. * If there are "public" functions declared in a header file like gst-helper.c, then I prefer those functions to have a gst_helper_ prefix. Some static helper functions may exist without a prefix but usually I am trying to avoid this. Maybe we can simply move these functions into gst-thumbnailer.c (if that doesn't conflict with the LGPL 2/later license ? Everything else looks ok. I trust you in that it actually works and doesn't crash unexpectedly.
I checked the compatibility of LGPLv2.1 and LGPLv2+later. According to the matrix on http://www.gnu.org/licenses/gpl-faq.html#AllCompatibility copying LGPLv2.1 only code into a LGPLv2 or later project is perfectly fine. As long as the LGPLv2.1 code is still there, the project may not be relicensed to LGPLv3. I'm ok with that. So you don't need to change the license of the v2.1 files.
Created attachment 3591 Revised patch Revised patch. gst-helper is a separate file for two reasons: 1) because it's a copy of code from another module and so easier to keep in sync, and 2) because its long and not directly involved in the thumbnailing logic. I've added a prefix for clarity though. It shouldn't blatantly leak any more, although I haven't yet ran it through valgrind.
Adding myself to the CC. list
Ping?
Sorry for not responding yet. I will try to find some time to look at the updated patch this week.
Ok, after a quick test with a few .mov and .ogv videos (everything else didn't work for me for some reason) I pushed this to master (see below for the hashes and commit messages). I would appreciate if you could check for memory leaks again, I didn't check that myself. If you have any follow-up commits, just let me know. Thanks for the work! commit 3f496d6483500102f14a8df1f444f63547916441 Author: Jannis Pohlmann <jannis@xfce.org> Date: Fri May 20 01:27:53 2011 +0200 Make a few tiny whitespace/coding style adjustment. commit 88a5e9692fcc2f2b64c152f25a98721d631b2ad7 Author: Ross Burton <ross@linux.intel.com> Date: Mon Jan 31 15:48:34 2011 +0000 Add GStreamer-based thumbnailer for video thumbnails Signed-off-by: Jannis Pohlmann <jannis@xfce.org>