[gstreamer-bugs] [Bug 629157] Move video frame conversion from playback plugin to libgstvideo

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=629157
  GStreamer | gst-plugins-base | unspecified

Tim-Philipp Müller <t.i.m> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #169847|none                        |needs-work
             status|                            |

--- Comment #3 from Tim-Philipp Müller <t.i.m at zen.co.uk> 2010-09-09 13:22:24 UTC ---
(From update of attachment 169847)
>+#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.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.