* Try to make variable and function names more clear.
* Add plenty of comments describing the logic step-by-step.
* Improve the logging around this, making the logs easier to read and
understand when debugging these issues.
* Revise the logic of packets that are actually beyond saving in doing
the following:
1. Do an optimistic estimation of which packets can still arrive.
2. Based on this, find which packets (and duration) are now hopelessly
lost.
3. Issue an immediate lost-event for the hopelessly lost and then add
lost/rtx timers for the ones we still hope to save, meaning that if
they are to arrive, they will not be discarded.
* Revise the use of rtx-delay:
Earlier the rtx-delay would vary, depending on the pts of the latest
packet and the estimated pts of the packet it being issued a RTX for,
but now that we aim to estimate the PTS of the missing packet accurately,
the RTX delay should remain the same for all packets.
Meaning: If the packet have a PTS of X, the delay in asked for a RTX
for this packet is always a constant X + delay, not a variable one.
* Finally ensure that the chaotic "check-for-stall" tests uses timestamps
that starts from 0 to make them easier to debug.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/952>
Currently, videocrop, only negotiates raw caps (system memory) because
it's the type of memory it can modify. Nonetheless, it's also possible
for the element to handle non-raw caps when only adding the crop meta
is possible, in other words, when downstream buffer pools expose the
crop API.
This patch enable non-raw caps negotiation. If downstream doesn't
expose crop API and negotiated caps are featured, the negotiation
fails.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/915>
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>
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>
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>
- 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>
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>
- introduce two new properties:
* temporal-scalability-layer-flags:
Provide fine-grained control of layer encoding to the
outside world. The flags sequence should be a multiple of
the periodicity and is indexed by a running count of encoded
frames modulo the sequence length.
* temporal-scalability-layer-sync-flags:
Specify the pattern of inter-layer synchronisation (i.e.
which of the frames generated by the layer encoding
specification represent an inter-layer synchronisation).
There must be one entry per entry in
temporal-scalability-layer-flags.
- apply temporal scalability settings and expose as buffer
metadata.
This allows the codec to allocate a given frame to the correct
internal bitrate allocator. Additionally, all the
non-bitstream metadata needed to payload a temporally scaled
stream is now attached to each output buffer as a
GstVideoVP8Meta.
- add unit test for temporally scaled encoding.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/728>
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>
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>
- make test_encode_simple cope with libvpx built with
CONFIG_REALTIME_ONLY. Sadly, there's no way to detect this at
runtime beyond trying to set lag-in-frames to >0, pushing a
buffer and catching the GST_FLOW_NOT_NEGOTIATED return.
- fix bitrot in test_encode_simple_when_bitrate_set_to_zero.
- port test_encode_simple to GstHarness and introduce a separate
test for the lag-in-frames property.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/708>
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>
As part of this also change the default bitrate value to 0. The default
value was 256000 previously. In reality, if the property was not set the
bitrate value would be scaled according to the resolution which is not
very intuitive behavior. It is better to use 0 for this purpose. Now
together with newly introduced property "bits-per-pixel" 0 means to
assign the bitrate according to resolution/framerate.
The default bitrates are now
- 1.2Mbps for VP8 720p@30fps
- 0.8Mbps for VP9 720p@30fps
and scaled accordingly for different resolutions/framerates.
Previously the default bitrate was also not scaled according to the
framerate but only took the resolution into account.
This also fixes the side effect of setting bitrate to 0. Previously
encoder would not produce any data at all.
Addition from Sebastian Dröge <sebastian@centricular.com> to assume
30fps if no framerate is given in the caps instead of not calculating
any bitrate at all.
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/611>
For VP8 it's possible to signal width or height to be 0, but it does
not make sense to do so. For VP9 it's impossible. Hence, we most
likely have a corrupt stream. Trying to negotiate caps downstream with
either width or height as 0 will fail with something like
gst_video_decoder_negotiate_default: assertion 'GST_VIDEO_INFO_WIDTH (&state->info) != 0' failed
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/610>
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.
gst_buffer_map () results in memcopying when a GstBuffer contains
more than one GstMemory.
This has quite an impact on performance on systems with limited amount
of resources. With this patch the whole GstBuffer will not be mapped at
once, instead each individual GstMemory will be iterated and mapped
separately.
There is a use-case for a server to re-payload opus going through it.
Problem was that the payloader requires channels in the caps, but
this is not something the depayloader can parse out of the stream, meaning
caps-negotiation would fail.
Removing the requirement of channels in the template-caps fixes this.
Mainly generalize all the latest tests that have found various stalls
in the jitterbuffer, so that they only consist of a series of packets
with various seqnum/rtptime/rtx combinations, arriving at a specific time.
This means future tests can be more easily written to prove certain
behavior does not cause stalls.
Also fix the warning on windows:
warning C4244: 'initializing': conversion from 'double' to 'gint', possible loss of data
