rtcpbuffer: allow Padding bit for non-compound packets #1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A very common pipeline is feeding an encoder inheriting from GstAudioEncoder into a payloader inheriting from GstRtpBasePayload. If this is the case, the rtp-timestamp will, if perfect_rtptime is TRUE, reflect the amount of bytes in each packet, and not (like it should) reflect the timestamp of the packets in the clock-rate domain. Example: 20ms AAC packets with clock-rate of 90000, should have the rtp-timestamp incrementing with 90000 * 20 / 1000 = 1800. Currently, because perfect_rtptime is default TRUE, the AAC-packets will be incrementing with the buffer-size of the packets, which is typically 64000 * 20 / 1000 = 1280. In the case that bitrate and clock-rate are the same (like with ALAW and MULAW) it will work by chance, which is probably why it has not been discovered earlier. https://bugzilla.gnome.org/show_bug.cgi?id=761943
An implementation of https://tools.ietf.org/html/rfc6464, it can translate between audio level indication from an RTP packet and GstRTPAudioLevelMeta.
To embed GstRTPAudioLevelMeta as specified by RFC 6464
To read RFC 6464 information from the RTP packet and add that information as a GstRTPAudioLevelMeta to the outgoing buffer.
When doing DTX generation in decoders, they are indeed producing buffers without having a corresponding input buffer, so this code needs to be revisited, and perhaps need a special method for pushing buffers like this? https://bugzilla.gnome.org/show_bug.cgi?id=773106
…eo alignment requirements" [pexhack] This reverts commit 8b96b52.
…and use duration 0"" [pexhack] This reverts commit b60ab75. https://bugzilla.gnome.org/show_bug.cgi?id=754224
Add a "request-key-unit-interval" property which is used to enable and rate limit number of upstream key unit requests. Add "error-drop-frame" property to drop frames that have decode error. Add "max-errors" property: How many consecutive errors one should accept before returning flow error should be up to the application, not the element. Hence max-error should be exposed as a property.
…xhack] A simple workaround for non-robust decoders (vp9).
Sync points are often important when debugging and deserves DEBUG level.
If timestamp is none then we can't track how long it is since last event. Send immediately.
…t [pexhack] Extend existing gap event with additional parameter 'no-packet-loss'. Indicates that a gap in a stream occurred not due to the packet loss. In that case decoders configured with error-drop-frame=true should wait for a key-frame, but not send force-key-unit event
Depayloaders should be less strict about spec conformance on incoming packets.
Used to communicate out-of-band information between the codec and the payloader.
With this patch pexip/mcu@f819d92 @johnbassett discovered that old frames would be produced if stopping and starting a layer. However, setting drop-only always to true means we can never increase the framerate making the caps negotiation fail. This hack sets drop-only to true when the input rate is higher or equal to the output one. A proper fix would be to identify the bug @johnbassett discovered and do a test and fix for that in the videorate element.
…f frame" This is because we set this flag ourselves in the implementations. This reverts commit 7013a58.
A way to embed ROI meta in the RTP extension header.
…& writer signal - Initial support for roi-ext-id - This allow an application to provide a custom roi-ext-hdr reader and writer callback by passing both the GstRTPBuffer and plain buffer only containing metas. - Payloaders can still make use of the roi-ext-id property to write all GstVideoRegionOfInterest metas as RTP extension headers if desired. - (De)payloaders should still make use of the roi-ext-id property to write/read all GstVideoRegionOfInterest metas as RTP extension headers. Wheter or not the custom signal fires depends on if it is bound or not. So this commit also changes the logic to make the roi-ext-id required. - rtpbasedepayload: parse roi-ext-id from sinkcaps This allows pay "roi-ext-id=5 ! depay" to work as expected since the payloader will signal via caps the ext-id. This is really a hack, and should die as soon as we get the new and fancy RTP header extensions base object / element.
We validate the header extensions length of an RTP buffer by comparing it against the block size. Since we multiply the length in words by 4 to get the length in bytes, a suitably large length could cause a wrapround of the uint16, giving a lower length which erroneously passes the check and allows the buffer to be mapped.
Basically, don't set a DISCONT flag when detecting a seqnum jump. This is useful for ULPFEC, as a FEC packet might be hiding inside the original stream, and this stream is actually completely valid, even if the FEC packet is removed.
While this is not something we want to argue upstream, in the "pexip case" the jitterbuffer handles setting DISCONT for us, and we don't want any of our depayloaders messing around with its own gap handling.
jmb202
reviewed
Mar 18, 2021
| padding = TRUE; | ||
| /* last byte of padding contains the number of padded bytes including | ||
| * itself */ | ||
| pad_bytes = data[data_len - 1]; |
There was a problem hiding this comment.
Probably want to reject data_len == header_len if the padding bit is set (otherwise, we're using the bottom byte of the length field as the padding length, which is unlikely to be a thing).
There was a problem hiding this comment.
So I wrote this one to address it: https://github.com/pexip/gst-plugins-base/pull/1/files#diff-89f869857981d183cf338bd394969e5580bce2723d9cc1273fcceba2af2b4872R1009
But it looks like it is covered, between pad_bytes being 0 (invalid) or the length of the packet being invalid.
jmb202
reviewed
Mar 18, 2021
jmb202
reviewed
Mar 18, 2021
...and deal with non-modulo 4 padding bytes for non-standard RTCP. This is thanks to Chrome sending these packets to us when doing TWCC. Whereas the standard says that non-modulo 4 bytes padding are not allowed, for reduced size and TWCC in particular, this does not seem to apply. In order to be able to safely parse FCIs from these packets, a new API is proposed: gst_rtcp_packet_fb_get_fci_length_bytes, that will respect the non-mod4 padding and subtract that from the valid FCI length. Co-Authored-By: Tulio Beloqui <[email protected]> Co-Authored-By: John-Mark Bell <[email protected]>
havardgraff
force-pushed
the
hgr/chrome-twcc-fixes
branch
from
165d26a to
cca4ac9
Compare
havardgraff
changed the title
|
Aha. Yes, ok!
…On Fri, 19 Mar 2021, 08:30 Håvard Graff, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In gst-libs/gst/rtp/gstrtcpbuffer.c
<#1 (comment)>:
> /* get packet length */
header_len = (((data[2] << 8) | data[3]) + 1) << 2;
if (data_len < header_len)
goto wrong_length;
+ /* check padding of new packet */
+ if (data[0] & 0x20) {
+ guint8 pad_bytes;
+ padding = TRUE;
+ /* last byte of padding contains the number of padded bytes including
+ * itself */
+ pad_bytes = data[data_len - 1];
So I wrote this one to address it:
https://github.com/pexip/gst-plugins-base/pull/1/files#diff-89f869857981d183cf338bd394969e5580bce2723d9cc1273fcceba2af2b4872R1009
But it looks like it is covered, between pad_bytes being 0 (invalid) or
the length of the packet being invalid.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABXZYX2NGRLP6LHZZ7YGWDTEMDSTANCNFSM4ZNOAOBQ>
.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
...and deal with non-modulo 4 padding bytes for non-standard RTCP.
This is thanks to Chrome sending these packets to us when doing TWCC.
Whereas the standard says that non-modulo 4 bytes padding are not allowed,
for reduced size and TWCC in particular, this does not seem to apply.
In order to be able to safely parse FCIs from these packets, a new API
is proposed: gst_rtcp_packet_fb_get_fci_length_bytes, that will
respect the non-mod4 padding and subtract that from the valid FCI length.
Co-Authored-By: Tulio Beloqui [email protected]
Co-Authored-By: John-Mark Bell [email protected]