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 330996 - basesrc emits EOS unconditionally when going to READY
basesrc emits EOS unconditionally when going to READY
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
Other Linux
: Normal normal
: 0.10.4
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Reported: 2006-02-13 14:14 UTC by Andy Wingo
Modified: 2006-02-24 11:12 UTC
See Also:
GNOME target: ---
GNOME version: ---

proposed fix (2.92 KB, patch)
2006-02-13 15:35 UTC, Tim-Philipp Müller
needs-work Details | Review
new patch (4.45 KB, patch)
2006-02-13 18:17 UTC, Tim-Philipp Müller
committed Details | Review

Description Andy Wingo 2006-02-13 14:14:14 UTC
Currently basesrc emits an EOS event unconditionally when going PAUSED->READY. This occurs even if an EOS was already sent. This is a bug and should be fixed.

Reverting will show the problem in the test suite. The bug was introduced on The bug was not previously noticed because event probes used to not be fired if the pad was flushing, but now they are.
Comment 1 Tim-Philipp Müller 2006-02-13 15:35:33 UTC
Created attachment 59267 [details] [review]
proposed fix

Proposed patch attached (felt it was time to add a private struct ...)
Comment 2 Andy Wingo 2006-02-13 15:55:01 UTC
Should add a pointer to the priv data from the public struct, as in and in GstBus.
Comment 3 Tim-Philipp Müller 2006-02-13 18:17:30 UTC
Created attachment 59278 [details] [review]
new patch

New patch. Note in particular the additions to gst_base_src_activate_pull() and gst_base_src_activate_push(), which are there to avoid sending the event when last operating pullrange()-based. (on a side note, basesrc->pad_mode seems unused)
Comment 4 Andy Wingo 2006-02-14 11:07:22 UTC
Perhaps the correct fix is to set basesrc->pad_mode appropriately. Would make things a bit more maintainable.
Comment 5 Tim-Philipp Müller 2006-02-14 15:08:55 UTC
I am not sure whether setting basesrc->pad_mode would help, as it would be set to NONE when the pad gets deactivated, so in the state change function we still wouldn't know what the value before NONE was. Sending the EOS in the pad de-activation function also doesn't appear to be an option, as seems perfectly legitimate to me to change the mode of operation while running, no? (elements downstream might be unplugged/replaced etc.)
Comment 6 Wim Taymans 2006-02-15 10:40:51 UTC
Sending EOS when going from READY->PAUSED is pretty useless since the downstream element will already be in READY and the event will thus be pushed onto a flushing pad. 

Apparently sound-juicer sets the source to READY _first_ so that the EOS event is able to travel downstream and the elements can flush and close properly... I have a feeling a more elegant way of handling this use case exists...

Comment 7 Tim-Philipp Müller 2006-02-15 10:55:13 UTC
Originally I simply wanted to add a gst_base_src_stop() or _send_eos() (or use a signal, but that's a bit hacky) ...

Comment 8 Tim-Philipp Müller 2006-02-23 11:02:54 UTC

Given that this 'sends-EOS-on-setting-to-READY' behaviour went into the 0.10.3 release we probably need to keep it in future 0.10.x releases, no? If so, is there really an alternative to the patch I proposed, even if it's admittedly not a nice solution code-wise?

(btw, it's gnome-sound-recorder, not sound-juicer)
Comment 9 Wim Taymans 2006-02-23 11:13:22 UTC
patch stays in. Keeping bug open because we must not forget to document how to shut down this type of (live) pipelines.
Comment 10 Tim-Philipp Müller 2006-02-23 18:08:11 UTC
Patch committed, gstutils test case reverted, and new tests for GstBaseSrc EOS behaviour added:

2006-02-23  Tim-Philipp Müller  <tim at centricular dot net>

        * tests/check/
        * tests/check/libs/basesrc.c: (eos_event_counter),
        (basesrc_eos_events_pull), (basesrc_eos_events_push),
        (basesrc_eos_events_push_live_op), (basesrc_eos_events_pull_live_op),
        (gst_basesrc_suite), (main):
          ... and add some tests for the base source EOS stuff.

2006-02-23  Tim-Philipp Müller  <tim at centricular dot net>

        * tests/check/gst/gstutils.c: (test_buffer_probe_n_times):
          Test case originally showed the problem fixed below,
          but was then amended. Add checks back at the place
          where they used to be.

2006-02-23  Tim-Philipp Müller  <tim at centricular dot net>

        * libs/gst/base/gstbasesrc.c: (gst_base_src_class_init),
        (gst_base_src_init), (gst_base_src_loop),
        (gst_base_src_activate_push), (gst_base_src_activate_pull),
        * libs/gst/base/gstbasesrc.h:
          Don't unconditionally send EOS when going from PAUSED to
          READY state, esp. make sure we don't send two EOS events
          in some cases (e.g. one when reaching EOS and one when
          going from PAUSED to READY). Also, we don't want to send
          EOS events when operating in pull mode. However, we do
          want to send an EOS event when shutting down a live
          source explicitly, for example (fixes #330996).
Comment 11 Tim-Philipp Müller 2006-02-24 11:12:20 UTC
Added some docs, closing.

2006-02-24  Tim-Philipp Müller  <tim at centricular dot net>

        * libs/gst/base/gstbasesrc.c:
          Document how applications can stop recording from
          live sources (see #330996).