[Bug 698712] playbin: autoplug video decoder and sink based on caps features

[GStreamer (bugzilla.gnome.org)]

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

Sebastian Dröge <slomo> changed:

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

--- Comment #2 from Sebastian Dröge <slomo at circular-chaos.org> 2013-04-24 12:33:43 UTC ---
Review of attachment 242314:
 --> (https://bugzilla.gnome.org/review?bug=698712&attachment=242314)

::: gst/playback/gstplaybin2.c
@@ +3300,3 @@
+
+  /* comparison based on the name of sink elements */
+  return strcmp (GST_OBJECT_NAME (fs1), GST_OBJECT_NAME (fs2));

And if they are equal check the name of the decoder elements?

@@ +3327,3 @@
+        ve->vdec = gst_object_ref (d_factory);
+        ve->vsink = gst_object_ref (s_factory);
+        ve_list = g_list_prepend (ve_list, ve);

You could keep this list smaller by only including decoder-sink combinations
that are compatible by their template caps

@@ +3355,3 @@
+      tmp_templ = walk->data;
+      if (tmp_templ->direction == GST_PAD_SRC)
+        d_templ = tmp_templ;

Shouldn't you break on the first srcpad here?

@@ +3371,3 @@
+      for (j = 0; j < s_caps_size; j++) {
+        s_features = gst_caps_get_features ((const GstCaps *) s_tmpl_caps, j);
+        if (gst_caps_features_is_equal (d_features, s_features))

You should also check that the corresponding structures are compatible

@@ +3445,3 @@
   GST_PLUGIN_FEATURE_LIST_DEBUG (mylist);

+  /* check whether the caps are asking for a list of video_decoders */

All this shouldn't be done just for video

@@ +3470,3 @@
+        GST_ELEMENT_FACTORY_TYPE_MEDIA_IMAGE, GST_RANK_MARGINAL);
+    video_sinks =
+        g_list_sort (video_sinks, gst_plugin_feature_rank_compare_func);

I think these lists should be created at a central place like the other element
factory lists already, otherwise this needs some locking here as the autoplug
signals can be called from multiple threads at once iirc

@@ +3474,3 @@
+    GST_SOURCE_GROUP_LOCK (group);
+    /* create the video element list */
+    group->velements = create_velements_list (mylist, video_sinks);

Try to find a better name than mylist :)

@@ +3725,3 @@
+          ve = (GstVideoElement *) tmp->data;
+          if (!ve->is_dirty && factory == ve->vdec) {
+            ve->is_dirty = 1;

This is not threadsafe, and why is this loop necessary at all? Shouldn't the
overall loop only be over all velements that have factory==ve->vdec?

@@ +3797,3 @@
+          GST_SOURCE_GROUP_LOCK (group);
+          gst_element_set_state (group->video_sink, GST_STATE_NULL);
+          gst_object_unref (group->video_sink);

Why do you destroy the previously selected sink here?

@@ +3812,3 @@
+      for (; tmp; tmp = tmp->next) {
+        ve = (GstVideoElement *) tmp->data;
+        ve->is_dirty = 1;

Also not threadsafe and see above ;)

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