Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(125)

Issue 2260803002: Generalize FEC header formatting. (pt. 4) (Closed)

Created:
4 years, 4 months ago by brandtr
Modified:
4 years, 2 months ago
CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, danilchap, stefan-webrtc, mflodman
Base URL:
https://chromium.googlesource.com/external/webrtc.git@master
Target Ref:
refs/pending/heads/master
Project:
webrtc
Visibility:
Public.

Description

Generalize FEC header formatting. - Split out reading/writing of FEC headers to classes separate from ForwardErrorCorrection. This makes ForwardErrorCorrection oblivious to what FEC header scheme is used, and lets it focus on encoding/decoding the FEC payloads. - Add unit tests for FEC header readers/writers. - Split ForwardErrorCorrection::XorPackets into XorHeaders and XorPayloads and reuse these functions for both encoding and decoding. - Rename AttemptRecover -> AttemptRecovery in ForwardErrorCorrection. BUG=webrtc:5654 R=danilchap@webrtc.org, stefan@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/78db1582e514b5ecd5bf9a38ef6a1613ba3a5752

Patch Set 1 #

Total comments: 29

Patch Set 2 : Split up CL a bit. #

Patch Set 3 : Rebase. #

Patch Set 4 : Move unnamed namespace into webrtc namespace. #

Patch Set 5 : Use std::unique_ptr instead of move constructor during creation. #

Patch Set 6 : Rebase fixes. #

Patch Set 7 : New upload for dependent patch sets. #

Total comments: 13

Patch Set 8 : Review response. #

Patch Set 9 : Missed an 'override' for a virtual destructor. #

Total comments: 28

Patch Set 10 : Feedback response. #

Total comments: 32

Patch Set 11 : Partial responses to feedback. #

Total comments: 1

Patch Set 12 : Feedback response (with accidental rebase). #

Total comments: 59

Patch Set 13 : Feedback response 4. #

Patch Set 14 : Add protected_ssrc. #

Total comments: 20

Patch Set 15 : Feedback response 5. #

Total comments: 5

Patch Set 16 : Feedback response 6. #

Total comments: 6

Patch Set 17 : Feedback response 7. #

Patch Set 18 : Rebase. #

Patch Set 19 : Reorder some things. #

Total comments: 10

Patch Set 20 : Feedback response 8. #

Patch Set 21 : Improved data flow in unit test. #

Total comments: 14

Patch Set 22 : Feedback response 9. #

Patch Set 23 : Feedback response 10. #

Patch Set 24 : Rebase. #

Total comments: 4

Patch Set 25 : Review response 11. #

Patch Set 26 : Minor fixes, due to review response of FlexFEC CL. #

Patch Set 27 : Rebase after gyp deprecation. #

Patch Set 28 : Some more minor fixes. #

Patch Set 29 : Lint fix. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+994 lines, -469 lines) Patch
M webrtc/modules/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +1 line, -0 lines 0 comments Download
M webrtc/modules/rtp_rtcp/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +2 lines, -0 lines 0 comments Download
M webrtc/modules/rtp_rtcp/rtp_rtcp.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +2 lines, -0 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/fec_receiver_impl.h View 1 2 3 4 2 chunks +3 lines, -1 line 0 comments Download
M webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc View 1 2 3 4 2 chunks +4 lines, -3 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc View 1 2 3 4 5 6 3 chunks +9 lines, -7 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/forward_error_correction.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 6 chunks +171 lines, -72 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/forward_error_correction.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 19 chunks +290 lines, -334 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/forward_error_correction_internal.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +15 lines, -4 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/forward_error_correction_internal.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 5 chunks +12 lines, -10 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/producer_fec.h View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M webrtc/modules/rtp_rtcp/source/producer_fec.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 7 chunks +13 lines, -16 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 3 chunks +13 lines, -3 lines 0 comments Download
A webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +66 lines, -0 lines 0 comments Download
A webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +130 lines, -0 lines 0 comments Download
A webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 1 chunk +244 lines, -0 lines 0 comments Download
M webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc View 1 2 3 4 6 chunks +9 lines, -8 lines 0 comments Download
M webrtc/modules/rtp_rtcp/test/testFec/test_packet_masks_metrics.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 2 chunks +9 lines, -10 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 66 (29 generated)
brandtr
Can you guys have a look at this CL? It was hard to keep this ...
4 years, 3 months ago (2016-08-22 10:43:35 UTC) #2
danilchap
This CL does doesn't look unsplittable, Here are comments about some random places in the ...
4 years, 3 months ago (2016-08-22 13:00:41 UTC) #3
brandtr
Split up previous large CL in five semi-indenpendent ones. https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc File webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc#newcode1 webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:1: ...
4 years, 3 months ago (2016-08-23 08:19:11 UTC) #5
brandtr
https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/source/forward_error_correction.h File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/source/forward_error_correction.h#newcode143 webrtc/modules/rtp_rtcp/source/forward_error_correction.h:143: static ForwardErrorCorrection CreateUlpfec(); > On 2016/08/22 13:00:41, danilchap wrote: ...
4 years, 3 months ago (2016-08-23 11:17:48 UTC) #6
danilchap
few more suggestions, still not a full review for this part. https://codereview.webrtc.org/2260803002/diff/120001/webrtc/modules/rtp_rtcp/source/forward_error_correction.h File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (left): ...
4 years, 3 months ago (2016-08-24 10:42:40 UTC) #7
brandtr
Thanks for the feedback! See answers below. https://codereview.webrtc.org/2260803002/diff/120001/webrtc/modules/rtp_rtcp/source/forward_error_correction.h File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (left): https://codereview.webrtc.org/2260803002/diff/120001/webrtc/modules/rtp_rtcp/source/forward_error_correction.h#oldcode82 webrtc/modules/rtp_rtcp/source/forward_error_correction.h:82: ReceivedPacket(); On ...
4 years, 3 months ago (2016-08-24 11:34:24 UTC) #9
danilchap
few more comments https://codereview.webrtc.org/2260803002/diff/120001/webrtc/modules/rtp_rtcp/source/forward_error_correction.h File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (left): https://codereview.webrtc.org/2260803002/diff/120001/webrtc/modules/rtp_rtcp/source/forward_error_correction.h#oldcode82 webrtc/modules/rtp_rtcp/source/forward_error_correction.h:82: ReceivedPacket(); On 2016/08/24 11:34:23, brandtr wrote: ...
4 years, 3 months ago (2016-08-24 16:41:27 UTC) #10
brandtr
Feedback response.
4 years, 3 months ago (2016-08-25 13:20:29 UTC) #12
brandtr
https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc File webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc#newcode49 webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:49: packet_mask_() {} On 2016/08/24 16:41:27, danilchap wrote: > needed? ...
4 years, 3 months ago (2016-08-25 13:23:08 UTC) #14
danilchap
some more comments https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc File webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc#newcode35 webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:35: // These are pulled in from ...
4 years, 3 months ago (2016-08-25 17:49:15 UTC) #15
brandtr
https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc File webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc#newcode35 webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:35: // These are pulled in from forward_error_correction_internal.h On 2016/08/25 ...
4 years, 3 months ago (2016-08-26 11:22:23 UTC) #16
danilchap
description mention no unique_ptr for ForwardErrorCorrection. That is not true any more. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc ...
4 years, 3 months ago (2016-08-29 12:26:03 UTC) #17
brandtr
Thanks for feedback! Comments have been adressed. The FlexFEC header formatting CL has been rebased ...
4 years, 3 months ago (2016-08-30 07:12:42 UTC) #21
danilchap
https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc#newcode243 webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:243: static_cast<uint16_t>(last_seq_num - first_seq_num) - num_media_packets + On 2016/08/30 07:12:40, ...
4 years, 3 months ago (2016-08-30 12:29:01 UTC) #22
brandtr
Addressed comments. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc#newcode243 webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:243: static_cast<uint16_t>(last_seq_num - first_seq_num) - num_media_packets + On ...
4 years, 3 months ago (2016-08-30 14:14:24 UTC) #24
danilchap
https://codereview.webrtc.org/2260803002/diff/380002/webrtc/modules/rtp_rtcp/source/forward_error_correction.h File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2260803002/diff/380002/webrtc/modules/rtp_rtcp/source/forward_error_correction.h#newcode140 webrtc/modules/rtp_rtcp/source/forward_error_correction.h:140: uint32_t ssrc; there are 3 ssrcs in this class. ...
4 years, 3 months ago (2016-08-31 14:38:30 UTC) #25
brandtr
https://codereview.webrtc.org/2260803002/diff/380002/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2260803002/diff/380002/webrtc/modules/rtp_rtcp/source/forward_error_correction.cc#newcode388 webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:388: for (const auto& stream_packet_mask_info : fec_packet->packet_mask_infos) { This big ...
4 years, 3 months ago (2016-09-01 11:57:20 UTC) #29
danilchap
https://codereview.webrtc.org/2260803002/diff/470001/webrtc/modules/rtp_rtcp/source/forward_error_correction.h File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2260803002/diff/470001/webrtc/modules/rtp_rtcp/source/forward_error_correction.h#newcode141 webrtc/modules/rtp_rtcp/source/forward_error_correction.h:141: } packet_mask; does anonymous struct help here? i.e. may ...
4 years, 3 months ago (2016-09-01 12:58:48 UTC) #30
brandtr
Rebase.
4 years, 3 months ago (2016-09-01 13:43:46 UTC) #31
brandtr
https://codereview.webrtc.org/2260803002/diff/470001/webrtc/modules/rtp_rtcp/source/forward_error_correction.h File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2260803002/diff/470001/webrtc/modules/rtp_rtcp/source/forward_error_correction.h#newcode141 webrtc/modules/rtp_rtcp/source/forward_error_correction.h:141: } packet_mask; On 2016/09/01 12:58:48, danilchap wrote: > does ...
4 years, 3 months ago (2016-09-01 13:57:36 UTC) #32
danilchap
few more nits https://codereview.webrtc.org/2260803002/diff/530001/webrtc/modules/rtp_rtcp/source/forward_error_correction.h File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2260803002/diff/530001/webrtc/modules/rtp_rtcp/source/forward_error_correction.h#newcode120 webrtc/modules/rtp_rtcp/source/forward_error_correction.h:120: using ProtectedPacketList = std::list<std::unique_ptr<ProtectedPacket>>; may be ...
4 years, 3 months ago (2016-09-01 16:28:48 UTC) #33
brandtr
Rebase + improved data flow in unit test.
4 years, 3 months ago (2016-09-02 08:26:07 UTC) #34
brandtr
Addressed comments and rewritten unit tests to have clearer data flow. https://codereview.webrtc.org/2260803002/diff/530001/webrtc/modules/rtp_rtcp/source/forward_error_correction.h File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): ...
4 years, 3 months ago (2016-09-02 08:29:13 UTC) #36
danilchap
fewer more nits https://codereview.webrtc.org/2260803002/diff/570001/webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/570001/webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc#newcode31 webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:31: using PacketList = ::webrtc::ForwardErrorCorrection::PacketList; using? https://codereview.webrtc.org/2260803002/diff/570001/webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc#newcode45 ...
4 years, 3 months ago (2016-09-02 09:54:55 UTC) #37
brandtr
https://codereview.webrtc.org/2260803002/diff/570001/webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/570001/webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc#newcode31 webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:31: using PacketList = ::webrtc::ForwardErrorCorrection::PacketList; On 2016/09/02 09:54:55, danilchap wrote: ...
4 years, 3 months ago (2016-09-02 11:09:15 UTC) #38
danilchap
https://codereview.webrtc.org/2260803002/diff/570001/webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/570001/webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc#newcode45 webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:45: std::unique_ptr<uint8_t[]> packet_mask(new uint8_t[kUlpfecMaxPacketMaskSize]); On 2016/09/02 11:09:14, brandtr wrote: > ...
4 years, 3 months ago (2016-09-02 11:34:58 UTC) #39
brandtr
https://codereview.webrtc.org/2260803002/diff/570001/webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/570001/webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc#newcode45 webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:45: std::unique_ptr<uint8_t[]> packet_mask(new uint8_t[kUlpfecMaxPacketMaskSize]); On 2016/09/02 11:34:58, danilchap wrote: > ...
4 years, 3 months ago (2016-09-02 11:55:29 UTC) #40
danilchap
lgtm
4 years, 3 months ago (2016-09-02 12:01:19 UTC) #41
brandtr
Rebase.
4 years, 3 months ago (2016-09-06 14:35:38 UTC) #42
stefan-webrtc
https://codereview.webrtc.org/2260803002/diff/650001/webrtc/modules/rtp_rtcp/source/forward_error_correction.h File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2260803002/diff/650001/webrtc/modules/rtp_rtcp/source/forward_error_correction.h#newcode347 webrtc/modules/rtp_rtcp/source/forward_error_correction.h:347: class FecHeaderReader { Have you considered moving these to ...
4 years, 3 months ago (2016-09-13 08:49:34 UTC) #43
brandtr
https://codereview.webrtc.org/2260803002/diff/650001/webrtc/modules/rtp_rtcp/source/forward_error_correction.h File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2260803002/diff/650001/webrtc/modules/rtp_rtcp/source/forward_error_correction.h#newcode347 webrtc/modules/rtp_rtcp/source/forward_error_correction.h:347: class FecHeaderReader { On 2016/09/13 08:49:34, stefan-webrtc (holmer) wrote: ...
4 years, 3 months ago (2016-09-14 11:45:41 UTC) #49
brandtr
Rebase after gyp deprecation.
4 years, 3 months ago (2016-09-20 09:25:00 UTC) #50
brandtr
Rebase after gyp deprecation.
4 years, 3 months ago (2016-09-20 09:27:15 UTC) #52
brandtr
Comments have been adressed, PTAL again holmer :)
4 years, 3 months ago (2016-09-20 11:06:13 UTC) #53
stefan-webrtc
lgtm
4 years, 3 months ago (2016-09-20 11:21:32 UTC) #54
commit-bot: I haz the power
Patchset 29 (id:??) landed as https://crrev.com/78db1582e514b5ecd5bf9a38ef6a1613ba3a5752 Cr-Commit-Position: refs/heads/master@{#14316}
4 years, 2 months ago (2016-09-21 07:19:47 UTC) #65
brandtr
4 years, 2 months ago (2016-09-21 07:19:47 UTC) #66
Message was sent while issue was closed.
Committed patchset #29 (id:870001) manually as
78db1582e514b5ecd5bf9a38ef6a1613ba3a5752 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698