Feature Request P2
Status Update
No update yet.
Comments
ph...@microsoft.com
+hta for how to do that in SDP, +Rasmus who iirc reviewed the most recent changes to flexfec.
Both flexfec-03 as well as the RFC specify a bit more than was implemented in libwebrtc, in particular its retransmission support is missing (and the implementation ignores what is stipulated in 1.1.7)
I am not convinced that retransmission support is necessary even, NACK works fine.
One thing that is terrible in the current implementation is that the 46-byte masks are implemented ontop of ulpfec's 48 byte masks in flexfec_header_reader_writer.cc which ends up being terrible.
Solving that properly would be great.
[Monorail components: Video]
Both flexfec-03 as well as the RFC specify a bit more than was implemented in libwebrtc, in particular its retransmission support is missing (and the implementation ignores what is stipulated in 1.1.7)
I am not convinced that retransmission support is necessary even, NACK works fine.
One thing that is terrible in the current implementation is that the 46-byte masks are implemented ontop of ulpfec's 48 byte masks in flexfec_header_reader_writer.cc which ends up being terrible.
Solving that properly would be great.
[Monorail components: Video]
gi...@appspot.gserviceaccount.com
The following revision refers to this bug:
https://webrtc.googlesource.com/src/+/f3de65aebe80b2c6f3fa6b6de1e710086debdfec
commit f3de65aebe80b2c6f3fa6b6de1e710086debdfec
Author: Yosef Twaik <twaik@meta.com>
Date: Thu May 04 15:19:54 2023
Change ReceivedFecPacket to have list of ssrcs, seq nums and masks.
This change replaces ReceivedFecPacket FEC header fields with vectors (for protected ssrcs, sequence numbers and masks), which is needed to support protection of multiple ssrcs in the same FEC packet (as part of the flexfec RFC -https://datatracker.ietf.org/doc/html/rfc8627 ).
Bug: webrtc:15002
Change-Id: I82c54203fcfec10c760f34f805cc6308562e3df1
Reviewed-on:https://webrtc-review.googlesource.com/c/src/+/303200
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Rasmus Brandt <brandtr@webrtc.org>
Reviewed-by: Rasmus Brandt <brandtr@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#40075}
[modify]https://crrev.com/f3de65aebe80b2c6f3fa6b6de1e710086debdfec/modules/rtp_rtcp/source/forward_error_correction.cc
[modify]https://crrev.com/f3de65aebe80b2c6f3fa6b6de1e710086debdfec/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc
[modify]https://crrev.com/f3de65aebe80b2c6f3fa6b6de1e710086debdfec/modules/rtp_rtcp/source/forward_error_correction.h
[modify]https://crrev.com/f3de65aebe80b2c6f3fa6b6de1e710086debdfec/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc
[modify]https://crrev.com/f3de65aebe80b2c6f3fa6b6de1e710086debdfec/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc
[modify]https://crrev.com/f3de65aebe80b2c6f3fa6b6de1e710086debdfec/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc
commit f3de65aebe80b2c6f3fa6b6de1e710086debdfec
Author: Yosef Twaik <twaik@meta.com>
Date: Thu May 04 15:19:54 2023
Change ReceivedFecPacket to have list of ssrcs, seq nums and masks.
This change replaces ReceivedFecPacket FEC header fields with vectors (for protected ssrcs, sequence numbers and masks), which is needed to support protection of multiple ssrcs in the same FEC packet (as part of the flexfec RFC -
Bug: webrtc:15002
Change-Id: I82c54203fcfec10c760f34f805cc6308562e3df1
Reviewed-on:
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Rasmus Brandt <brandtr@webrtc.org>
Reviewed-by: Rasmus Brandt <brandtr@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#40075}
[modify]
[modify]
[modify]
[modify]
[modify]
[modify]
gi...@appspot.gserviceaccount.com
The following revision refers to this bug:
https://webrtc.googlesource.com/src/+/b4015689b854762be6e4f60f33419a5c17c503d4
commit b4015689b854762be6e4f60f33419a5c17c503d4
Author: Yosef Twaik <twaik@meta.com>
Date: Thu May 04 15:27:43 2023
Initial copy of flexfec_header_reader_writer.
Create a copy of flexfec_header_reader_writer for changing the implementation according to updated RFC. The fork is needed, since the updated RFC is incompatible with flexfec-03.
In the updated RFC, we receive the list and the number of protected ssrcs from the RTP header (from it's CSRCs , and CSRC count fields).
This Change is only a copy of the existing files. This will make it easier to understand the changes to the implementation in the next change sets.
Bug: webrtc:15002
Change-Id: I31bf5eca0d8f3cb23b4caabb477897eeb0ca6d96
Reviewed-on:https://webrtc-review.googlesource.com/c/src/+/303240
Reviewed-by: Rasmus Brandt <brandtr@webrtc.org>
Commit-Queue: Rasmus Brandt <brandtr@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#40103}
[add]https://crrev.com/b4015689b854762be6e4f60f33419a5c17c503d4/modules/rtp_rtcp/source/flexfec_header_reader_writer2.cc
[modify]https://crrev.com/b4015689b854762be6e4f60f33419a5c17c503d4/modules/rtp_rtcp/BUILD.gn
[add]https://crrev.com/b4015689b854762be6e4f60f33419a5c17c503d4/modules/rtp_rtcp/source/flexfec_header_reader_writer2.h
[add]https://crrev.com/b4015689b854762be6e4f60f33419a5c17c503d4/modules/rtp_rtcp/source/flexfec_header_reader_writer2_unittest.cc
commit b4015689b854762be6e4f60f33419a5c17c503d4
Author: Yosef Twaik <twaik@meta.com>
Date: Thu May 04 15:27:43 2023
Initial copy of flexfec_header_reader_writer.
Create a copy of flexfec_header_reader_writer for changing the implementation according to updated RFC. The fork is needed, since the updated RFC is incompatible with flexfec-03.
In the updated RFC, we receive the list and the number of protected ssrcs from the RTP header (from it's CSRCs , and CSRC count fields).
This Change is only a copy of the existing files. This will make it easier to understand the changes to the implementation in the next change sets.
Bug: webrtc:15002
Change-Id: I31bf5eca0d8f3cb23b4caabb477897eeb0ca6d96
Reviewed-on:
Reviewed-by: Rasmus Brandt <brandtr@webrtc.org>
Commit-Queue: Rasmus Brandt <brandtr@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#40103}
[add]
[modify]
[add]
[add]
gi...@appspot.gserviceaccount.com
The following revision refers to this bug:
https://webrtc.googlesource.com/src/+/4c1e9598a30b46500ef1b145e3e9965fc34913e3
commit 4c1e9598a30b46500ef1b145e3e9965fc34913e3
Author: Yosef Twaik <twaik@meta.com>
Date: Mon May 15 16:56:11 2023
Change flexfec header reader to parse according to updated RFC.
This change changes the flexfec header reader ReadFecHeader function to parse the FEC header according the the updated RFC. The fec_packet argument is expected to have the protected ssrcs list already populated, as they should be retrieved from the RTP header.
Updated and added Reader unittests. Unittests that are relevant for the Writer, were put inside a comment. In the next change set, when the header writer will be updated, we will update the unittests accordingly.
Bug: webrtc:15002
Change-Id: I118303e31c15c356ffeb2c0aafe503cf293bcad6
Reviewed-on:https://webrtc-review.googlesource.com/c/src/+/303260
Reviewed-by: Rasmus Brandt <brandtr@webrtc.org>
Commit-Queue: Rasmus Brandt <brandtr@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#40172}
[modify]https://crrev.com/4c1e9598a30b46500ef1b145e3e9965fc34913e3/modules/rtp_rtcp/source/flexfec_header_reader_writer2.cc
[modify]https://crrev.com/4c1e9598a30b46500ef1b145e3e9965fc34913e3/modules/rtp_rtcp/BUILD.gn
[modify]https://crrev.com/4c1e9598a30b46500ef1b145e3e9965fc34913e3/modules/rtp_rtcp/source/flexfec_header_reader_writer2.h
[modify]https://crrev.com/4c1e9598a30b46500ef1b145e3e9965fc34913e3/modules/rtp_rtcp/source/flexfec_header_reader_writer2_unittest.cc
commit 4c1e9598a30b46500ef1b145e3e9965fc34913e3
Author: Yosef Twaik <twaik@meta.com>
Date: Mon May 15 16:56:11 2023
Change flexfec header reader to parse according to updated RFC.
This change changes the flexfec header reader ReadFecHeader function to parse the FEC header according the the updated RFC. The fec_packet argument is expected to have the protected ssrcs list already populated, as they should be retrieved from the RTP header.
Updated and added Reader unittests. Unittests that are relevant for the Writer, were put inside a comment. In the next change set, when the header writer will be updated, we will update the unittests accordingly.
Bug: webrtc:15002
Change-Id: I118303e31c15c356ffeb2c0aafe503cf293bcad6
Reviewed-on:
Reviewed-by: Rasmus Brandt <brandtr@webrtc.org>
Commit-Queue: Rasmus Brandt <brandtr@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#40172}
[modify]
[modify]
[modify]
[modify]
gi...@appspot.gserviceaccount.com
The following revision refers to this bug:
https://webrtc.googlesource.com/src/+/ade07ca45e722141090348f0b3d9951bfaac96ca
commit ade07ca45e722141090348f0b3d9951bfaac96ca
Author: Yosef Twaik <twaik@meta.com>
Date: Sun Jun 04 13:16:01 2023
Rename current flexfec implementation flexfec_03
As per the comment inhttps://webrtc-review.googlesource.com/c/src/+/303240
on the flexfec_header_reader_writer2.h, renaming this file to flexfec_header_reader_writer.h
and renaming the current implementation to flexfec_03_header_reader_writer.h
as it is based on the 03 draft of the RFC.
Change-Id: I80cb2aba6225ec7cd989a134c3204d1db0ac6f7c
Bug: webrtc:15002
Reviewed-on:https://webrtc-review.googlesource.com/c/src/+/307600
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#40231}
[modify]https://crrev.com/ade07ca45e722141090348f0b3d9951bfaac96ca/modules/rtp_rtcp/source/forward_error_correction.cc
[delete]https://crrev.com/43df03db38eae0508f70883fdbf0711c3d81d800/modules/rtp_rtcp/source/flexfec_header_reader_writer2.cc
[modify]https://crrev.com/ade07ca45e722141090348f0b3d9951bfaac96ca/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc
[add]https://crrev.com/ade07ca45e722141090348f0b3d9951bfaac96ca/modules/rtp_rtcp/source/flexfec_03_header_reader_writer.cc
[delete]https://crrev.com/43df03db38eae0508f70883fdbf0711c3d81d800/modules/rtp_rtcp/source/flexfec_header_reader_writer2.h
[delete]https://crrev.com/43df03db38eae0508f70883fdbf0711c3d81d800/modules/rtp_rtcp/source/flexfec_header_reader_writer2_unittest.cc
[modify]https://crrev.com/ade07ca45e722141090348f0b3d9951bfaac96ca/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc
[modify]https://crrev.com/ade07ca45e722141090348f0b3d9951bfaac96ca/modules/rtp_rtcp/source/rtp_fec_unittest.cc
[add]https://crrev.com/ade07ca45e722141090348f0b3d9951bfaac96ca/modules/rtp_rtcp/source/flexfec_03_header_reader_writer.h
[modify]https://crrev.com/ade07ca45e722141090348f0b3d9951bfaac96ca/modules/rtp_rtcp/source/flexfec_header_reader_writer.h
[add]https://crrev.com/ade07ca45e722141090348f0b3d9951bfaac96ca/modules/rtp_rtcp/source/flexfec_03_header_reader_writer_unittest.cc
[modify]https://crrev.com/ade07ca45e722141090348f0b3d9951bfaac96ca/modules/rtp_rtcp/BUILD.gn
commit ade07ca45e722141090348f0b3d9951bfaac96ca
Author: Yosef Twaik <twaik@meta.com>
Date: Sun Jun 04 13:16:01 2023
Rename current flexfec implementation flexfec_03
As per the comment in
on the flexfec_header_reader_writer2.h, renaming this file to flexfec_header_reader_writer.h
and renaming the current implementation to flexfec_03_header_reader_writer.h
as it is based on the 03 draft of the RFC.
Change-Id: I80cb2aba6225ec7cd989a134c3204d1db0ac6f7c
Bug: webrtc:15002
Reviewed-on:
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#40231}
[modify]
[delete]
[modify]
[add]
[delete]
[delete]
[modify]
[modify]
[add]
[modify]
[add]
[modify]
gi...@appspot.gserviceaccount.com
The following revision refers to this bug:
https://webrtc.googlesource.com/src/+/bde7c6ad116bba6e02293ca2d99e4b83ceb4d0b9
commit bde7c6ad116bba6e02293ca2d99e4b83ceb4d0b9
Author: Yosef Twaik <twaik@meta.com>
Date: Wed Jun 07 17:31:54 2023
Change FinalizeFecHeader to receive list of streams
Changed FinalizeFecHeader to recieve a list of `ProtectedStream` struct,
in order to prepare for receiving multiple ssrcs to protect in the same
FEC packet header. Implementation of the multistream case will follow in
next CL.
Change-Id: I697ef9172a07797a6f500b9ec3a9916f8f45bc04
Bug: webrtc:15002
Reviewed-on:https://webrtc-review.googlesource.com/c/src/+/307620
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#40269}
[modify]https://crrev.com/bde7c6ad116bba6e02293ca2d99e4b83ceb4d0b9/modules/rtp_rtcp/source/forward_error_correction.cc
[modify]https://crrev.com/bde7c6ad116bba6e02293ca2d99e4b83ceb4d0b9/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc
[modify]https://crrev.com/bde7c6ad116bba6e02293ca2d99e4b83ceb4d0b9/modules/rtp_rtcp/source/forward_error_correction.h
[modify]https://crrev.com/bde7c6ad116bba6e02293ca2d99e4b83ceb4d0b9/modules/rtp_rtcp/source/ulpfec_header_reader_writer.h
[modify]https://crrev.com/bde7c6ad116bba6e02293ca2d99e4b83ceb4d0b9/modules/rtp_rtcp/source/flexfec_03_header_reader_writer.h
[modify]https://crrev.com/bde7c6ad116bba6e02293ca2d99e4b83ceb4d0b9/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc
[modify]https://crrev.com/bde7c6ad116bba6e02293ca2d99e4b83ceb4d0b9/modules/rtp_rtcp/source/flexfec_header_reader_writer.h
[modify]https://crrev.com/bde7c6ad116bba6e02293ca2d99e4b83ceb4d0b9/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc
[modify]https://crrev.com/bde7c6ad116bba6e02293ca2d99e4b83ceb4d0b9/modules/rtp_rtcp/source/flexfec_03_header_reader_writer_unittest.cc
[modify]https://crrev.com/bde7c6ad116bba6e02293ca2d99e4b83ceb4d0b9/modules/rtp_rtcp/source/flexfec_03_header_reader_writer.cc
commit bde7c6ad116bba6e02293ca2d99e4b83ceb4d0b9
Author: Yosef Twaik <twaik@meta.com>
Date: Wed Jun 07 17:31:54 2023
Change FinalizeFecHeader to receive list of streams
Changed FinalizeFecHeader to recieve a list of `ProtectedStream` struct,
in order to prepare for receiving multiple ssrcs to protect in the same
FEC packet header. Implementation of the multistream case will follow in
next CL.
Change-Id: I697ef9172a07797a6f500b9ec3a9916f8f45bc04
Bug: webrtc:15002
Reviewed-on:
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#40269}
[modify]
[modify]
[modify]
[modify]
[modify]
[modify]
[modify]
[modify]
[modify]
[modify]
gi...@appspot.gserviceaccount.com
The following revision refers to this bug:
https://webrtc.googlesource.com/src/+/61deed5696716c2f48d38f327b2b7335347af4d7
commit 61deed5696716c2f48d38f327b2b7335347af4d7
Author: Yosef Twaik <twaik@meta.com>
Date: Thu Jun 08 13:22:22 2023
Change flexfec header writer to finalize header according to updated RFC
Change implementation of `FinalizeFecHeader` to write the FEC header
for multiple ssrcs according to the updated RFC.
Change-Id: I280964b2e53c3579f348fbd42815c966840375ac
Bug: webrtc:15002
Reviewed-on:https://webrtc-review.googlesource.com/c/src/+/307601
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#40270}
[modify]https://crrev.com/61deed5696716c2f48d38f327b2b7335347af4d7/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc
[modify]https://crrev.com/61deed5696716c2f48d38f327b2b7335347af4d7/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc
commit 61deed5696716c2f48d38f327b2b7335347af4d7
Author: Yosef Twaik <twaik@meta.com>
Date: Thu Jun 08 13:22:22 2023
Change flexfec header writer to finalize header according to updated RFC
Change implementation of `FinalizeFecHeader` to write the FEC header
for multiple ssrcs according to the updated RFC.
Change-Id: I280964b2e53c3579f348fbd42815c966840375ac
Bug: webrtc:15002
Reviewed-on:
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#40270}
[modify]
[modify]
ah...@nvidia.com
Has the meaning of the k-bit changed from flexfec-03 to RFC ?
[Deleted User] <[Deleted User]>
Yes you're right. In the 03 draft, "0" meant there is another block to the mask, and in the final RFC "0" means that's the last block.
Will publish a fix CL.
Will publish a fix CL.
di...@microsoft.com
Backwards compatibility with 03 is in theory being preserved, but how is that being validated? (asking because of the k-bit issue that was just raised)
[Deleted User] <[Deleted User]>
There will not be backwards compatibility with 03. Currently only the FEC header parsers are implemented. Once we finish all the wiring, and the parsing of the RTP header of FEC packets, the plan is to make this a different protocol that is published in SDP.
di...@microsoft.com
Ok, sounds like 03 will still be a protocol that can be negotiated, which is what I wanted to confirm (i.e., peers running an older Chromium version can still use FLEXFEC-03 against newer peers).
gi...@appspot.gserviceaccount.com
The following revision refers to this bug:
https://webrtc.googlesource.com/src/+/75a3ba216e1cccfd2c7f77ab184460dc4b2de002
commit 75a3ba216e1cccfd2c7f77ab184460dc4b2de002
Author: Yosef Twaik <twaik@meta.com>
Date: Thu Nov 16 12:00:21 2023
Reverse the kbits logic according to RFC
The updated Flexfec RFC states that a kbit of "0" means this is the last block of the mask, whereas in the 03 draft, "0" meant there's another block.
Reversing the logic in the updated RFC parser to fix.
Bug: webrtc:15002
Change-Id: I40e4c950b09ddf2db9da6c01908737282161bf1c
Reviewed-on:https://webrtc-review.googlesource.com/c/src/+/327580
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#41174}
[modify]https://crrev.com/75a3ba216e1cccfd2c7f77ab184460dc4b2de002/modules/rtp_rtcp/source/flexfec_header_reader_writer.cc
[modify]https://crrev.com/75a3ba216e1cccfd2c7f77ab184460dc4b2de002/modules/rtp_rtcp/source/flexfec_header_reader_writer_unittest.cc
commit 75a3ba216e1cccfd2c7f77ab184460dc4b2de002
Author: Yosef Twaik <twaik@meta.com>
Date: Thu Nov 16 12:00:21 2023
Reverse the kbits logic according to RFC
The updated Flexfec RFC states that a kbit of "0" means this is the last block of the mask, whereas in the 03 draft, "0" meant there's another block.
Reversing the logic in the updated RFC parser to fix.
Bug: webrtc:15002
Change-Id: I40e4c950b09ddf2db9da6c01908737282161bf1c
Reviewed-on:
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#41174}
[modify]
[modify]
is...@google.com
ly...@gmail.com
Any update for multiple stream protection ?
Description
Context:
The current implementation of “flexfec” in Webrtc is:
1) Based on an older version of the RFC (see diff in FEC header structure -
2) Doesn’t implement the RFC fully, as it does not support multi-stream protection with FEC packets.
We are working on adding the above functionality with accordance to the up-to-date version of the RFC.
Our plan is roughly:
- Since the header format has changed, we will fork flexfec_header_reader_writer and re-implement with updated format. (adding additional internal parameters increase cyclomatic complexity a bit too much)
- Update the new flexfec_header_reader_writer to support decoding multiple SSRCs.
- Update the new flexfec_header_reader_writer to support encoding multiple SSRCs.
- Higher level changes in call, video_receive_stream etc. Most usages of the single stream case can changed to be a special case of multi-stream (of length 1).
- Since this is a slightly different protocol structure we will need to introduce this as a new “flexfec-21” in the SDP. The current one is “flexfec-03” as it is based on the “03” revision of the RFC. The last draft before the final is 20, so going with 21.
Note: Once Retransmission and Fixed block schema are implemented, this can be changed to a final version, “flexfec”.
- Add new field trial to experiment with the new implementation.
We can maintain backwards compatibility for now (offering both 03 and 21); in the future it makes sense to eventually deprecate 03
Any thoughts or comments are greatly appreciated; meanwhile we will start preparing the more straightforward header reading/writing CLs.