GNOME Bugzilla – Bug 621929
[PLUGIN-MOVE] move jack plugin from -bad to -good
Last modified: 2011-01-02 20:49:14 UTC
Created attachment 163958 [details] [review] removes gstjackbin.c I'd like to fix any remaining issues in the jack plugins so that they could be moved from plugins-bad to plugins-good. To get the ball rolling, I think the file jackbin.c should be removed as it hasn't been touched in years and isn't used. Additionally, Stefan Kost had mentioned wanting a client property for the source and sink plugins, so that applications could pass in their own jack client's to be used. This could be done before the move.
Created attachment 169371 [details] [review] adds "client" property to jackaudiosrc and jackaudiosink
Created attachment 169372 [details] testcase for new "client" property
Review of attachment 169371 [details] [review]: Looks good. Just a small change needed. ::: ext/jack/gstjackaudiosink.c @@ +690,3 @@ + g_object_class_install_property (gobject_class, PROP_CLIENT, + g_param_spec_boxed ("client", "JackClient", "Handle for jack client", + GST_TYPE_JACK_CLIENT, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); Please add GST_PARAM_MUTABLE_READY (its not enforced, but helps to document the restriction). ::: ext/jack/gstjackaudiosrc.c @@ +713,3 @@ + g_param_spec_boxed ("client", "JackClient", "Handle for jack client", + GST_TYPE_JACK_CLIENT, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); + Same as in sink.
Created attachment 169614 [details] [review] adds "client" property to jackaudiosrc and jackaudiosink
Created attachment 169623 [details] [review] testcase for new "client" property
Looks good now. Thanks for the patches!
Ok hopefully this can go into -good for the next release.
> Ok hopefully this can go into -good for the next release. The next cycle is core/base/good, you'll have to wait for the cycle after that to move anything from -bad to -good. Besides, it's not entirely clear to me that the moving-plugin guidelines/requirements have been fulfilled (code review etc.). I'd like to see Wim sign off on this particular plugin move.
Created attachment 169884 [details] [review] adds translatable text for server not found error.
Created attachment 169885 [details] summary of how the jack plugins satisfy the checklist for -good.
Comment on attachment 169884 [details] [review] adds translatable text for server not found error. committed with the Makefile.am change missing in the patch
check-list looks good +1 for moving from my side
+ case PROP_CLIENT: + if (GST_STATE (src) == GST_STATE_NULL || + GST_STATE (src) == GST_STATE_READY) { + src->jclient = g_value_get_boxed (value); + } + break; There is no need to check the states. It can't be done in a threadsafe way and the app has no way of knowning if it actually worked or not. You would just use the flags on the paramspec, which is a hint that the change only takes effect when in those states.
check-list looks good to me too. +1
Created attachment 170954 [details] [review] don't check states when setting client property
> There is no need to check the states. It can't be done in a threadsafe way and > the app has no way of knowing if it actually worked or not. The app has a way of knowing: it can check via g_object_get() if the setting was applied or not. But since the property is flagged correctly with GST_PARAM_MUTABLE_*, setting the property in other states is a programming error anyway. I think this could (should?) be checked with a g_return_if_fail() or so at the beginning of the function some day. There's a convenience macro in bugzilla somewhere for this, but we didn't think it was quite right yet I guess..
Moved to -good.