The reason why it doesn't work with a basetransform in between is because basetransform doesn't forward the ACCEPT_CAPS query but it does a getcaps and _is_subset().

Yes, I think get_caps is missing (or broken).

Created attachment 245366 [details] [review]
pulsesink: Take a lock on the ringbuffer in acceptcaps

This is neede as a concurrent state change could pull the context or
stream out from under our feet.

Created attachment 245367 [details] [review]
pulsesink: Get rid of acceptcaps side-effects

The sink info callback should not have side-effects on the GstPulseSink
object since we are sometimes using with a dummy stream in acceptcaps.

Created attachment 245368 [details] [review]
pulsesink: Add a getcaps function

this allows us to have more fine-tuned caps in READY or above. However,
this is _really_ inefficient since we create a new stream and query sink
for every getcaps in READY, which on a simple gst-launch line happens
about 35 times. The next step is to cache getcaps results.

Created attachment 245369 [details] [review]
pulsesink: Cache the getcaps/acceptcaps probe stream

getcaps is called frequently during stream setup, and creating a new
stream each time is very inefficient. There's some more room for
optimisation by caching the queried sink formats as well, but this needs
some more changes to listen for format changes on the sink as well (for
when supported formats change between probe stream creation and sink
querying).

Review of attachment 245366 [details] [review]:

Seems a bit risky to hold the object lock that long over that many function calls. Did you check that nothing called there can ever take the object lock itself, and that there's no lock order inversion anywhere (you introduce object_lock > mainloop lock here)

Review of attachment 245368 [details] [review]:

::: ext/pulse/pulsesink.c
@@ +2021,3 @@
+          /* raw formats PA supports == raw formats GST supports, so skip
+           * setting formats (FIXME?) */
+          ret = gst_caps_from_string ("audio/x-raw");

gst_caps_new_empty_simple()

Could you also use the GstAudioInfo caps creation function here? There are some fields missing here :)

@@ +2053,3 @@
+
+  if (pa_format_info_get_prop_int (format, PA_PROP_FORMAT_RATE, &rate) == 0)
+    gst_caps_set_simple (ret, "rate", G_TYPE_INT, rate, NULL);

Should always have rate and channels set, in the worst case the [1,MAX] range. Alternatively intersect with the template caps afterwards

@@ +2131,3 @@
+  }
+
+  if (!pbuf->stream) {

It should only create a stream and do all that when in >=READY state

@@ +2141,3 @@
+    pa_format_info_set_sample_format (format, PA_SAMPLE_S16LE);
+    pa_format_info_set_rate (format, GST_AUDIO_DEF_RATE);
+    pa_format_info_set_channels (format, GST_AUDIO_DEF_CHANNELS);

Can using this format ever fail although in general stuff would work?

(In reply to comment #7)
> Review of attachment 245366 [details] [review]:
> 
> Seems a bit risky to hold the object lock that long over that many function

I couldn't think of a better way to deal with the problem of making sure that only the real stream or probe stream can exist at any given time.

That said, there's potential to improve the lock granularity by dropping the object lock once the probe stream is set up. I'll try to hack this up shortly.

> calls. Did you check that nothing called there can ever take the object lock
> itself, and that there's no lock order inversion anywhere (you introduce
> object_lock > mainloop lock here)

The only callers of GST_OBJECT_LOCK within that file are the ones I've added. All other locks on the object are taken at the GstAudioRingBuffer class level so the object lock -> mainloop lock order should always hold.

(In reply to comment #8)
> Review of attachment 245368 [details] [review]:
> 
> ::: ext/pulse/pulsesink.c
> @@ +2021,3 @@
> +          /* raw formats PA supports == raw formats GST supports, so skip
> +           * setting formats (FIXME?) */
> +          ret = gst_caps_from_string ("audio/x-raw");
> 
> gst_caps_new_empty_simple()
> 
> Could you also use the GstAudioInfo caps creation function here? There are some
> fields missing here :)

I've fixed this and the next comment a little differently (and IMO less messily). Patch coming soon.

> @@ +2053,3 @@
> +
> +  if (pa_format_info_get_prop_int (format, PA_PROP_FORMAT_RATE, &rate) == 0)
> +    gst_caps_set_simple (ret, "rate", G_TYPE_INT, rate, NULL);
> 
> Should always have rate and channels set, in the worst case the [1,MAX] range.
> Alternatively intersect with the template caps afterwards

See previous note.

> @@ +2131,3 @@
> +  }
> +
> +  if (!pbuf->stream) {
> 
> It should only create a stream and do all that when in >=READY state

When we get to this point, we know we're in READY because there is a pbuf->context.

> @@ +2141,3 @@
> +    pa_format_info_set_sample_format (format, PA_SAMPLE_S16LE);
> +    pa_format_info_set_rate (format, GST_AUDIO_DEF_RATE);
> +    pa_format_info_set_channels (format, GST_AUDIO_DEF_CHANNELS);
> 
> Can using this format ever fail although in general stuff would work?

It really should not. Eventually, as the comment notes, I'd like to be able to connect with an "ANY" format. This needs some fixes with how the formats handling happens in PulseAudio, which I am doing independently.

(In reply to comment #9)

> > calls. Did you check that nothing called there can ever take the object lock
> > itself, and that there's no lock order inversion anywhere (you introduce
> > object_lock > mainloop lock here)
> 
> The only callers of GST_OBJECT_LOCK within that file are the ones I've added.
> All other locks on the object are taken at the GstAudioRingBuffer class level
> so the object lock -> mainloop lock order should always hold.

Note that various GStreamer functions can take the object lock too, like gst_object_get_parent() ;)

But if you checked all this I'm fine with the patch

Created attachment 246160 [details] [review]
pulsesink: Add a getcaps function

this allows us to have more fine-tuned caps in READY or above. However,
this is _really_ inefficient since we create a new stream and query sink
for every getcaps in READY, which on a simple gst-launch line happens
about 35 times. The next step is to cache getcaps results.

Review of attachment 246160 [details] [review]:

Looks good, can be pushed after fixing the second comment and if you think nothing else is missing

::: ext/pulse/pulsesink.c
@@ +2053,3 @@
+    gst_caps_set_simple (ret, "channels", G_TYPE_INT, channels, NULL);
+
+  /* FIXME: add channel map */

What's missing to implement that?

@@ +2222,3 @@
 
+  pad_caps = gst_pad_get_pad_template_caps (GST_BASE_SINK_PAD (psink));
+  ret = gst_caps_can_intersect (pad_caps, caps);

Should be gst_caps_is_subset(caps, pad_caps)

@@ +2300,3 @@
   } else {
     /* We're in READY, let's connect a stream to see if the format is
+     * accepted by whatever sink we're routed to */

Even fixed a typo ;)

Created attachment 246381 [details] [review]
pulsesink: Add a getcaps function

(updated to be able to read rate/channels as int array or range as well)

Created attachment 246382 [details] [review]
pulsesink: Cache the getcaps/acceptcaps probe stream

(updated to wait for probe destruction before creating the actual stream, else
compressed streams will fail to be created)

The following fixes have been pushed:

Created attachment 246676 [details] [review]
Require libpulse >= 2.0 for pa_format_info_get_prop_*
 functions.


The pa_format_info_get_prop_* functions were added in libpulse 2.0 (http://freedesktop.org/software/pulseaudio/doxygen/format_8h.html#aae6ab2ad009c17d0f999ce1011d99f08), so commit 14e784f9fc629af4c33b41dae159aa5060fb8f5f makes gst-plugins-good not build on Ubuntu 12.04:

  CC     libgstpulse_la-pulseutil.lo
pulsesink.c: In function 'gst_pulse_format_info_int_prop_to_value':
pulsesink.c:2028:3: error: implicit declaration of function 'pa_format_info_get_prop_int' [-Werror=implicit-function-declaration]
pulsesink.c:2028:3: error: nested extern declaration of 'pa_format_info_get_prop_int' [-Werror=nested-externs]
pulsesink.c:2032:3: error: implicit declaration of function 'pa_format_info_get_prop_int_array' [-Werror=implicit-function-declaration]
pulsesink.c:2032:3: error: nested extern declaration of 'pa_format_info_get_prop_int_array' [-Werror=nested-externs]
pulsesink.c:2043:3: error: implicit declaration of function 'pa_format_info_get_prop_int_range' [-Werror=implicit-function-declaration]
pulsesink.c:2043:3: error: nested extern declaration of 'pa_format_info_get_prop_int_range' [-Werror=nested-externs]
pulsesink.c: In function 'gst_pulse_format_info_to_caps':
pulsesink.c:2066:7: error: implicit declaration of function 'pa_format_info_to_sample_spec' [-Werror=implicit-function-declaration]
pulsesink.c:2066:7: error: nested extern declaration of 'pa_format_info_to_sample_spec' [-Werror=nested-externs]
pulsesink.c:2068:7: error: implicit declaration of function 'pa_format_info_get_prop_string' [-Werror=implicit-function-declaration]
pulsesink.c:2068:7: error: nested extern declaration of 'pa_format_info_get_prop_string' [-Werror=nested-externs]

This patch just requires libpulse >= 2.0, since that will fix the build. It may be nicer to keep building pulsesink and just use the old behavior if PA_MAJOR < 2.

Instead of this it looks possible to just conditionally disable the new stuff added by this patch if PA < 2.0 is used.

The following fix has been pushed:
0145702 pulsesink: Make 2.0 dependency optional

Thanks for the patch, Brendan. I've made the 2.0 optional instead for now,
since it's "only" a year old. :)


 functions.
 - none

Created attachment 246685 [details] [review]
pulsesink: Make 2.0 dependency optional

The getcaps function we added uses some pa_format_info_get_prop...
accessor functions that were only added in 2.0, so we only have our
getcaps implementation exist if we're compiling against libpulse 2.0 or
above.

Eventually, we could bump the minimum requirement to 2.0 or above.

I don't think this is a good/complete fix like this.

If I understand it right, now this bug continues to exist if pulseaudio < 2.0 is used, and it's only fixed in newer pulseaudio? If that is true, then we should remove all passthrough support for older pulseaudio versions IMHO, so this breakage doesn't happen.

(In reply to comment #21)
> I don't think this is a good/complete fix like this.
> 
> If I understand it right, now this bug continues to exist if pulseaudio < 2.0
> is used, and it's only fixed in newer pulseaudio? If that is true, then we
> should remove all passthrough support for older pulseaudio versions IMHO, so
> this breakage doesn't happen.

That's fair, but then I think the resulting change would be larger than is worth supporting for the gain of supporting older versions. Maybe we /should/ then just bump the requirement to 2.0.

Reverted my patch and applied Brendan's with some modification for the commit message. We now depend on PulseAudio >= 2.0 unconditionally.