GNOME Bugzilla – Bug 795366
debugutils: Add a testsrc element
Last modified: 2018-07-03 12:29:50 UTC
See commit message.
Created attachment 371119 [details] [review] debugutils: Add a testsrc element This is a simple Bin that will expose audiotestsrc or videotestsrc based on what is asked by the user either through the GstURIHandler API or through the "stream-types" property. This element also provides GstStream and GstStreamCollection so it is nicely usable from playbin3.
Created attachment 371120 [details] [review] debugutils: Add a testsrc element This is a simple Bin that will expose audiotestsrc or videotestsrc based on what is asked by the user either through the GstURIHandler API or through the "stream-types" property. This element also provides GstStream and GstStreamCollection so it is nicely usable from playbin3.
Created attachment 371128 [details] [review] debugutils: Add a testsrc element This is a simple Bin that will expose audiotestsrc or videotestsrc based on what is asked by the user either through the GstURIHandler API or through the "stream-types" property. This element also provides GstStream and GstStreamCollection so it is nicely usable from playbin3.
Created attachment 371129 [details] [review] debugutils: Add a testsrc element This is a simple Bin that will expose audiotestsrc or videotestsrc based on what is asked by the user either through the GstURIHandler API or through the "stream-types" property. This element also provides GstStream and GstStreamCollection so it is nicely usable from playbin3.
Review of attachment 371129 [details] [review]: This looks generally useful, had a few remarks though :) Also, I guess you could just add that to the autotools build, shouldn't be too annoyying :) ::: gst/debugutils/gsttestsrc.c @@ +30,3 @@ + * + * This element also provides GstStream and GstStreamCollection and + * thus the element is usefull for testing the new playbin3 infrastructure. useful* @@ +60,3 @@ + + gchar *uri; + gboolean is_live; Not used @@ +73,3 @@ +}; + +#define DEFAULT_LIVE FALSE Not used @@ +85,3 @@ +gst_test_src_uri_handler_get_protocols (GType type) +{ + static const gchar *protocols[] = { "test", NULL }; we might want a more specific protocol here, eg "testbin", what do you think ? @@ +94,3 @@ +{ + GstTestSrc *self = GST_TEST_SRC (handler); + gcha uri; probably doesn't compile ;) @@ +143,3 @@ + if (data->collection) { + GstStreamCollection *collection = data->collection; + data->collection = NULL; aren't you leaking data->collection here? @@ +307,3 @@ + G_IMPLEMENT_INTERFACE (GST_TYPE_URI_HANDLER, gst_test_src_uri_handler_init)) + + static void indent fail :) @@ +358,3 @@ + GstStateChangeReturn result = GST_STATE_CHANGE_FAILURE; + + switch (transition) { This whole switch case doesn't seem very useful :P @@ +406,3 @@ + gobject_class->set_property = gst_test_src_set_property; + + /** bad indent?
Created attachment 371130 [details] [review] debugutils: Add a testsrc element This is a simple Bin that will expose audiotestsrc or videotestsrc based on what is asked by the user either through the GstURIHandler API or through the "stream-types" property. This element also provides GstStream and GstStreamCollection so it is nicely usable from playbin3.
(In reply to Mathieu Duponchelle from comment #5) > Review of attachment 371129 [details] [review] [review]: > > This looks generally useful, had a few remarks though :) > > Also, I guess you could just add that to the autotools build, shouldn't be > too annoyying :) Erg, autotools yes :P > ::: gst/debugutils/gsttestsrc.c > @@ +30,3 @@ > + * > + * This element also provides GstStream and GstStreamCollection and > + * thus the element is usefull for testing the new playbin3 infrastructure. > > useful* Done > @@ +60,3 @@ > + > + gchar *uri; > + gboolean is_live; > > Not used FIXED. > @@ +73,3 @@ > +}; > + > +#define DEFAULT_LIVE FALSE > > Not used Done. > @@ +85,3 @@ > +gst_test_src_uri_handler_get_protocols (GType type) > +{ > + static const gchar *protocols[] = { "test", NULL }; > > we might want a more specific protocol here, eg "testbin", what do you think > ? Not sure, `testsrc` maybe? As you wish tbh :-) > > @@ +143,3 @@ > + if (data->collection) { > + GstStreamCollection *collection = data->collection; > + data->collection = NULL; > > aren't you leaking data->collection here? Nop, gst_event_new_stream_collection is transfer-full > @@ +307,3 @@ > + G_IMPLEMENT_INTERFACE (GST_TYPE_URI_HANDLER, > gst_test_src_uri_handler_init)) > + > + static void > > indent fail :) INDENT-OFF on :-) > @@ +358,3 @@ > + GstStateChangeReturn result = GST_STATE_CHANGE_FAILURE; > + > + switch (transition) { > > This whole switch case doesn't seem very useful :P Removed. > @@ +406,3 @@ > + gobject_class->set_property = gst_test_src_set_property; > + > + /** > > bad indent? Not sure what gst-indent did.
(In reply to Thibault Saunier from comment #7) > (In reply to Mathieu Duponchelle from comment #5) > > Nop, gst_event_new_stream_collection is transfer-full > The return value is transfer full, not the parameter afaict
Created attachment 371131 [details] [review] debugutils: Add a testsrcbin element This is a simple Bin that will expose audiotestsrc or videotestsrc based on what is asked by the user either through the GstURIHandler API or through the "stream-types" property. This element also provides GstStream and GstStreamCollection so it is nicely usable from playbin3.
Review of attachment 371131 [details] [review]: Good to go for me, please push after fixing these last two remarks, thanks :) ::: gst/debugutils/gsttestsrcbin.c @@ +21,3 @@ + +/** + * SECTION:element-testsrc testsrcbin now @@ +143,3 @@ + GstStreamCollection *collection = data->collection; + data->collection = NULL; + gst_pad_push_event (pad, gst_event_new_stream_collection (collection)); just pass data->collection here then gst_object_replace :)
Attachment 371131 [details] pushed as 4984af5 - debugutils: Add a testsrcbin element
*** Bug 781441 has been marked as a duplicate of this bug. ***