[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

Edward Hervey <bilboed> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|0                           |1

--- Comment #4 from Edward Hervey <bilboed at gmail.com> 2010-09-09 14:23:32 UTC ---
(In reply to comment #3)
> (From update of attachment 169847 [details])
> >+#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.

-- 
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.