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 681247 - [0.11] pulsesrc breaks when reconfiguring
[0.11] pulsesrc breaks when reconfiguring
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal blocker
: 0.11.x
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks: 674179
 
 
Reported: 2012-08-05 17:23 UTC by Sjoerd Simons
Modified: 2012-09-10 11:37 UTC
See Also:
GNOME target: 3.6
GNOME version: 3.5/3.6


Attachments
testcase (880 bytes, text/plain)
2012-08-05 17:23 UTC, Sjoerd Simons
  Details
pulsesrc: Release ringbuffer before creating new stream (1.34 KB, patch)
2012-08-08 01:16 UTC, Olivier Crête
none Details | Review
Workaround (1.22 KB, patch)
2012-08-12 09:56 UTC, Sjoerd Simons
none Details | Review
This makes renegotiation work. Tested with the attached case -- you'll get a (6.17 KB, patch)
2012-08-14 11:47 UTC, Arun Raghavan
none Details | Review

Description Sjoerd Simons 2012-08-05 17:23:34 UTC
Created attachment 220393 [details]
testcase

When a reconfiguration is triggered pulsesrc unfortunately breaks as follows:
  ERROR:pulsesrc.c:1315:gst_pulsesrc_negotiate: assertion failed: (pulsesrc->stream == NULL)

trivial testcase attached.
Comment 1 Olivier Crête 2012-08-08 01:16:04 UTC
Created attachment 220622 [details] [review]
pulsesrc: Release ringbuffer before creating new stream

Alright, here is a possible fix.

Otherwise the new stream is destroyed by setcaps in GstBaseAudioSrc
as it tries to release the ringbuffer and re-create it. This is a workaround
for the fact that opening a new stream can affect caps negotiation and our base
classes aren't design for that.
Comment 2 Olivier Crête 2012-08-08 01:17:06 UTC
Someone who understand these things please review. It may make sense to re-work the base classes to take into account that preparing the ringbuffer (ie creating the stream) can affect the caps.
Comment 3 Sjoerd Simons 2012-08-08 08:16:54 UTC
This makes the assertion go away, but the testcase now throws a lot of warnings:
  0:00:01.251020532  5433      0x1faead0 WARN                audiosrc gstaudiosrc.c:246:audioringbuffer_thread_func:<pulsesrc0> error reading data -1 (reason: Success), skipping segment
Comment 4 Tim-Philipp Müller 2012-08-08 18:45:18 UTC
Marking as blocker, since it blocks empathy port to 1.0 (the breakage, not sure if actual renegotation is needed for it, disallowing it would probably work too).
Comment 5 Sjoerd Simons 2012-08-12 09:56:56 UTC
Created attachment 220936 [details] [review]
Workaround

This patch works around the issue filed in this bug by simply ignore negotiation requests.
Comment 6 Arun Raghavan 2012-08-14 11:47:13 UTC
Created attachment 221129 [details] [review]
This makes renegotiation work. Tested with the attached case -- you'll get a

warning message when the stream is killed, but a new stream is set up and
seems to work fine. Not sure if we can make a testcase of this (needs a
running PulseAudio server), but doing so would be nice.

The patch sits on a few other minor fixes that I've got, pending review at:
http://cgit.collabora.com/git/user/arun/gst-plugins-good.git/log/?h=pulse

----

pulsesrc: Handle negotiation events

This makes sure that we:

a) Destroy an existing stream if a negotiate() request comes in: this is
required when receiving a downstream renegotiation request after a
stream has been created.

b) Create a new stream on prepare(): this is required since we do a
setcaps() in negotiate(), which causes the stream to be dropped by a
ringbuffer release() call (this does not happen during first negotiation
since the release is only done on a running ringbuffer). The subsequent
call to ringbuffer acquire() fails because the stream was lost on
release().
Comment 7 Wim Taymans 2012-08-22 09:43:01 UTC
Pushed Arun's patches.
Comment 8 Sjoerd Simons 2012-08-27 07:03:09 UTC
These cause pulsesrc_is_dead to trigger from gst_pulsesrc_reset, which makes things unhappy. From the attached testcase:


0:00:02.354882169  9972      0x193eb70 DEBUG                  pulse pulsesrc.c:1511:gst_pulsesrc_reset:<pulsesrc0> reset
0:00:02.355253312  9972      0x193eb70 WARN                   pulse pulsesrc.c:382:gst_pulsesrc_is_dead:<pulsesrc0> error: Disconnected: Bad state
0:00:02.355337535  9972      0x193eb70 INFO        GST_ERROR_SYSTEM gstelement.c:1802:gst_element_message_full:<pulsesrc0> posting message: Disconnected: Bad state
0:00:02.355410387  9972      0x193eb70 INFO        GST_ERROR_SYSTEM gstelement.c:1825:gst_element_message_full:<pulsesrc0> posted error message: Disconnected: Bad state


At a guess _reset is called during renegotiation and is unhappy with the stream state..
Comment 9 Wim Taymans 2012-09-10 11:37:38 UTC
commit c47c410e7b86075cc48fbe2470f953027bcd70b1
Author: Wim Taymans <wim.taymans@collabora.co.uk>
Date:   Mon Sep 10 13:35:15 2012 +0200

    pulsesrc: consider stream alive when not connected yet
    
    When we start and renegotiate, there is a moment where the stream is created but
    not yet connected. Make sure all functions deal with this situation correctly
    instead of erroring out.
    
    Fixes https://bugzilla.gnome.org/show_bug.cgi?id=681247