[gstreamer-bugs] [Bug 627476] New profile library and encoding plugin

[GStreamer (bugzilla.gnome.org)]

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

--- Comment #5 from Sebastian Dröge <slomo at circular-chaos.org> 2010-08-20 11:18:11 UTC ---
Review of attachment 168378:
 --> (https://bugzilla.gnome.org/review?bug=627476&attachment=168378)

::: gst-libs/gst/pbutils/gstprofile.c
@@ +125,3 @@
+  /* FIXME : Free stream profiles */
+  g_list_foreach (prof->encodingprofiles,
+      (GFunc) gst_stream_encoding_profile_free, NULL);

This will work better if the stream profiles are mini objects... that could all
be unreffed that same way

@@ +153,3 @@
+  GstEncodingProfile *prof;
+
+  prof = g_new0 (GstEncodingProfile, 1);

GSlice unless you use miniobjects

@@ +157,3 @@
+  prof->name = g_strdup (name);
+  if (format)
+    prof->format = gst_caps_copy (format);

Maybe only ref the caps? Otherwise make the parameter const

@@ +186,3 @@
+  }
+  /* FIXME : Maybe we should have a better way to detect if an exactly 
+   * similar profile is already present */

Add a compare vfunc maybe... or compare by caps

@@ +214,3 @@
+  for (tmp = profile->encodingprofiles; tmp; tmp = g_list_next (tmp)) {
+    GstStreamEncodingProfile *sprof = (GstStreamEncodingProfile *) tmp->data;
+    gst_caps_append (res, gst_stream_encoding_profile_get_output_caps
(sprof));

gst_caps_merge() might be better

@@ +250,3 @@
+    prof = g_new0 (GstStreamEncodingProfile, 1);
+  prof->type = type;
+  prof->format = gst_caps_copy (format);

Maybe just ref here... and const parameters

::: gst-libs/gst/pbutils/gstprofile.h
@@ +43,3 @@
+ * @GST_ENCODING_PROFILE_AUDIO: audio stream
+ * @GST_ENCODING_PROFILE_TEXT: text/subtitle stream
+ * @GST_ENCODING_PROFILE_IMAGE: image

How are subpictures handled? What's the difference between image and video
except that image has a fixed 0/1 framerate?

@@ +77,3 @@
+  gchar     *name;
+  gchar     *category;
+  GList     *profiles;

Struct padding missing here and everywhere else

@@ +95,3 @@
+  GstCaps       *format;
+  gchar         *preset;
+  gboolean       multipass;

You could make a generic GstEncodingProfileBase and let the others inherit from
this one. GstStreamEncodingProfile shares all but one field with
GstEncodingProfile.

@@ +109,3 @@
+ * Invididual stream profile, to be used in #GstEncodingProfile
+ */
+struct _GstStreamEncodingProfile {

Use GstMiniObjects here maybe :)

@@ +138,3 @@
+GType                 gst_encoding_profile_get_type (void);
+GstEncodingProfile *  gst_encoding_profile_new (gchar *name, GstCaps *format,
+                        gchar *preset,

Mark the char * as const and copy internally (you probably already do that)

@@ +140,3 @@
+                        gchar *preset,
+                        gboolean multipass);
+GstEncodingProfile *  gst_encoding_profile_copy (GstEncodingProfile *profile);

const here too and for every other copy function and the char *

@@ +179,3 @@
+
+/*
+ * Application convenience methods (possibly to be added in gst-pb-utils)

That's what you're doing here, drop the comment ;) Or do you mean into another
header?

@@ +184,3 @@
+GstElement *gst_pb_utils_create_encoder(GstCaps *caps, gchar *preset, gchar
*name);
+GstElement *gst_pb_utils_create_encoder_format(gchar *format, gchar *preset,
+                           gchar *name);

const for the parameters again and for the other similar functions

@@ +192,3 @@
+
+/*
+ * GstPreset modifications

...and move this to core, with a separate bugreport and patch

::: gst-libs/gst/pbutils/gstprofileutils.c
@@ +1,1 @@
+/* GStreamer encoding profiles library utilities

And nothing is implemented here yet ;)

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