While testing the [implementation for insertable streams] in `webrtcsink` &
`webrtcsrc`, I encountered critical warnings, which turned out to result from
two race conditions in `rtpsession`. Both race conditions produce:
> GLib-CRITICAL: g_hash_table_foreach:
> assertion 'version == hash_table->version' failed
This commit fixes one of the race conditions observed.
In its simplest form, the test consists in 2 pipelines and a Signalling server:
* pipelines_sink: audiotestsrc ! webrtcsink
* pipelines_src: webrtcsrc ! appsrc
1. Set `pipelines_sink` to `Playing`.
2. The Signalling server delivers the `producer_id`.
3. Initialize `pipelines_src` to establish a session with `producer_id`.
4. Set `pipelines_src` to `Playing`.
5. Wait for a buffer to be received by the `appsrc`.
6. Set `pipelines_src` to `Null`.
7. Set `pipelines_sink` to `Null`.
The race condition happens in the following sequence:
* `webrtcsink` runs a task to periodically retrieve statistics from `webrtcbin`.
This transitively ends up executing `rtp_session_create_stats`.
* `pipelines_sink` is set to `Null`.
* In `Paused` to `Ready`, `gst_rtp_session_change_state()` calls
`rtp_session_reset()`.
* The assertion failure occurs when `rtp_session_reset` is called while
`rtp_session_create_stats` is executing.
This is because `rtp_session_create_stats` acquires the lock on `session` prior
to calling `g_hash_table_foreach`, but `rtp_session_reset` doesn't acquire the
lock before calling `g_hash_table_remove_all`.
Acquiring the lock in `rtp_session_reset` fixes the issue.
[implementing insertable streams support]: https://gitlab.freedesktop.org/gstreamer/gst-plugins-rs/-/merge_requests/1176
Part-of: <https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/4528>
This reverts commit f29c19be58. If this is
called for the reference context then we would run into an infinite
loop, which is not really better than an assertion.
By fixing up DTS to never be ahead of the PTS in the previous commit
this situation should be impossible to hit now.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/4498>
Fix the following use:
- upstream sends a video with a rotation tag, say 90°
- upstream switches to another video without rotation
- the second video was still rotated by videoflip
Fix this by resetting the orientation when receiving STREAM_START.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/4377>
The previous code would only check if two packets in a row were duplicates. If
not (i.e. a packet is a duplicate of a packet received slightly before) the code
would generate completely bogus FCI because it assumes there were no duplicates
present in the array.
In order to be efficient, just store all received packets and remove the
duplicates just before the FCI is generated once the array of observations have
been sorted by seqnum.
Fixes TWCC usage with moderate to high packet duplication.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/4328>
With GST_SEEK_FLAG_SNAP_AFTER present, the previous version would
adjust seek time based on the keyframe farthest away from desired_time.
This was incorrect, because we always want the *earliest* suitable keyframe
to seek to, not the last one.
With this fix, in case of the SNAP_AFTER, we now look for the closest keyframe
that can be found after desired_time. Behaviour for SNAP_BEFORE should remain
unchanged.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/4183>
The flowcombiner and active_streams shouldn't be cleared in the
mse-bytestream variant, only in the mss-fragmented one. Otherwise the
soft reset leaves qtdemux in a state where it still believes that it has
streams, but they've been cleared. In that case, a null pointer
dereference happens and the app crashes.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/4199>
The abort() method of SourceBuffer in Media Source Extensions is
expected to flush the demuxer and discard the current fragment,
if any. The configuration of tracks, if any, should be preserved.
qtdemux has different behavior for flush events depending on the
context.
This patch activates the intended behaviour only for streams of the
VARIANT_MSE_BYTESTREAM type, conformant to the ISO BMFF Bytestream
specification[1]. This flush behaviour is the same as the one
already in use for adaptivedemux sources.
[1] https://www.w3.org/TR/mse-byte-stream-format-isobmff/https://bugzilla.gnome.org/show_bug.cgi?id=795424
Part-of: <https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/4101>
This patch prevents a possible race condition from taking place between the EOS event handling and rtcp send
function/thread.
The condition starts by getting the GST_EVENT_EOS event on the send_rtp_sink pad, which causes two core things
to happen -- the event gets pushed down to the send_rtp_src pad and all sessions get marked "bye" prior to
completion of the event handler. In another thread the rtp_session_on_timeout function gets called after an
expiration of gst_clock_id_wait in the rtcp_thread function. This results in a call to the
ess->callbacks.send_rtcp(), which is configured as a function pointer to gst_rtp_session_send_rtcp via the
RTPSessionCallbacks structure passed to rtp_session_set_callbacks in the gst_rtp_session_init function.
In the race condition, the call to gst_rtp_session_send_rtcp can have the all_sources_bye boolean set to true
while GST_PAD_IS_EOS(rtpsession->send_rtp_sink) evaluates to false. This is the result of gst_rtp_session_send_rtcp
running before the send_rtp_sink's GST_EVENT_EOS handler completes. The exact point at which this condition occurs
is if there's a context switch to the rtcp_thread right after the call to rtp_session_mark_all_bye in the
GET_EVENT_EOS handler, but before the handler returns.
Normally, this would not be an issue because the rtcp_thread continues to run and indirectly call
gst_rtp_session_send_rtcp. However, the call to rtp_source_reset sets the sent_bye boolean to false, which ends up
causing rtp_session_are_all_sources_bye to return false. This gets passed to gst_rtp_session_send_rtcp and the EOS
event never gets sent.
The race condition results in the EOS event never getting passed to the rtcp_src pad, which prevents the bin and
pipeline from ever completing with EOS.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/3798>
A data offset with an offset smaller than the moof length is wrong
in smooth streaming streams.
The samples will not be located and eventually playback will
error out. So compensate assuming data is in mdat following moof.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/3840>
The av1C box is optional so dropping parsing does not break anything
fundamentally, and there seems to be no historical record how version 0
even looks like while the comments and the parsing disagreed with each
other.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/3882>
When using qtdemux in a pipeline that should only work as a pure demuxer (not
for actual playback), qtdemux shouldn't emit new GstSegments to correct
the start time (jump to the future) to ensure that the user experiences no
playback delay. By doing so, it's generating the wrong segments when an append
of data from the past happens. When that happens, downstream elements such as
parsers (eg: aacparse) may clip those buffers laying before the GstSegment and
create problems on the GStreamer client app (eg: WebKit).
Getting buffers clipped out because of the wrong GstSegments started becoming
a problen when this commit was introduced:
ab6e49e9cc audioparsers: add back segment clipping to parsers that have lost it
This clipping makes test DASH shaka 35 from MVT tests[1] to fail in
WebKitGTK/WPE (at least) and can potentially cause a number of other problems
in the WebKit Media Source Extensions (MSE) code.
Note that this new behaviour of not emitting new GstSegments only makes sense
when qtdemux is being used as a pure demuxer and not as part of a regular
pipeline. This is why the variant field has been added. When equal to
VARIANT_MSE_BYTESTREAM, it will make qtdemux behave differently in push mode,
taking decisions that meet the expectations for an MSE-like processing mode.
This kind of tweaks have been done in the past for MSS streams, for instance.
That code has been refactored to use VARIANT_MSS_FRAGMENTED now, instead of
its own dedicated boolean flag.
Co-authored by: Alicia Boya García <ntrrgc@gmail.com>
...who suggested to use "variant: mse-bytestream" in the caps to identify that
mode, as proposed in her unmerged patch:
https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/issues/467
[1] https://github.com/rdkcentral/mvt
Part-of: <https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/3867>
All the RTP src pads were sharing the same stream-id while each actually
carry a different stream.
This was causing problem for example when funneling the streams together
and then trying to split them using 'streamiddemux'.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/3855>
In theory, `dispose()` functions should be idempotent and should be
prepared not to crash or cause a double-free if an unref done from
inside caused a recursive call to `dispose()` of the same object.
https://developer.gnome.org/gobject/stable/howto-gobject-destruction.html
This patch modifies the `dispose()` method to honor these constraints.
Since the double `dispose()` call won't actually occur in qtdemux (there
is no cycle detection mechanism that could invoke it to work that way),
this is more of a code cleanup than a user-facing problem fix.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/3822>
