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 702282 - basesink: makes element go to PLAYING without PAUSED_TO_PLAYING transition
basesink: makes element go to PLAYING without PAUSED_TO_PLAYING transition
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
git master
Other Linux
: Normal blocker
: 1.0.9
Assigned To: GStreamer Maintainers
GStreamer Maintainers
: 696675 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2013-06-14 18:38 UTC by Youness Alaoui
Modified: 2013-07-24 09:37 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Test code to reproduce basesink bug (1.37 KB, application/octet-stream)
2013-06-14 18:38 UTC, Youness Alaoui
Details

Description Youness Alaoui 2013-06-14 18:38:12 UTC
Created attachment 246846 [details]
Test code to reproduce basesink bug

It looks like basesink is somehow making the element go to PLAYING state but it doesn't go through the PAUSED state.. or at least, it doesn't get the PAUSED_TO_PLAYING transition which makes pulsesink not work.

In my code, using autoaudiosink, it's working (uses pulsesink internally), but if I replace it with pulsesink directly, it stops working... 
The difference I noticed in the log is that with autoaudiosink, it says "commiting state to PAUSED" then the whole preroll, etc.. while with pulsesink it directly says "commiting state to PLAYING".

The actual issue with the sound is that it doesn't do the latency query, so when it does a get_calibration on PulseSinkClock, it gets 'cexternal = 0' which makes it wait days before rendering the first buffer, while the one with autoaudiosink does do the latency query, and sets calibration on the clock (in sync_latency) which makes audiobasesink report the right buffer timestamps because PulseSinkClock doesn't return a wrong external calibration value

in audiobasesink, on the state change PAUSED_TO_PLAYING, it does this :
      GST_DEBUG_OBJECT (sink, "ringbuffer may start now");
      sink->priv->sync_latency = TRUE;
In my log, when using pulsesink directly, there isn't any "ringbuffer may start now", that's why it doesn't call sync_latency when the first buffer arrives and why the PulseSinkClock is not calibrated and reports wrong values, and I believe that issue is caused by the missing PAUSED_TO_PLAYING transition.
If I force the sink to go to PAUSED before going to PLAYING, then it works.

I wrote a small test code to reproduce this bug, and I've attached it to this bug report.
You will see I set my pipeline to playing, then I add the source and sink to it and set them to PLAYING. When you run it, no sound will play and the g_debug will output that the sink is in the state PLAYING.
If you enable GST_DEBUG=audiobasesink:5, you will notice that the debug message "ringbuffer may start now" does not appear, even though the element went to the PLAYING state.
If you uncomment the line that uses autoaudiosink, it works and on stdout, it will report that the state of the sink is PAUSED, and the "ringbuffer may start now" message does appear.
If you use the alternative code that sets the element to PAUSED first, it will work with pulsesink.
Comment 1 Wim Taymans 2013-06-17 07:58:02 UTC
I know about this issue for some time. It happens when you set a sink to PLAYING, it will got to PAUSED async, when it gets the first buffer it commits to PAUSED and then go to PLAYING. When it goes to PLAYING, it doesn't call the state change function with PAUSED_TO_PLAYING (see gst_base_sink_commit_state())

This is tricky to solve, I have tried some things already but they are all racy:

 1 call state change function. We should ideally call this with the STATE_LOCK
   but that would deadlock easily.
 2 call state change without the state lock. This deadlocks because the PREROLL
   lock is taken in the state change
 3 release PREROLL lock and call state change. It is possible that the two state
   change functions are called at the same time. This could be fixed by taking
   locks in the subclass.

I think I will try 3 some more.
Comment 2 Wim Taymans 2013-06-17 08:37:07 UTC
commit 124b8e38afa5a7e18b01d8f5969b7b20b9854030
Author: Wim Taymans <wim.taymans@collabora.co.uk>
Date:   Mon Jun 17 10:25:20 2013 +0200

    basesink: call state change in all cases
    
    When we asynchronously go from READY to PLAYING, also call the
    state change function so that subclasses can update their state for PLAYING.
    Because the PREROLL lock is not recursive, we can't make this without
    races and we must assume for now that the subclass can handle concurrent calls
    to PAUSED->PLAYING and PLAYING->PAUSED. We can make this assumption because not
    many elements actually do something in those state changes and the ones that
    did would be broken even more without this change.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=702282
Comment 3 Tim-Philipp Müller 2013-07-15 21:34:34 UTC
cherry-picked into 1.0 branch.
Comment 4 Wim Taymans 2013-07-24 09:37:23 UTC
*** Bug 696675 has been marked as a duplicate of this bug. ***