[gstreamer-bugs] [Bug 626181] [pbutils] Add enhanced gstfactorylists

[GStreamer (bugzilla.gnome.org)]

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

--- Comment #11 from Edward Hervey <bilboed at gmail.com> 2010-08-16 10:03:05 UTC ---
(In reply to comment #8)
> Review of attachment 167259 [details]:
> 
> First of all I don't think this belongs into core... There's stuff about audio
> and video in here.

  Maybe not everything, but the generic code and the enums could be moved
there.

> 
> Also, why don't you use a GstCaps instead of a GValueArray? That makes things a
> bit easier I guess... especially because your caps inside the value array only
> have a single, simple structure ;)

  For the caps part, I agree.

> 
> And then you might want to try to port subtitleoverlay to this. It needs a list
> of subtitle parsers/decoders and subtitle renderers.

  I'll keep that in mind when adding subtitle handling.

> 
> Other comments below
> 
> ::: gst-libs/gst/pbutils/gstfactorylists.c
> @@ +348,3 @@
> +    while (!gst_structure_foreach (st,
> +            (GstStructureForeachFunc) remove_range_foreach, st));
> +  }
> 
> For this you might want to take a look at
> http://svn.debian.org/viewsvn/pkg-gstreamer/unstable/gstreamer0.10/debian/gst-codec-info.c?revision=3298&view=markup
> 
> Search for "codec_data" and remove_min_max_fields(). I think that's a bit safer
> and better than simply removing *all* ranges.

  Agreed, but I wouldn't blindly remove the width/height/formats fields for
example. They will be needed, for example, if you have several different
encoders available which can only handle certain formats/sizes.

> 
> @@ +353,3 @@
> +
> +  /* Explode to individual structures */
> +  res2 = gst_caps_normalize (res);
> 
> Why is the normalizing here necessary?
> 
> @@ +376,3 @@
> +    tmpc = gst_caps_new_full (st, NULL);
> +
> +    /* Check if the caps are already present */
> 
> If you were using a GstCaps instead of a GValueArray of normalized, simple caps
> this could be done by gst_caps_merge_structure() ;)

  Yes on switching to caps instead of arrays.

> 
> ::: gst-libs/gst/pbutils/gstfactorylists.h
> @@ +40,3 @@
> + * @GST_FACTORY_LIST_MEDIA_VIDEO: Elements handling video media types
> + * @GST_FACTORY_LIST_MEDIA_AUDIO: Elements handling audio media types
> + * @GST_FACTORY_LIST_MEDIA_IMAGE: Elements handling image media types
> 
> Maybe add something for subtitles, subpictures and metadata here

  will do

> 
> @@ +61,3 @@
> +  GST_FACTORY_LIST_DEPAYLOADER  = (1 << 8),
> +
> +  GST_FACTORY_LIST_MAX_ELEMENTS = (1 << 16),
> 
> If you want to add special purpose elements here 16 might be too small

  I'll make it << 24, that should be enough.

> 
> @@ +101,3 @@
> +GValueArray * gst_factory_list_get_elements (GstFactoryListType type, GstRank
> minrank);
> +
> +void          gst_factory_list_debug        (GValueArray *array);
> 
> Should this debug function really be public?

  It was already there and is useful for code using factorylist.

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