[gstreamer-bugs] [Bug 626181] [pbutils] Add enhanced gstfactorylists
GStreamer (bugzilla.gnome.org)
bugzilla at gnome.org
Mon Aug 16 03:03:12 PDT 2010
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.
More information about the Gstreamer-bugs
mailing list