GNOME Bugzilla – Bug 781958
harness: Abort when failed to construct the specified pipeline
Last modified: 2017-05-04 20:10:31 UTC
I found this while creating a new plugin. It was entirely my mistake not to initialize my plugin properly but the error message was a bit cryptic IMHO. So I asked in #gstreamer and __tim gave me some advice: 22:17 < yashi> but gst_harness_add_parse() doesn't have a way to propage error gst_parse_launch_full() find 22:19 < __tim> I think you should just abort 22:20 < __tim> and let the caller do checks 22:20 < __tim> you could add a new function that doesn't abort if you think that's useful 22:20 < __tim> but realistically, then the unit test will do fail_unless (harness != NULL); 22:20 < __tim> so it just moves things elsewhere So here I am crafted a few lines of change to gst_harness_add_parse(). The following is the commit message in the patch. --- >8 --- >8 --- Even though gst_harness_new_parse() doesn't find the specified element, it returns without any error. Then a succeeding call to gst_harness_set_sink_caps_str() causes an error like this: Unexpected critical/warning: gst_pad_push_event: assertion 'GST_IS_PAD (pad)' failed This is a bit cryptic and doesn't give users any clue what was going on. gst_harness_new_parse() calls gst_harness_add_parse() with a newly created empty harness and a pipeline description string, but gst_harness_add_parse() does not have a way to propagate the error back to the caller. Since the function, gst_harness_add_parse(), is a public API, it's not a good idea to change its signature. This patch, instead, makes the function to g_error() when it finds any error. With this change the same error prints: ** (myelement-test:25345): ERROR **: Unable to create pipeline 'bin.( myelement )': no element "myelement" The current implementation of gst_parse_launch_full() doesn't return partially constructed pipeline when GST_PARSE_FLAG_FATAL_ERRORS is specified, however, this patch also adds a check for it.
Created attachment 350752 [details] [review] harness: Abort when failed to construct the specified pipeline Even though gst_harness_new_parse() doesn't find the specified element, it returns without any error. Then a succeeding call to gst_harness_set_sink_caps_str() causes an error like this: Unexpected critical/warning: gst_pad_push_event: assertion 'GST_IS_PAD (pad)' failed This is a bit cryptic and doesn't give users any clue what was going on. gst_harness_new_parse() calls gst_harness_add_parse() with a newly created empty harness and a pipeline description string, but gst_harness_add_parse() does not have a way to propagate the error back to the caller. Since the function, gst_harness_add_parse(), is a public API, it's not a good idea to change its signature. This patch, instead, makes the function to g_error() when it finds any error. With this change the same error prints: ** (myelement-test:25345): ERROR **: Unable to create pipeline 'bin.( myelement )': no element "myelement" The current implementation of gst_parse_launch_full() doesn't return partially constructed pipeline when GST_PARSE_FLAG_FATAL_ERRORS is specified, however, this patch also adds a check for it.
Comment on attachment 350752 [details] [review] harness: Abort when failed to construct the specified pipeline Looks good to me, thanks. I'd use one error message for both cases ("Unable to create pipeline"). I think it's enough to check for error != NULL, this will include the bin == NULL case too.
I think GstHarness should abort in this case (as the patch does) instead of silently failing and returning, but CC-ing Havard in case this was done like this intentionally for some reason.
(In reply to Tim-Philipp Müller from comment #3) > I think GstHarness should abort in this case (as the patch does) instead of > silently failing and returning, but CC-ing Havard in case this was done like > this intentionally for some reason. Completely agree! Fail early! I would be happy with a simple g_assert (bin) as well. :)
It would have to be g_assert (err == NULL), but I think the proposed failure message is nicer. Keep in mind that people might run your tests in an environment where some plugins are missing, so that helps figure out what to install.
Perfect!
Created attachment 350840 [details] [review] harness: Abort when failed to construct the specified pipeline Here is a new patch which refects your comments.
Comment on attachment 350840 [details] [review] harness: Abort when failed to construct the specified pipeline Thanks, will push that once git master is open again.
commit 5d40e49d12ecc9202c446909b0cc410d0bbc2cbb Author: Yasushi SHOJI <yashi@atmark-techno.com> Date: Sun Apr 30 12:10:49 2017 +0900 harness: Abort when failed to construct the specified pipeline gst_harness_new_parse() returns without any error even if it doesn't find the specified element. Then a succeeding call to gst_harness_set_sink_caps_str() causes an error like this: Unexpected critical/warning: gst_pad_push_event: assertion 'GST_IS_PAD (pad)' failed This is a bit cryptic and doesn't give users any clue what was going on. gst_harness_new_parse() calls gst_harness_add_parse() with a newly created empty harness and the given pipeline description string, but gst_harness_add_parse() does not have a way to propagate the error back to the caller. Since the function, gst_harness_add_parse(), is a public API, it's not a good idea to change its signature. This patch, instead, makes the function to g_error() when it discovers any error. With this change the same error prints: ** (myelement-test:25345): ERROR **: Unable to create pipeline 'bin.( myelement )': no element "myelement" The current implementation of gst_parse_launch_full() doesn't return partially constructed pipeline when GST_PARSE_FLAG_FATAL_ERRORS is specified, however, this patch also adds a check for it. https://bugzilla.gnome.org/show_bug.cgi?id=781958