The old code had a couple of issues that all lead to potential memory
safety bugs.
- Use a constant for the Wavpack4Header size instead of using sizeof.
It's written out into the data and not from the struct and who knows
what special alignment/padding requirements some C compilers have.
- gst_buffer_set_size() does not realloc the buffer when setting a
bigger size than allocated, it only allows growing up to the maximum
allocated size. Instead use a GstAdapter to collect all the blocks
and take out everything at once in the end.
- Check that enough data is actually available in the input and
otherwise handle it an error in all cases instead of silently
ignoring it.
Among other things this fixes out of bounds writes because the code
assumed gst_buffer_set_size() can grow the buffer and simply wrote after
the end of the buffer.
Thanks to Natalie Silvanovich for reporting.
Fixes https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/issues/859
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/902>
This usually doesn't matter, but it is disruptive when streaming from
a shared media since it will pause all other clients when any client
exits.
This new behaviour is opt-in and should be safe because you need to
set the NULL state on rtspsrc directly, instead of just on the
pipeline. See the updated documentation for an explanation.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/901>
Some cameras (e.g. HikVision DS-2CD2732F-IS) return "551 Option
not supported" when a command is sent that is not implemented
(e.g. PAUSE). Instead; it should return "501 Not Implemented".
This is wrong, as previously, the camera did announce support for PAUSE
in the OPTIONS.
In this case, handle the 551 as if it was 501 to avoid throwing errors
to application level. */
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/885>
wavparse claims to be able to support seeking in the READY state by
saving the pending seek event and actually seeking later after having parsed the
header.
Problem was that this seek event was reset on the READY to PAUSED
transition, making all this code useless. Fixing it by stop resetting
on READY to PAUSED transition as we already reset on PAUSED to READY
and when initiating the element.
Note that DTS marker detection isn't support in such scenario as
gst_type_find_helper_for_buffer() needs a buffer containing the
beginning of the stream.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/879>
Similar to rtpvp8depay, when packet loss occurs, the depayloader
starts waiting for a keyframe.
We try to only stop waiting when all the packets for the new keyframe
have been received, by only resetting waiting_for_keyframe when
encountering the first packet of a keyframe, this is slightly
fragile because there is no bit that explicitly marks the start
of an access unit, so we rely on the existing picture_start
detection code.
As a consequence, the property is only meaningful when outputting
access units, and is ignored when outputting NALs directly.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/834>
Due to the may_cancel flag in GstRTSPConnection, receiving might not get
cancelled when supposed to. In this case, gst_rtsp_src_receive_response
will have to wait until timeout instead but if busy receiving RTP
data, this timeout will never occur.
With this patch, gst_rtsp_src_receive_response returns GST_RTSP_EINTR
if flushing is set to TRUE instead of continuing to receive.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/831>
These should be with a single ':'. The double '::' results in a CI with
build failure message like below.
ERROR: [links]: (mandatory-link-not-found): Mandatory link Link GstSocketTimestamp -> None (GstSocketTimestamp) could not be resolved
ERROR: [check-missing-since-markers]: (missing-since-marker): Missing since marker for udpsrc:socket-timestamp
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/828>
A classic case of not enough locking.
One interesting thing with this is the interaction between the
rotation value and caps negotiation. i.e. the width/height of the caps
can be swapped depending on the video-direction property. We can't lock
the entirety of the caps negotiation for obvious reasons so we need to
do something else. This takes the approach of trying to use a single
rotation value throughout the entirety of the negotiation and then
subsequent output frame in a kind of latching sequence.
Fixes: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/issues/792
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/836>
While the standard is a bit vague about whether the padding,
extension and marker bits should be protected:
> The usage, by senders and receivers, of the following bits shall
> be defined by the associated video/audio transport standards:
It is obviously necessary and useful for some formats (eg VP8)
that those indeed be protected.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/839>
https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/798
introduced a check in the need-new-fragment logic to avoid starting a
new fragment unless there has been some data on the reference stream,
but the check is done against the number of bytes that have been
received on the input, not the number that were released for output
into the current fragment.
Fix the check to remember and test against bytes that have been sent
for output.
This also fixes a problem where starting a new fragment fails to
request a new filename from the format-location signal.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/833>
This will end up as a "received" packet, due to the code in
source_push_rtp, which will think this is a packet being received.
Instead drop the packet and hope that either:
1. Something upstream responds to the GstRTPCollision event and changes
SSRC used for sending.
2. That the application responds to the "on-ssrc-collision" signal, and
forces the sender (payloader) to change its SSRC.
3. That the BYE sent to the existing user of this SSRC will respond to
the BYE, and that we timeout this source, so we can continue sending
using the chosen SSRC.
The test reproduces a scenario where we previously would have sent
on a non-internal source.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/817>
In baseparse we set the fixed caps flag on all src pads, therefore the
source pad caps query in get_allowed_caps will return the current caps.
Current caps won't necessarily intersect with the new caps (e.g. sample
rate change). Replace get_allowed_caps with peer_query_caps.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/816>
Add a new state for ending the overall stream, and use it to decide
whether to pass the final EOS message up the bus instead of dropping
it. Fixes a small race that makes the testsuite sometimes not generate
the last fragment(s) sometimes because the wrong EOS gets
allowed through too early.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/798>
Using the element state lock to avoid splitmuxsink shutting
down while doing element manipulations can lead to a deadlock on
shutdown if a fragment switch happens at exactly the wrong moment.
Use a private mutex and a shutdown boolean instead.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/798>
If a pad gets into the check_completed_gop method and then
the underlying conditions change on the reference context,
things could get stuck in a busy loop when the context should
instead jump back out and wait for more data.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/798>
Make sure that any late gst_element_call_async() callbacks
know that the elements is shutting down and bail out instead
of operating on the element we're trying to stop.
Fixes a spurious test failure in elements_splitmuxsrc
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/798>
- Fix start and end of picture to support multiple layers. Start of
picture is the first packet of the base layer, while end of picture
is when the marker bit is set (last packet of the enhancement
layers).
- All "layers" (aka "frames") of a picture are pushed downstream in a
single buffer when picture is complete.
- Forgive SID=0 for enhancement layers (invalid, but Chrome and
Firefox sends it)
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/773>
This is ad adaptation of a Pexip patch for dealing with spurious
GstRTPPacketLost events caused by lost ulpfec packets: as FEC packets
under that scheme are spliced in the same sequence domain as the media
packets, it is not generally possible to determine whether a lost packet
was a FEC packet or a media packet.
When upstreaming pexip's ulpfec patches, we decided to drop all lost
events at the base depayloader level, and where the original patch
from pexip was making use of picture ids and marker bits to determine
whether a packet should be forwarded, this patch makes use of those
to determine whether they should be dropped instead (by removing their
might-have-been-fec field).
Spurious lost events coming out of the depayloader can cause the
decoder to stop decoding until the next keyframe and / or request a new
keyframe, and while this is not desirable it makes sense to forward
that information when we have other means to determine whether a lost
packet was indeed a FEC packet, as is the case with VP8 / VP9 payloads
when they carry a picture id.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/769>
The AVC codec_data has a flaw that it can only accomodate
31 SPS headers, even though H.264 can have 32, and 255 PPS,
when there can be 256 in H.264. When streaming RTP some
clients like to cycle through SPS/PPS ids when changing
configuration and can eventually accumulate a full set.
In that case, we have no choice but to discard one (oldest)
entry, or else the count written into the codec_data is wrong
and downstream decoding failures ensue.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/775>
Due to us not properly acknowleding the time when the last RTX was sent
when scheduling a new one, it can easily happen that due to the packet
you are requesting have a PTS that is slightly old (but not too old when
adding the latency of the jitterbuffer), both its calculated second and
third (etc.) timeout could already have passed. This would lead to a burst
of RTX requests, which acts completely against its purpose, potentially
spending a lot more bandwidth than needed.
This has been properly reproduced in the test:
test_rtx_not_bursting_requests
The good news is that slightly re-thinking the logic concerning
re-requesting RTX, made it a lot simpler to understand, and allows us
to remove two members of the RtpTimer which no longer serves any purpose
due to the refactoring. If desirable the whole "delay" concept can actually
be removed completely from the timers, and simply just added to the timeout
by the caller of the API. But that can be a change for a another time.
The only external change (other than the improved behavior around bursting
RTX) is that the "delay" field now stricly represents the delay between
the PTS of the RTX-requested packet and the time it is requested on,
whereas before this calculation was more about the theoretical calculated
delay. This is visible in three other RTX-tests where the delay had
to be adjusted slightly. I am confident however that this change is
correct.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/789>
This action signal will delegate to clear-ssrc onto the rtpssrcdemux element
associated with the session. This allow rtpbin users to clear pads and
elements for a specific ssrc that is known to no longer be in use. This
happens when a pad is reused in rtpsrc or ristsrc.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/736>
Add property to set the initial value for picture-id. RFC7741 says
that picture-id MAY be initialized to a random value, thus it's also
valid to simply set it to a fixed initial value. A fixed value is very
useful for testing.
Default behavior is not changed.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/728>
In order to support the symbol g_enum_to_string in various
project using GStreamer ( gst-validate etc.), the glib minimum
version should be 2.56.0.
Remove compat code as glib requirement
is now > 2.56
Version used by Ubuntu 18.04 LTS
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/774>
Scenario:
- gap event causes h264parse to push made up caps that may fail checks
inside qtmux (e.g missing codec_data).
- the caps event has already been marked as received and is sticky on
the sink pad
- gst_qt_mux_pad_can_renegotiate() will retrieve the failed caps event
using gst_pad_get_current_caps() and reject the correct updated caps
with codec_data.
- Failure!
Keep track of the configured caps ourselves instead of relying on the
sticky event on the pad.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/732>
Since !348, the jitterbuffer was only removed with the session. This restores
the original behaviour and removes the jitterbuffer when the stream is
removed. This avoid accumulating jitterbuffer objects into the bin when a
session is reused.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/735>
The rtpjitterbuffer is now part of the session elements, we no longer need
to do the ref_sink dance when signalling it. It is already owned by the bin
when signalled. Also, the code that handles generic session elements already
handle the ref_sink() calls since:
03dc22951b
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/735>
If we have not received a FU with a start bit set, any subsequent FU
data is not useful at all and would result in an invalid stream.
This case is constructed from multiple requirements in
RFC 3984 Section 5.8 and RFC 7798 Section 4.4.3. Following are excerpts
from RFC 3984 but RFC 7798 contains similar language.
The FU in a single FU case is forbidden:
A fragmented NAL unit MUST NOT be transmitted in one FU; i.e., the
Start bit and End bit MUST NOT both be set to one in the same FU
header.
and dropping is possible:
If a fragmentation unit is lost, the receiver SHOULD discard all
following fragmentation units in transmission order corresponding to
the same fragmented NAL unit.
The jump in seqnum case is supported by this from the specification
instead of implementing the forbidden_zero_bit mangling:
If a fragmentation unit is lost, the receiver SHOULD discard all
following fragmentation units in transmission order corresponding to
the same fragmented NAL unit.
A receiver in an endpoint or in a MANE MAY aggregate the first n-1
fragments of a NAL unit to an (incomplete) NAL unit, even if fragment
n of that NAL unit is not received. In this case, the
forbidden_zero_bit of the NAL unit MUST be set to one to indicate a
syntax violation.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/730>
Used by some proprietary software for their fragmented files.
Adds some support for multi-stream fragmented files
Flow is as follows.
1. The first 'fragment' is written as a self-contained fragmented
mdat+moov complete with an edit list and durations, tags, etc.
2. Subsequent fragments are written with a mdat+moof and each stream is
interleaved as data arrives (currently ignoring the interleave-*
properties). data-offsets in both the traf and the trun ensure
data is read from the correct place on demuxing. Data/chunk offsets
are also kept for writing out the final moov.
3. On finalisation, the initial moov is invalidated to a hoov and the
size of the first mdat is extended to cover the entire file contents.
Then a moov is written as regularly would in moov-at-end mode (the
default).
This results in a file that is playable throughout while leaving a
finalised file on completion for players that do not understand
fragmented mp4.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/643>
When we are fragmented, the edit list may only refer to the portion of
the media that is in the moov. Extend the edit list stop time when we
if there is only one qt segment and we are reading a fragmented file.
Fixes playback of some fragmented mp4 files generated by proprietary
programs.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/643>
This is modelled after the DASH Common Encryption scheme, but is somewhat
simpler as more parts are fixed, i.e. just one encryption scheme.
The output caps are fixed to 'application/x-aavd'. All information
required for decryption are part of the 'adrm' atom, which is passed
on as a property. The property is attached to the buffer.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/577>
The 'aavd' box is contained in the 'stsd' sample description. The 'aavd'
box follows the layout of an 'mp4a' entry, i.e. it contains a single
standard 'esds' extension box, and the two proprietary 'adrm' and 'aabd'
extension boxes.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/577>
- Release the split mux lock while removing the probes
- Flush the sinkpad to unblock other pads
- Turn check_completed_gop into a do while statement, when
waking up we want to recheck whether the current GOP is
ready for sending
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/719>
The code seems to use `continue` and `break` as if both refer to the
surrounding `while` loop. But because `break` breaks out of the
`switch`, they actually have the same effect.
This may have caused the loop not to terminate when it should. E.g. when
`skip_backwards_streams` drops a buffer we should abort the aggregation
and wait for all pads to be filled again. Instead, we might have just
selected a subsequent pad as our new "best".
Replace `break` with `done = TRUE; break`, and `continue` with `break`.
Then simplify the code a bit.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/710>
When we are dropping a buffer in find_best_pad (e.g. waiting for a
keyframe, or skipping backwards timestamp), return
GST_AGGREGATOR_FLOW_NEED_DATA to make sure we have enough data at the
next run. Otherwise, a stream that accidentally fell behind (e.g.
relinking race, or just waiting for a keyframe) will never get the
opportunity to catch up to the other one, because the other one will
always keep advancing.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/696>
* Trying to disconnect a stream from a running splitmuxsink by flushing
it results in the FLUSH_START blocking in the stream queue's
gst_pad_pause_task because the flush did not unblock
complete_or_wait_on_out, so add a check for ctx->flushing there.
* Add a GST_SPLITMUX_BROADCAST_INPUT so check_completed_gop notices
flushing changed and the incoming push is unblocked.
* Pass the FLUSH_STOP along to the muxer without waiting.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/687>
GCC 10 was complaining like following. It really is complaining about default cases returning
with potentially unitialized *desval, but those cases in the switch should never be hit.
```
../subprojects/gst-plugins-good/gst/auparse/gstauparse.c: In function 'gst_au_parse_chain':
../subprojects/gst-plugins-good/gst/auparse/gstauparse.c:481:37: error: 'timestamp' may be used uninitialized in this function [-Werror=maybe-uninitialized]
481 | GST_BUFFER_TIMESTAMP (outbuf) = timestamp;
../subprojects/gst-plugins-good/gst/auparse/gstauparse.c:482:36: error: 'duration' may be used uninitialized in this function [-Werror=maybe-uninitialized]
482 | GST_BUFFER_DURATION (outbuf) = duration;
../subprojects/gst-plugins-good/gst/auparse/gstauparse.c:480:34: error: 'offset' may be used uninitialized in this function [-Werror=maybe-uninitialized]
480 | GST_BUFFER_OFFSET (outbuf) = offset;
cc1: all warnings being treated as errors
```
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/671>
This identifiers are registered in the MPEG-RA and defined
to be used by the Dolby Vision AVC/HEVC streams.
This is a first step to present the stream to the decoder.
Additional box parsing of DOVIConfigurationBox is necessary
to complete the media presentation with proper Dolby Vision
enhancements.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/658>
Previously, the user input for stsd entries is trusted completely, and
so a maliciously crafted file could choose the length of the stsd
entries arbitrarily and cause qtdemux to try to allocate up to 2GB of
memory (half of a 32 bit max int).
This patch fixes this by sanity checking the stsd input against the
size of the entire stsd atom.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/670>
During trak parsing, we need to check for the existence of stsd_entries,
otherwise, we end up with a NULL pointer to them. It is entirely
possible for the stsd to exist, but for it to have no entries, which the
previous checks did not take into account.
This patch adds a simply check to ensure that all files that do not
contain a stsd entry are deemed corrupt, and adds a test case to prevent
a regression.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/670>
Previously imagefreeze would always operate as non-live element and
output frames as fast as possible according to the configured segment
(via SEEK events) and the negotiated framerate from start to stop or the
other way around.
With the new live mode (enabled via the is-live property) it would only
output frames in PLAYING. Frames would be output according to the
negotiated framerate unless it would be too late, in which case it would
jump ahead and skip over the requirement amount of frames.
This makes it possible to actually use imagefreeze in live pipelines
without having to manually ensure somehow that it would start outputting
at the current running time and without still risking to fall behind
without recovery.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/653>
We never run as a live element, even if upstream is live, and never
output any buffers with latency but immediately generate buffers as
fast as we can according to the negotiated framerate.
Passing the query upstream would proxy whatever mode of operation
upstream has, which has nothing to do with how we produce buffers.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/653>
Move the SVMI stereoscopic atom parsing out to a helper
function to shrink qtdemux_parse_trak a bit.
Add a bounds check that the received atom is large enough
before parsing it.
Add a note to the atom parser that svmi comes from the
MPEG-A spec 23000-11.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/634>
Until now, do_expected_timeout() was shortly dropping the JBUF_LOCK in order
to push RTX event event without causing deadlock. As a side effect, some
CPU hung would happen as the timerqueue would get filled while looping over
the due timers. To mitigate this, we were processing the lost timer first and
placing into a queue the remainign to be processed later.
In the gap caused by an unlock, we could endup receiving one of the seqnum
present in the pending timers. In that case, the timer would not be found and
a new one was created. When we then update the expected timer, the seqnum
would already exist and the updated timer would be lost.
In this patch we remove the unlock from do_expected_timeout() and place all
pending RTX event into a queue (instead of pending timer). Then, as soon as
we have selected a timer to wait (or if there is no timer to wait for) we send
all the upstream RTX events. As we no longer unlock, we no longer need to pop
more then one timer from the queue, and we do so with the lock held, which
blocks any new colliding timers from being created.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/616>
When a GST_EVENT_FLUSH_START reaches the jitterbuffer, there is a chance that
our task is currently blocking waiting for a timer.
There was two problems:
* That wait wasn't checking for flushing situations
* The flushing handling wasn't waking up that conditional (to check whether it
should abort)
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/608>
In some algorithms (like yadif), the Y plane has to be handled different
than the UV plane. Therefore, the planar_y functions are now called for
the Y plane, and the nv12/nv21 functions are handling only the UV/VU
planes respectively.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/444>
Switching the deinterlacing mode on-the-fly from disabled to
auto used to work, but was broken by commit #1f21747c some
years ago.
Force re-negotiation with downstream when the mode or
fields properties are changed, otherwise deinterlace
never switches out of the passthrough mode.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/584>
First of all get rid of the atomic seeking boolean, which was only ever
set and never read. Replace it with a flushing boolean that is used in
the loop function to distinguish no buffer because of flushing and no
buffer because of an error as otherwise we could end up in a
GST_FLOW_ERROR case during flushing.
Also only reset the state of imagefreeze in flush-stop when all
processing is stopped instead of doing it as part of flush-start.
And last, get a reference to the imagefreeze buffer in the loop function
in the very beginning and work from that as otherwise it could in theory
be replaced or set to NULL in the meantime as we release and re-take the
mutex a couple of times during the loop function.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/580>
Backwards timestamps confuse librtmp, even if they're only backwards
relative to the other stream. If the timestamp of a stream is going
backwards related to the other stream, this property allows the muxer to
skip a few buffers until it reaches the timestamp of the other stream.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/572>
Allows us to request pads after writing header for streamable flv's.
For non-streamable it doesn't make sense to request a new pad after
writing the header, because the headers have been written already and we
can't add the new stream. But for streamable, any clients that connect
after the new pad has been added will be able to see both streams.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/572>
As we override the GLib item with our own structure, we cannot use any
function from GList or GQueue that would try to free the RTPJitterBufferItem.
In this patch, we move away from g_queue_new() which forces using
g_queue_free(). This this function could use g_slice_free() if there is any items
left in the queue. Passing the wrong size to GSLice may cause data corruption
and crash.
A better approach would be to use a proper intrusive linked list
implementation but that's left as an exercise for the next person
running into crashes caused by this.
Be ware that this regression was introduced 6 years ago in the following
commit [0], the call to flush() looked useless, as there was a g_queue_free()
afterward.
Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
[0] 479c7642fd
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/573>
The calculated threshold for timecode might be varying depending on
"max-size-timecode" and framerate.
For instance, with framerate 29.97 (30000/1001) and
"max-size-timecode=00:02:00;02", every fragment will have identical
number of frames 3598. However, when "max-size-timecode=00:02:00;00",
calculated next keyframe via gst_video_time_code_add_interval()
can be different per fragment, but this is the nature of timecode.
To compensate such timecode drift, we should keep track of expected
timecode of next fragment based on observed timecode.
The problem was this:
Due to the highly irregular arrival of RTX-packet the max-misorder variable
could be pushed very low. (-10).
If you then at some point get a big in the sequence-numbers (62 in the
test) you end up sending RTX-requests for some of those packets, and then
if the sender answers those requests, you are going to get a bunch of
RTX-packets arriving. (-13 and then 5 more packets in the test)
Now, if max-misorder is pushed very low at this point, these RTX-packets
will trigger the handle_big_gap_buffer() logic, and because they arriving
so neatly in order, (as they would, since they have been requested like
that), the gst_rtp_jitter_buffer_reset() will be called, and two things
will happen:
1. priv->next_seqnum will be set to the first RTX packet
2. the 5 RTX-packet will be pushed into the chain() function
However, at this point, these RTX-packets are no longer valid, the
jitterbuffer has already pushed lost-events for these, so they will now
be dropped on the floor, and never make it to the waiting loop-function.
And, since we now have a priv->next_seqnum that will never arrive
in the loop-function, the jitterbuffer is now stalled forever, and will
not push out another buffer.
The proposed fixes:
1. Don't use RTX in calculation of the packet-rate.
2. Don't use RTX in large-gap logic, as they are likely to be dropped.
If the start of the GOP is >= the requested running time, put it into a
new fragment. That is, split-at-running-time would always ensure that a
split happens as early as possible after the given running time.
Previously it was comparing against the current incoming timestamp,
which does not tell us what we actually want to know as it has no direct
relation to the GOP start/end.
