GNOME Bugzilla – Bug 757563
audiomixer: issues with seamless looping (missing support for non-flushing seeks)
Last modified: 2018-11-03 11:43:27 UTC
Despite a test that should cover this in real world apps, audiomixer (aggregator) goes crazy when looping (gst_aggregator_do_seek): I can play once with a segmented+flushing seek. When handling segment_done and sending another segmented seek (non flushing), I get this log forever, until I stop the pipeline. 0:00:20.315003477 13555 0x7f8f84005e30 INFO bt-core src/lib/core/song.c:512:on_song_segment_done: received SEGMENT_DONE (114) bus message: 0x7f8f60005e20, from song, with fmt=time, ts=0:00:07.680000000 0:00:20.315071362 13555 0x7f8f84005e30 INFO aggregator gstaggregator.c:1671:gst_aggregator_do_seek:<master:audiomixer> starting SEEK: seek event: 0x4505430, time 99:99:99.999999999, seq-num 1473, GstEventSeek, rate=(double)1, format=(GstFormat)GST_FORMAT_TIME, flags=(GstSeekFlags)GST_SEEK_FLAG_SEGMENT, cur-type=(GstSeekType)GST_SEEK_TYPE_SET, cur=(gint64)0, stop-type=(GstSeekType)GST_SEEK_TYPE_SET, stop=(gint64)7680000000; 0:00:20.315172122 13555 0x7f8f84005e30 INFO aggregator gstaggregator.c:1700:gst_aggregator_do_seek:<master:audiomixer> seek done 0:00:20.315196595 13555 0x7f8f84005e30 INFO bt-core src/lib/core/song.c:532:on_song_segment_done: -> loop (1473) 0:00:20.421714132 13555 0x7f8f84005e30 INFO bt-core src/lib/core/song.c:1076:bt_song_update_playback_position: query playback-pos: cur=7656970000, tick=63 0:00:20.435006447 13555 0x7f8f84005e30 INFO bt-core src/lib/core/song.c:512:on_song_segment_done: received SEGMENT_DONE (1473) bus message: 0x44fb1c0, from song, with fmt=time, ts=0:00:07.680000000 0:00:20.435093709 13555 0x7f8f84005e30 INFO aggregator gstaggregator.c:1671:gst_aggregator_do_seek:<master:audiomixer> starting SEEK: seek event: 0x4505740, time 99:99:99.999999999, seq-num 1651, GstEventSeek, rate=(double)1, format=(GstFormat)GST_FORMAT_TIME, flags=(GstSeekFlags)GST_SEEK_FLAG_SEGMENT, cur-type=(GstSeekType)GST_SEEK_TYPE_SET, cur=(gint64)0, stop-type=(GstSeekType)GST_SEEK_TYPE_SET, stop=(gint64)7680000000; 0:00:20.435201716 13555 0x7f8f84005e30 INFO aggregator gstaggregator.c:1700:gst_aggregator_do_seek:<master:audiomixer> seek done 0:00:20.435227403 13555 0x7f8f84005e30 INFO bt-core src/lib/core/song.c:532:on_song_segment_done: -> loop (1651)
The handling of non-flushing seeks looked a bit weird to me, so not exactly surprised that this doesn't work great yet :) Needs some work Do flushing seeks work well for you though?
With flushing seeks it works, but then the loops are not smooth. TBH, I don't see where not flushing sees are *handled*.
I think they are *not* handled currently :)
Created attachment 355775 [details] [review] audioaggregator: fix non flushing segmented seeks This makes it work for audio. There are still a few things to fix (partial buffer, when the seek comes in). With this change the tests pass and this starts to work. I'd welcome someone taking a look so that this is not breaking videomixer. If it looks good, I'll continue with fleshing out the audio details in another patch with associated tests.
Created attachment 356590 [details] [review] aggregator: WIP implementation for non flushing seek segment handling I am stuck with this change. I added another test and that fails as expected. The intention of the change on aggregator is to provide another vmethod to drain a pending buffer to close a segment. Aggregator would wait to get segments from all sink-pads, then flags that a new segment should be send down stream and signals implementations via vmethod to drain. The change is not too complex, but I am totally stuck with the locking. All tests timeout. I really appreciate a 2nd pair of eyes.
Created attachment 356733 [details] [review] aggregator: WIP implementation for non flushing seek segment handling Nevermind, I was just too tired.
Created attachment 362753 [details] [review] implementation for non flushing seek segment handling This is my current WIP patch. It works for buzztrax and passes the tests (incl gst-validate ges). There is one major things to discuss (see gst_aggregator_do_events_and_queries()). I need to handle all SEGMENT_DONE events before proceeding into the pad-queues. If I would handle SEGMENT_DONE and then the next queued SEGMENT event. I would toggle the flag on the pad and never notice that I got all SEGMENT-DONE events. We might need to do something based on sequence numbers here, but it sounds expensive.
Review of attachment 362753 [details] [review]: Just gave a quick look, I'll try to give a more in-depth asap, at least on the aggregator side :) ::: gst-libs/gst/base/gstaggregator.c @@ +1567,3 @@ + + /* send whatever is in the mix buffer */ + gst_aggregator_drain (self); This should be done from the aggregate (srcpad) thread, not from a random sinkpad's streaming thread :) @@ +2140,3 @@ + priv->segmented_seeking = segmented; + /* we apply the segment, once we have segment from all sources */ + /* FIXME: do we need a queue of seek-events? */ If I understood what you told me at the conf correctly, yes ?
(In reply to Mathieu Duponchelle from comment #8) > Review of attachment 362753 [details] [review] [review]: > > Just gave a quick look, I'll try to give a more in-depth asap, at least on > the aggregator side :) > > ::: gst-libs/gst/base/gstaggregator.c > @@ +1567,3 @@ > + > + /* send whatever is in the mix buffer */ > + gst_aggregator_drain (self); > > This should be done from the aggregate (srcpad) thread, not from a random > sinkpad's streaming thread :) I know. That's why it is done in gst_aggregator_default_sink_event(), which is called from gst_aggregator_do_events_and_queries(), which is called from gst_aggregator_aggregate_func() and thats the src-pad task. > > @@ +2140,3 @@ > + priv->segmented_seeking = segmented; > + /* we apply the segment, once we have segment from all sources */ > + /* FIXME: do we need a queue of seek-events? */ > > If I understood what you told me at the conf correctly, yes ? That's why there is a FIXME: - I am still pondering whats the best way to do this. We'll basically need a GQueue for sequence numbers (guint32) on the aggregator and on each sink-pad. 1) we get a seek on src, if segmented push seq_num to aggregator queue and each sink-pad queue 2) we get a segment on a sink-pad 2.1) check that ev.seq_num == queue.head.seq_num 2.2) pop the seq_num off the pad_queue 2.3) if none of the sink-pad queues has the aggregator-queue.head.seq_num at head, do the seek and pop the seq_num from the aggregator queue, the seek is done This sounds somewhat involved and not cheap.
(In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #9) > I know. That's why it is done in > gst_aggregator_default_sink_event(), which is called from > gst_aggregator_do_events_and_queries(), which is called from > gst_aggregator_aggregate_func() and thats the src-pad task. > Right, this function is called from the srcpad thread with serialized events, sorry about that (I did say I only gave a quick look :) > That's why there is a FIXME: - I am still pondering whats the best way to do > this. We'll basically need a GQueue for sequence numbers (guint32) on the > aggregator and on each sink-pad. > 1) we get a seek on src, if segmented push seq_num to aggregator queue and > each sink-pad queue > 2) we get a segment on a sink-pad > 2.1) check that ev.seq_num == queue.head.seq_num > 2.2) pop the seq_num off the pad_queue > 2.3) if none of the sink-pad queues has the aggregator-queue.head.seq_num > at head, do the seek and pop the seq_num from the aggregator queue, the seek > is done > > This sounds somewhat involved and not cheap. Think you could write some unit testing? Might make it easier to figure out a good solution for that and also the other FIXME (iteration stopped in do_events_and_queries)
(In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #9) > That's why there is a FIXME: - I am still pondering whats the best way to do > this. We'll basically need a GQueue for sequence numbers (guint32) on the > aggregator and on each sink-pad. > 1) we get a seek on src, if segmented push seq_num to aggregator queue and > each sink-pad queue > 2) we get a segment on a sink-pad > 2.1) check that ev.seq_num == queue.head.seq_num > 2.2) pop the seq_num off the pad_queue > 2.3) if none of the sink-pad queues has the aggregator-queue.head.seq_num > at head, do the seek and pop the seq_num from the aggregator queue, the seek > is done > Just thought about this a bit more, would it not be enough to have a single queue in the aggregator, containing a struct {guint32 seqnum; guint n_pads}, do atomic operations on n_pads and do the seek when dec_and_test succeeds?
Stefan ? :)
Maybe we can do a step by step approach. There are tons of open questions and with these changes it is at least not completely broken. I can try to find some time over Christmas to work further on this. But before this works, the element shouldn't be moved.
A good first step might be to split out the aggregator patches and the audioaggregator patches, now that aggregator is in core :) Can we list what scenarios we want to support, and how audiomixer is supposed to behave in those scenarios? For example, the discussion above sounds like the assumption is that there will be a seek event with a segment seek request travelling upstream through audiomixer and then all inputs will have the same segment? What about the case where different inputs have different segments? Or where the seek does not go through audiomixer, but e.g. an upstream element simply loops independently on one input, while the other input is continuous? (e.g. some automated announcement loop with another input that's live from an audio source or rtp or so)
Tim, what about reading the changes. The baseclass change is needed to make it work at all, right now when doing a seamless loop, the baseclass is not able to send the last clipped buffer. Now since you already moved it all my changes are ruined. I guess all I can do is to fork the element and ship it myself. I am happy to discuss this in ITC, but I'd say you rather revert the move or help.
I'll try to redo the patches. On the discussion side. I am all for having a 100% features seeking behaviour, but please understand that it the implementation will be complicated and hence I advocate to take steps. Apparently we'Re happy shipping something that does not work at all (wrt to non flushing seeks). So if we can improve this for the 90% of the cases, I don't see why we would hold this back. Right now basesrc won't be able to handle more that one see anyway.
Not sure what the problem with the patches was or why the move "ruined" anything - you can export them with git format-patch, and fix up the file paths to match the new file location in core. If it applied in bad it will apply fine in core.
The attached patches are obsolete, I am starting separate changes now.
Created attachment 366010 [details] [review] implementation for non flushing seek segment handling
Did you propose the drain vmethod in a separate issue? Can't seem to find it :)
Review of attachment 366010 [details] [review]: I'm ok with merging this patch. What happens in the case where the segment.stop is exactly on the frame boundary, doesnt it get into the FIXME case that you're documenting?
Drain vmethod in https://bugzilla.gnome.org/show_bug.cgi?id=791986. I'll comment on the comment #21 in a bit.
It sounds to me like we should get this in (and the other patches in the other bugs), and then we can fix up corner cases or anything else later?
Review of attachment 366010 [details] [review]: Some comments, I also wonder the same thing as Olivier. Second part of the commit message should also be edited out :) ::: gst-libs/gst/audio/gstaudioaggregator.c @@ +1732,2 @@ /* Update position from the segment start/stop if needed */ + if (agg->segment.position == -1 || agg->segment.position < agg->segment.start) { How can this happen? @@ +1999,3 @@ + /* when using live sources, we might get output_offset to be offset + 1 + * due to rounding errors (?), clip the max_offset to the segment + */ That seems unrelated, and I'm not sure I understand how this can happen, I think it'd better be discussed in a separate issue. @@ +2049,3 @@ /* send it out */ GST_LOG_OBJECT (aagg, + "pushing outbuf %p, timestamp %" GST_TIME_FORMAT ", offset %" Could be simplified by just using GST_PTR_FORMAT + the offset instead
I'll upload a new patch in a bit (recovered from the flu).
(In reply to Mathieu Duponchelle from comment #24) > Review of attachment 366010 [details] [review] [review]: > > Some comments, I also wonder the same thing as Olivier. Second part of the > commit message should also be edited out :) > > ::: gst-libs/gst/audio/gstaudioaggregator.c > @@ +1732,2 @@ > /* Update position from the segment start/stop if needed */ > + if (agg->segment.position == -1 || agg->segment.position < > agg->segment.start) { > > How can this happen? This is the same check as here: https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst-libs/gst/audio/gstaudioaggregator.c#n436 I don't have a repro right now, but I am sure I run into it since I added the condition there. > > @@ +1999,3 @@ > + /* when using live sources, we might get output_offset to be offset + > 1 > + * due to rounding errors (?), clip the max_offset to the segment > + */ > > That seems unrelated, and I'm not sure I understand how this can happen, I > think it'd better be discussed in a separate issue. I've extracted the change and will try to come up with a separate repro. > > @@ +2049,3 @@ > /* send it out */ > GST_LOG_OBJECT (aagg, > + "pushing outbuf %p, timestamp %" GST_TIME_FORMAT ", offset %" > > Could be simplified by just using GST_PTR_FORMAT + the offset instead Done.
Created attachment 367774 [details] [review] implementation for non flushing seek segment handling
WRT comment 21: A good question for the aggregator design! What do we do in this case? _aggregate() is called and supposed to fill the buffer, but it can't put anything into it due to end-of-segment. I can file a new bug for this if you agree to discuss it.
(In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #26) > (In reply to Mathieu Duponchelle from comment #24) > > Review of attachment 366010 [details] [review] [review] [review]: > > > > Some comments, I also wonder the same thing as Olivier. Second part of the > > commit message should also be edited out :) > > > > ::: gst-libs/gst/audio/gstaudioaggregator.c > > @@ +1732,2 @@ > > /* Update position from the segment start/stop if needed */ > > + if (agg->segment.position == -1 || agg->segment.position < > > agg->segment.start) { > > > > How can this happen? > > This is the same check as here: > https://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/gst-libs/gst/ > audio/gstaudioaggregator.c#n436 > I don't have a repro right now, but I am sure I run into it since I added > the condition there. OK, this is still unrelated however, please extract it as well :) > > > > @@ +1999,3 @@ > > + /* when using live sources, we might get output_offset to be offset + > > 1 > > + * due to rounding errors (?), clip the max_offset to the segment > > + */ > > > > That seems unrelated, and I'm not sure I understand how this can happen, I > > think it'd better be discussed in a separate issue. > > I've extracted the change and will try to come up with a separate repro. > > > > > @@ +2049,3 @@ > > /* send it out */ > > GST_LOG_OBJECT (aagg, > > + "pushing outbuf %p, timestamp %" GST_TIME_FORMAT ", offset %" > > > > Could be simplified by just using GST_PTR_FORMAT + the offset instead > > Done.
(In reply to Stefan Sauer (gstreamer, gtkdoc dev) from comment #28) > WRT comment 21: A good question for the aggregator design! What do we do in > this case? _aggregate() is called and supposed to fill the buffer, but it > can't put anything into it due to end-of-segment. > > I can file a new bug for this if you agree to discuss it. You should be able to just return NULL here, this is correctly handled by aggregator.
Disregard the last comment, looking at the diff I mistakenly thought we were in do_clip
->aggregate is not expected to fill the output buffer, returning GST_FLOW_OK should be just fine, or am I missing something?
Review of attachment 367774 [details] [review]: ::: gst-libs/gst/audio/gstaudioaggregator.c @@ +1992,2 @@ max_offset = MAX ((gint64) max_offset, (gint64) pad->priv->output_offset); + useless new line @@ +2042,2 @@ aagg->priv->current_buffer = NULL; + ret = gst_aggregator_finish_buffer (agg, outbuf); This seems unrelated too, any reason for doing that?
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/240.