GNOME Bugzilla – Bug 629157
Move video frame conversion from playback plugin to libgstvideo
Last modified: 2010-09-15 09:40:29 UTC
This moves the gstscreenshot code from the playback plugin to libgstvideo, allowing apps/plugins an easy way to convert video frames. In addition to what it previously offered (converting between raw video formats), this version can also encode to image formats if an encoder is available.
Created attachment 169847 [details] [review] video: Add new method for converting a video frame
Created attachment 169848 [details] [review] playback: Switch to using gst_video_convert_frame
Comment on attachment 169847 [details] [review] video: Add new method for converting a video frame >+#include <glib-object.h> >+#include <string.h> >+#include "video.h" glib-object.h include looks unnecessary >+static gboolean >+caps_are_raw (GstCaps * caps) >+{ >+ static GstCaps *raw_video = NULL; >+ static gsize once = 0; >+ >+ if (g_once_init_enter (&once)) { >+ raw_video = gst_caps_from_string ("video/x-raw-yuv;video/x-raw-rgb"); >+ g_once_init_leave (&once, 1); >+ } >+ >+ return gst_caps_can_intersect (caps, raw_video); >+} How about GstStaticCaps? I think something involving gst_structure_has_name() might be better though.. >+/* takes ownership of the input buffer */ >+/** >+ * gst_video_convert_frame: >+ * @buf: a #GstBuffer >+ * @to_caps: the #GstCaps to convert to >+ * >+ * Converts a raw video buffer into the specified output caps. >+ * >+ * The output caps can be any raw video formats or any image formats (jpeg, png, ...). >+ * >+ * The width, height and pixel-aspect-ratio can also be specified in the output caps. >+ * >+ * Returns: The converted #GstBuffer, or %NULL if an error happened. >+ */ >+GstBuffer * >+gst_video_convert_frame (GstBuffer * buf, GstCaps * to_caps) API that takes ownership of the arguments passed is always a bit awkward IMHO and should only be a last resort if needed for performance reasons or if it considerably simplifies usage. In this case this seems unnecessary to me and I would not make the function take ownership of the buffer, and also mark the GstCaps as const GstCaps *. >+ from_caps = GST_BUFFER_CAPS (buf); >+ g_return_val_if_fail (from_caps != NULL, NULL); g_return_val_if_fail (GST_BUFFER_CAPS (buf) != NULL, NULL) might make it clearer to users of this API what they did wrong. Should maybe also check for input caps != NULL and GST_IS_CAPS and GST_CAPS_IS_SIMPLE and gst_caps_is_fixed(). >+ /* Add black borders if necessary to keep the DAR */ >+ g_object_set (vscale, "add-borders", TRUE, NULL); This makes me wonder if we should add an option_flags parameter to the function. (User might prefer cropping, for example). >+ msg = >+ gst_bus_poll (bus, GST_MESSAGE_ERROR | GST_MESSAGE_ASYNC_DONE, >+ 25 * GST_SECOND); Should use gst_bus_timed_pop_filtered() here instead. Should definitely not iterate any GLib main loops IMHO. Do we also need a timeout argument? An async variant of the function? >+ if (result) { >+ GST_DEBUG ("conversion successful: result = %p", result); >+ } else { >+ GST_WARNING ("prerolled but no result frame?!"); >+ } Shouldn't this be an error of some instead? Instead of a GST_WARNING >+ case GST_MESSAGE_ERROR:{ >... >+ gst_message_parse_error (msg, &error, &dbg); >+ if (error) { >+ g_warning ("Could not convert video frame: %s", error->message); >+ GST_DEBUG ("%s [debug: %s]", error->message, GST_STR_NULL (dbg)); >+ g_error_free (error); >+ } else { >+ g_warning ("Could not convert video frame (and NULL error!)"); >... >+ } else { >+ g_warning ("Could not convert video frame: %s", >+ "timeout during conversion"); >+ } >... >+ /* ERRORS */ >+no_encoder: >+ { >+ g_warning ("could not find an encoder for provided caps"); >+ return NULL; >+ } >+no_elements: >+ { >+ g_warning ("Could not convert video frame: %s", error->message); >+ g_error_free (error); >+ return NULL; >+ } >+no_pipeline: >+ { >+ g_warning ("Could not convert video frame: %s", >+ "no pipeline (unknown error)"); >+ return NULL; >+ } >+link_failed: >+ { >+ g_warning ("Could not convert video frame: %s", "failed to link elements"); >+ gst_object_unref (pipeline); >+ return NULL; >+ } g_warning does not really seem like an appropriate form of error handling for such a function. We should add a GError ** argument as well for proper error handling.
(In reply to comment #3) > (From update of attachment 169847 [details] [review]) > >+#include <glib-object.h> > >+#include <string.h> > >+#include "video.h" > > glib-object.h include looks unnecessary > REMOVED > >+static gboolean > >+caps_are_raw (GstCaps * caps) > >+{ > >+ static GstCaps *raw_video = NULL; > >+ static gsize once = 0; > >+ > >+ if (g_once_init_enter (&once)) { > >+ raw_video = gst_caps_from_string ("video/x-raw-yuv;video/x-raw-rgb"); > >+ g_once_init_leave (&once, 1); > >+ } > >+ > >+ return gst_caps_can_intersect (caps, raw_video); > >+} > > How about GstStaticCaps? I think something involving gst_structure_has_name() > might be better though.. CHANGED > > > >+/* takes ownership of the input buffer */ > >+/** > >+ * gst_video_convert_frame: > >+ * @buf: a #GstBuffer > >+ * @to_caps: the #GstCaps to convert to > >+ * > >+ * Converts a raw video buffer into the specified output caps. > >+ * > >+ * The output caps can be any raw video formats or any image formats (jpeg, png, ...). > >+ * > >+ * The width, height and pixel-aspect-ratio can also be specified in the output caps. > >+ * > >+ * Returns: The converted #GstBuffer, or %NULL if an error happened. > >+ */ > >+GstBuffer * > >+gst_video_convert_frame (GstBuffer * buf, GstCaps * to_caps) > > API that takes ownership of the arguments passed is always a bit awkward IMHO > and should only be a last resort if needed for performance reasons or if it > considerably simplifies usage. That comment was leftover bogus. > > In this case this seems unnecessary to me and I would not make the function > take ownership of the buffer, and also mark the GstCaps as const GstCaps *. DONE > > >+ from_caps = GST_BUFFER_CAPS (buf); > >+ g_return_val_if_fail (from_caps != NULL, NULL); > > g_return_val_if_fail (GST_BUFFER_CAPS (buf) != NULL, NULL) might make it > clearer to users of this API what they did wrong. > > Should maybe also check for input caps != NULL and GST_IS_CAPS and > GST_CAPS_IS_SIMPLE and gst_caps_is_fixed(). FIXED > > > >+ /* Add black borders if necessary to keep the DAR */ > >+ g_object_set (vscale, "add-borders", TRUE, NULL); > > This makes me wonder if we should add an option_flags parameter to the > function. (User might prefer cropping, for example). Too bad for them. > > > >+ msg = > >+ gst_bus_poll (bus, GST_MESSAGE_ERROR | GST_MESSAGE_ASYNC_DONE, > >+ 25 * GST_SECOND); > > Should use gst_bus_timed_pop_filtered() here instead. Should definitely not > iterate any GLib main loops IMHO. FIXED > > Do we also need a timeout argument? An async variant of the function? They can be added later > > > >+ if (result) { > >+ GST_DEBUG ("conversion successful: result = %p", result); > >+ } else { > >+ GST_WARNING ("prerolled but no result frame?!"); > >+ } > > Shouldn't this be an error of some instead? Instead of a GST_WARNING fixed > > > >+ case GST_MESSAGE_ERROR:{ > >... > >+ gst_message_parse_error (msg, &error, &dbg); > >+ if (error) { > >+ g_warning ("Could not convert video frame: %s", error->message); > >+ GST_DEBUG ("%s [debug: %s]", error->message, GST_STR_NULL (dbg)); > >+ g_error_free (error); > >+ } else { > >+ g_warning ("Could not convert video frame (and NULL error!)"); > >... > >+ } else { > >+ g_warning ("Could not convert video frame: %s", > >+ "timeout during conversion"); > >+ } > >... > >+ /* ERRORS */ > >+no_encoder: > >+ { > >+ g_warning ("could not find an encoder for provided caps"); > >+ return NULL; > >+ } > >+no_elements: > >+ { > >+ g_warning ("Could not convert video frame: %s", error->message); > >+ g_error_free (error); > >+ return NULL; > >+ } > >+no_pipeline: > >+ { > >+ g_warning ("Could not convert video frame: %s", > >+ "no pipeline (unknown error)"); > >+ return NULL; > >+ } > >+link_failed: > >+ { > >+ g_warning ("Could not convert video frame: %s", "failed to link elements"); > >+ gst_object_unref (pipeline); > >+ return NULL; > >+ } > > g_warning does not really seem like an appropriate form of error handling for > such a function. We should add a GError ** argument as well for proper error > handling.
Pushed the changes to the 'convertframe' branch here : http://cgit.freedesktop.org/~bilboed/gst-plugins-base/
What I meant was that all the g_warnings and that GST_WARNING should be reported to the caller via a GError ** property.
Branch now updated with GError ** argument
A test for this would also be nice ;) You're leaking elements, if the first ones can be created by a later one can't. Now if you make the timeout configurable I have nothing to complain anymore :) I'll write an async version of this once you pushed it or the final version is in bugzilla
branch updated with fixes for leaked elements and configurable timeout.
Pushed, together with an async variant and tests. commit 168aceb3da00186f21297a679fcdf5a5e21e2ea0 Author: Edward Hervey <bilboed@bilboed.com> Date: Thu Sep 9 14:11:52 2010 +0200 playback: Switch to using gst_video_convert_frame https://bugzilla.gnome.org/show_bug.cgi?id=629157 commit 349a76ed19662ce7894fecbdeb9bf7e1ca5f91db Author: Edward Hervey <bilboed@bilboed.com> Date: Thu Sep 9 13:44:54 2010 +0200 video: Add new method for converting a video frame https://bugzilla.gnome.org/show_bug.cgi?id=629157
+ void + gst_video_convert_frame_async(GstBuffer *buf, const GstCaps *to_caps, + GstClockTime timeout, GstVideoConvertFrameCallback callback, + gpointer user_data); Do we need a GDestroyNotify here as well, for bindings? Also, is the bug still open on purpose?
(In reply to comment #11) > Do we need a GDestroyNotify here as well, for bindings? Good question, IMHO we shouldn't because the callback is guaranteed to be called exactly one single time, always. Edward, what are the Python bindings expecting? > Also, is the bug still open on purpose? Well, no. But we could keep it open for this discussion :)
Ok, binding generators need a destroy notify as otherwise they can't know that the callback is called exactly once. An gtk-doc annotation would be better though