After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 621929 - [PLUGIN-MOVE] move jack plugin from -bad to -good
[PLUGIN-MOVE] move jack plugin from -bad to -good
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal blocker
: 0.10.27
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2010-06-17 20:30 UTC by Tristan Matthews
Modified: 2011-01-02 20:49 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
removes gstjackbin.c (11.08 KB, patch)
2010-06-17 20:30 UTC, Tristan Matthews
committed Details | Review
adds "client" property to jackaudiosrc and jackaudiosink (11.00 KB, patch)
2010-09-02 16:01 UTC, Tristan Matthews
needs-work Details | Review
testcase for new "client" property (1.82 KB, text/plain)
2010-09-02 16:02 UTC, Tristan Matthews
  Details
adds "client" property to jackaudiosrc and jackaudiosink (10.94 KB, patch)
2010-09-06 20:13 UTC, Tristan Matthews
committed Details | Review
testcase for new "client" property (4.09 KB, patch)
2010-09-06 21:19 UTC, Tristan Matthews
committed Details | Review
adds translatable text for server not found error. (2.29 KB, patch)
2010-09-09 18:51 UTC, Tristan Matthews
committed Details | Review
summary of how the jack plugins satisfy the checklist for -good. (4.38 KB, text/plain)
2010-09-09 19:05 UTC, Tristan Matthews
  Details
don't check states when setting client property (1.04 KB, patch)
2010-09-23 18:15 UTC, Tristan Matthews
none Details | Review

Description Tristan Matthews 2010-06-17 20:30:21 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.
Comment 1 Tristan Matthews 2010-09-02 16:01:47 UTC
Created attachment 169371 [details] [review]
adds "client" property to jackaudiosrc and jackaudiosink
Comment 2 Tristan Matthews 2010-09-02 16:02:32 UTC
Created attachment 169372 [details]
testcase for new "client" property
Comment 3 Stefan Sauer (gstreamer, gtkdoc dev) 2010-09-06 19:44:18 UTC
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.
Comment 4 Tristan Matthews 2010-09-06 20:13:07 UTC
Created attachment 169614 [details] [review]
adds "client" property to jackaudiosrc and jackaudiosink
Comment 5 Tristan Matthews 2010-09-06 21:19:16 UTC
Created attachment 169623 [details] [review]
testcase for new "client" property
Comment 6 Stefan Sauer (gstreamer, gtkdoc dev) 2010-09-06 21:46:20 UTC
Looks good now. Thanks for the patches!
Comment 7 Tristan Matthews 2010-09-07 20:27:09 UTC
Ok hopefully this can go into -good for the next release.
Comment 8 Tim-Philipp Müller 2010-09-07 21:43:20 UTC
> 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.
Comment 9 Tristan Matthews 2010-09-09 18:51:10 UTC
Created attachment 169884 [details] [review]
adds translatable text for server not found error.
Comment 10 Tristan Matthews 2010-09-09 19:05:53 UTC
Created attachment 169885 [details]
summary of how the jack plugins satisfy the checklist for -good.
Comment 11 Stefan Sauer (gstreamer, gtkdoc dev) 2010-09-14 14:41:09 UTC
Comment on attachment 169884 [details] [review]
adds translatable text for server not found error.

committed with the Makefile.am change missing in the patch
Comment 12 Stefan Sauer (gstreamer, gtkdoc dev) 2010-09-14 14:41:49 UTC
check-list looks good +1 for moving from my side
Comment 13 Wim Taymans 2010-09-14 14:49:58 UTC
+    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.
Comment 14 Wim Taymans 2010-09-14 14:57:50 UTC
check-list looks good to me too. +1
Comment 15 Tristan Matthews 2010-09-23 18:15:03 UTC
Created attachment 170954 [details] [review]
don't check states when setting client property
Comment 16 Tim-Philipp Müller 2010-09-23 19:06:56 UTC
> 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..
Comment 17 Tim-Philipp Müller 2011-01-02 20:49:14 UTC
Moved to -good.