Chromium Code Reviews
|
|
Chromium Code Reviews
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. |
DescriptionGeneralize 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. #Depends on Patchset: Messages
Total messages: 66 (29 generated)
brandtr@webrtc.org changed reviewers: + danilchap@webrtc.org, stefan@webrtc.org
Can you guys have a look at this CL? It was hard to keep this CL small, since it changes several key parts of ForwardErrorCorrection. Hope it's still reviewable :) -- Rasmus
This CL does doesn't look unsplittable, Here are comments about some random places in the CL and suggestions how to split it into few smaller CLs. https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:1: /* make unittest filename match source file it is testing, i.e. rename this into flexfec_header_reader_writer_unittest.cc https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:36: : rtp_data_callback_(), it's cleaner to omit default constructions of members (same for generator_) https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/fec_test_helper.h (right): https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/fec_test_helper.h:63: class MediaPacketGenerator { Can you comment why do you need 2 packet generators? https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.h (right): https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.h:1: /* Generalizing FEC header reading/writing can be one CL, Adding FlexFec - another one. https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:108: class ProtectedPacket : public ForwardErrorCorrection::SortablePacket { any reason to have ProtectedPacket in the public section? https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:139: RTC_DISALLOW_COPY_AND_ASSIGN(ForwardErrorCorrection); this line should always be the last in the class definiton. https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:143: static ForwardErrorCorrection CreateUlpfec(); ForwardErrorCorrection is not a small class to return it by value. May be return std::unique_ptr<ForwardErrorCorrection> instead. Or, since the only difference is header reader/writer, join those two interfaces into one (since they always have to match: it doesn't make sense to have UlpReader with FlexWriter) Make a public constructor ForwardErrorCorrection(std::unique_ptr<FecHeaderFormat>) and in the interface FecHeaderFormat have static std::unique_ptr<FecHeaderFormat> CreateUlpfec(); static std::unique_ptr<FecHeaderFormat> CreateFlexfec(); https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/producer_fec.h (right): https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/producer_fec.h:44: ProducerFec(); Isn't this reducing genearlization or ProducerFec can work with ulpfec only? https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc:101: std::list<test::fec::RawRtpPacket*> rtp_packets; consider add using ::webrtc::test::fec::RawRtpPacket in the header of this file. https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc:115: producer_.GetFecPacketsAsRed(test::fec::kRedPayloadType, does this constants need to be general? may be keep those constants in local test file (even if they are same across several test files). https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:43: template <typename S> Can you have more descriptional name for this type? e.g. template <typename FecAlgorithm> https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:908: template <typename T> template <typename S, typename T> https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:345: - video_->FecPacketOverhead() // FEC/ULP/RED overhead. Non-functional renames, adding/removing empty lines, fixing comments, other inline non-functional changes easier to review when they are in separate CL. It is easier to jump between files in CL when there are less of them changed. https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:31: enum { RedForFecHeaderLength = 1 }; constexpr size_t kRedForFecHeaderLength = 1; i.e. do not forget 'k' prefix, and change from pre c++11 enum hack to constexpr
Description was changed from
==========
Generalize FEC header reading/writing to support both ULPFEC and FlexFEC.
- 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.
- Generalize ForwardErrorCorrection::XorPackets so that it can be
used for both encoding and decoding. (In the previous implementation,
the XORing was implemented twice.)
- Make ForwardErrorCorrection moveable (but not copyable), thus removing
further unnecessary unique_ptr's.
- Generalize the tests in rtp_fec_test.cc to support ForwardErrorCorrection
with either ULPFEC or FlexFEC header reading/writing, using gTest
typed tests.
- Move InsertZeroColumns and CopyColumn to ::internal. These functions
operate directly on the packet masks, and are thus not directly
involved in the encoding/decoding operation. The ::internal namespace
is currently used for packet mask-related functions, but may be
renamed later on.
- Move ConstructMediaPackets to fec_test_helper.{h,cc}.
Minor changes:
- Rename GenerateFec -> EncodeFec in ForwardErrorCorrection. This naming
is more consistent with DecodeFec.
- Rename AttemptRecover -> AttemptRecovery in ForwardErrorCorrection.
- Wrap some of the tests in the webrtc namespace.
BUG=webrtc:5654
==========
to
==========
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.
- Generalize ForwardErrorCorrection::XorPackets so that it can be
used for both encoding and decoding. (In the previous implementation,
the XORing was implemented twice.)
- Make ForwardErrorCorrection moveable (but not copyable), thus removing
further unnecessary unique_ptr's.
- Rename AttemptRecover -> AttemptRecovery in ForwardErrorCorrection.
BUG=webrtc:5654
==========
Split up previous large CL in five semi-indenpendent ones. https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:1: /* On 2016/08/22 13:00:41, danilchap wrote: > make unittest filename match source file it is testing, i.e. rename this into > flexfec_header_reader_writer_unittest.cc This file tests the Ulpfec stuff too though. I could split it in two files, but then I would need a third file for the shared class FecPacketizationTest, or duplicate some code. What is the best solution here? (Also note that ForwardErrorCorrection is unit tested in rtp_fec_unittests.cc. I plan to rename this file when I rename ForwardErrorCorrection!) https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:36: : rtp_data_callback_(), On 2016/08/22 13:00:41, danilchap wrote: > it's cleaner to omit default constructions of members > (same for generator_) Done, see https://codereview.webrtc.org/2267393002/diff/1/webrtc/modules/rtp_rtcp/sourc.... https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/fec_test_helper.h (right): https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/fec_test_helper.h:63: class MediaPacketGenerator { On 2016/08/22 13:00:41, danilchap wrote: > Can you comment why do you need 2 packet generators? I think you mean why both FrameGenerator and MediaPacketGenerator are needed? MediaPacketGenerator is just moved (this is now done in this CL: https://codereview.webrtc.org/2267393002/) from rtp_fec_unittest.cc, which is the only place it was used before. Now it is used by the fec_header_reader_writer_unittest.cc. FrameGenerator is used by the FecReceiver/ProducerFec unit tests. I agree that it seems that these are sort of duplicated efforts, so they could probably be merged. That might be the point of another CL though. https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.h (right): https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/flexfec_header_reader_writer.h:1: /* On 2016/08/22 13:00:41, danilchap wrote: > Generalizing FEC header reading/writing can be one CL, > Adding FlexFec - another one. Good idea! I'll split it up so this CL is about the generalization part, and another CL is the FlexFEC parts. See coming emails ;) https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:108: class ProtectedPacket : public ForwardErrorCorrection::SortablePacket { On 2016/08/22 13:00:41, danilchap wrote: > any reason to have ProtectedPacket in the public section? I think so, since it is used in the public interface of ReceivedFecPacket :( Depending on time, it is my intention to refactor these packet classes. This might be done in conjunction with the switch to RtpPacket's. https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:139: RTC_DISALLOW_COPY_AND_ASSIGN(ForwardErrorCorrection); On 2016/08/22 13:00:41, danilchap wrote: > this line should always be the last in the class definiton. Done, see https://codereview.webrtc.org/2260803002/diff/20001/webrtc/modules/rtp_rtcp/s.... https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:143: static ForwardErrorCorrection CreateUlpfec(); On 2016/08/22 13:00:41, danilchap wrote: > ForwardErrorCorrection is not a small class to return it by value. Right, but since ForwardErrorCorrection is neither copyable nor assignable, this function call will only ever result in moves, which should be OK? I initially considered returning a unique_ptr, but I didn't see the point of the additional indirection since this class is not polymorphic. > May be return std::unique_ptr<ForwardErrorCorrection> instead. > Or, since the only difference is header reader/writer, join those two interfaces > into one (since they always have to match: it doesn't make sense to have > UlpReader with FlexWriter) > Make a public constructor > ForwardErrorCorrection(std::unique_ptr<FecHeaderFormat>) > and in the interface FecHeaderFormat have > static std::unique_ptr<FecHeaderFormat> CreateUlpfec(); > static std::unique_ptr<FecHeaderFormat> CreateFlexfec(); I agree that they always have to match, but later on I plan to split ForwardErrorCorrection into two classes: ParityErasureCodeEncoder (which has a FecHeaderWriter) and ParityErasureCodeDecoder (which has a FecHeaderReader). This will make the structure of the code easier to follow. It will be up to higher layers to ensure that the correct decoder is paired with the current encoder. https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/producer_fec.h (right): https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/producer_fec.h:44: ProducerFec(); On 2016/08/22 13:00:41, danilchap wrote: > Isn't this reducing genearlization or ProducerFec can work with ulpfec only? Correct, ProducerFec will only support Ulpfec. (In fact, that class will later be renamed UlpfecSender). The reason for not generalizing the {Ulp,Flex}fecSender and {Ulp,Flex}fecReceiver is that they do different things. The Ulpfec classes will for example deal with RED packaging, which the Flexfec classes will not. https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc:101: std::list<test::fec::RawRtpPacket*> rtp_packets; On 2016/08/22 13:00:41, danilchap wrote: > consider add using ::webrtc::test::fec::RawRtpPacket in the header of this file. Done, see https://codereview.webrtc.org/2267393002/diff/1/webrtc/modules/rtp_rtcp/sourc.... https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc:115: producer_.GetFecPacketsAsRed(test::fec::kRedPayloadType, On 2016/08/22 13:00:41, danilchap wrote: > does this constants need to be general? > may be keep those constants in local test file (even if they are same across > several test files). Done. https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:43: template <typename S> On 2016/08/22 13:00:41, danilchap wrote: > Can you have more descriptional name for this type? > e.g. template <typename FecAlgorithm> Done, see https://codereview.webrtc.org/2267393002/diff/1/webrtc/modules/rtp_rtcp/sourc.... https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:908: template <typename T> On 2016/08/22 13:00:41, danilchap wrote: > template <typename S, typename T> That's what I though first too, but it doesn't compile :/ error: too many template parameters in template redeclaration template <typename ForwardErrorCorrectionType, typename PacketListType> See https://codereview.webrtc.org/2267393002/diff/1/webrtc/modules/rtp_rtcp/sourc.... https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:345: - video_->FecPacketOverhead() // FEC/ULP/RED overhead. On 2016/08/22 13:00:41, danilchap wrote: > Non-functional renames, adding/removing empty lines, fixing comments, other > inline non-functional changes easier to review when they are in separate CL. > > It is easier to jump between files in CL when there are less of them changed. Done, see https://codereview.webrtc.org/2275443002/diff/1/webrtc/modules/rtp_rtcp/sourc.... https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:31: enum { RedForFecHeaderLength = 1 }; On 2016/08/22 13:00:41, danilchap wrote: > constexpr size_t kRedForFecHeaderLength = 1; > i.e. do not forget 'k' prefix, and change from pre c++11 enum hack to constexpr Done, see https://codereview.webrtc.org/2275443002/diff/1/webrtc/modules/rtp_rtcp/sourc....
https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2260803002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:143: static ForwardErrorCorrection CreateUlpfec(); > On 2016/08/22 13:00:41, danilchap wrote: > > May be return std::unique_ptr<ForwardErrorCorrection> instead. Reimplemented to use std::unique_ptr's.
few more suggestions, still not a full review for this part. https://codereview.webrtc.org/2260803002/diff/120001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (left): https://codereview.webrtc.org/2260803002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:82: ReceivedPacket(); what is motiviation for removing constructor/destructor? this struct has non-POD data member, so while consturctor/destructor are default, they are not trivial. https://codereview.webrtc.org/2260803002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:97: RecoveredPacket(); same here. https://codereview.webrtc.org/2260803002/diff/120001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2260803002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:127: // SSRC -> [SN base, packet mask offset, packet mask size (bytes)] probably clearer instead of two comments have a helper struct: struct PacketMask { uint16_t seq_no_base; size_t offset; size_t size_bytes; }; std::map<uint32_t, PacketMask> packet_mask_info; btw, is this map part of a packet, is it used in ulpfec or would make sense only in flexfec? Since this class got new members and now used not just inside ForwardErrorCorrection, but in other classes (FecReader/Writer) may it is time to resolve Stefan's TODO and move this into own file and proper class (in own CL). https://codereview.webrtc.org/2260803002/diff/120001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:51: class UlpfecForwardErrorCorrection : public ForwardErrorCorrection { using both fec and ForwardErrorCorrection in same name feels redundant. Few alternatives suggestions: UlpForwardErrorCorrection UlpFec ForwardErrorCorrectionUlp TestUlpFec https://codereview.webrtc.org/2260803002/diff/120001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc (right): https://codereview.webrtc.org/2260803002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc:34: 2 + webrtc::kUlpfecPacketMaskSizeLBitSet; do you need namespace here? https://codereview.webrtc.org/2260803002/diff/120001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.h (right): https://codereview.webrtc.org/2260803002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.h:41: ~UlpfecHeaderReader(); add override since base destructor is virtual
Patchset #8 (id:140001) has been deleted
Thanks for the feedback! See answers below. https://codereview.webrtc.org/2260803002/diff/120001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (left): https://codereview.webrtc.org/2260803002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:82: ReceivedPacket(); On 2016/08/24 10:42:40, danilchap wrote: > what is motiviation for removing constructor/destructor? > this struct has non-POD data member, so while consturctor/destructor are > default, they are not trivial. Re-adding non-inlined default constructor, as per discussion offline. Also adding explicit non-inline default constructors to other classes which have a rtc::scoped_refptr as a member. (I assume from your comment that this class is non-POD?) https://codereview.webrtc.org/2260803002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:97: RecoveredPacket(); On 2016/08/24 10:42:40, danilchap wrote: > same here. Done. https://codereview.webrtc.org/2260803002/diff/120001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2260803002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:127: // SSRC -> [SN base, packet mask offset, packet mask size (bytes)] On 2016/08/24 10:42:40, danilchap wrote: > probably clearer instead of two comments have a helper struct: > struct PacketMask { > uint16_t seq_no_base; > size_t offset; > size_t size_bytes; > }; > std::map<uint32_t, PacketMask> packet_mask_info; Good idea, done! > btw, is this map part of a packet, is it used in ulpfec or would make sense only > in flexfec? The generality of having multiple streams protected (thus the map) only applies to FlexFEC. For a single stream, the three fields (seq_num_base, offset, size) applies to ULPFEC too. > Since this class got new members and now used not just inside > ForwardErrorCorrection, but in other classes (FecReader/Writer) may it is time > to resolve Stefan's TODO and move this into own file and proper class (in own > CL). Yep! It's on the to do list. https://codereview.webrtc.org/2260803002/diff/120001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:51: class UlpfecForwardErrorCorrection : public ForwardErrorCorrection { On 2016/08/24 10:42:40, danilchap wrote: > using both fec and ForwardErrorCorrection in same name feels redundant. > Few alternatives suggestions: > UlpForwardErrorCorrection > UlpFec > ForwardErrorCorrectionUlp > TestUlpFec In one sense, the naming is redundant, but in another it is not. Anyway, this naming is temporary. ForwardErrorCorrection will be renamed ParityErasureCode, and then this class will be called UlpfecParityErasureCode. "Ulpfec" then means that this does header formatting per RFC 5109, and "ParityErasureCode" means that this is a simple XOR-based rectangular code. "ForwardErrorCorrection" is a very undescriptive name for this class to start with. I decided to do the renaming at the end of the FlexFEC process, after I'm sure that I'm giving things their right names. https://codereview.webrtc.org/2260803002/diff/120001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc (right): https://codereview.webrtc.org/2260803002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc:34: 2 + webrtc::kUlpfecPacketMaskSizeLBitSet; On 2016/08/24 10:42:40, danilchap wrote: > do you need namespace here? Nope, that was from before. Thanks! https://codereview.webrtc.org/2260803002/diff/120001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.h (right): https://codereview.webrtc.org/2260803002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.h:41: ~UlpfecHeaderReader(); On 2016/08/24 10:42:40, danilchap wrote: > add override since base destructor is virtual Done.
few more comments https://codereview.webrtc.org/2260803002/diff/120001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (left): https://codereview.webrtc.org/2260803002/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:82: ReceivedPacket(); On 2016/08/24 11:34:23, brandtr wrote: > On 2016/08/24 10:42:40, danilchap wrote: > > what is motiviation for removing constructor/destructor? > > this struct has non-POD data member, so while consturctor/destructor are > > default, they are not trivial. > > Re-adding non-inlined default constructor, as per discussion offline. > > Also adding explicit non-inline default constructors to other classes which have > a rtc::scoped_refptr as a member. (I assume from your comment that this class is > non-POD?) yes. POD = Plain Old Data (http://en.cppreference.com/w/cpp/concept/PODType) https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:49: packet_mask_() {} needed? https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:52: void Bit15Clear(); May be have just two functions instead: void ClearBit(size_t index) { packet_mask_[index / 8] &= ~(1 << (7 - index % 8)); } void SetBit(size_t index) { packet_mask_[index / 8] |= (1 << (7 - index % 8)); } Or one: void SetBit(size_t bit_index, bool set) { size_t byte_index = bit_index / 8; uint8_t bit = (1 << (7 - bit_index % 8)); if (set) packet_mask_[byte_index] |= bit; else packet_mask_[byte_index] &= ~bit; } Two looks cleaner. https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:60: void GenerateUlpfecPacketMask(size_t packet_mask_size); Name of this function hints it is ulp specific. May be move it into UlpfecPacketizationTest? Or rename to GenerateRandomPacketMask(size_t packet_mask_size); https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:62: void ReadWriteAndVerifyHeaders(size_t ulpfec_packet_mask_size, Parameter name hints it is ulp specific. https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:78: uint8_t packet_mask_[14]; // Only filled with ULPFEC packet mask (6 bytes), may be add constant kMaxPacketMaskSize instead of 14 since that constants also used in one of the tests. https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:95: ForwardErrorCorrection::ReceivedFecPacket* fec_packet) { override https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:99: size_t ExpectedPacketMaskOffset() { return 12u; } add override (otherwise it raise question why you need a function instead of a constant) https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:103: TEST_F(UlpfecPacketizationTest, HeaderSizeTest) { using 'Test' in Test name feels unnecessary (there is already word 'Test' in the class name). But test name could be more descriptive: in addition to what it is about (HeaderSize), what is checked (CalculatedCorrectly) it also may hint setup, when it is calcuated: TEST_F(UlpfecPacketizationTest, HeaderWriterCalculatesMinPacketSize) or is it HeaderWriterCalcuatesShortMaskHeaderSize? https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:103: TEST_F(UlpfecPacketizationTest, HeaderSizeTest) { put tests below helper functions. It is also ok to 'inline' functions in the _unittest.cc files. (They will appear in one .cc and thus in single .o anyway) https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:106: EXPECT_EQ(14u, fec_header_writer_->FecHeaderSize( why 14 is a correct value? May be name the constant. https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:109: Theese are two separate tests, each with own setup, own run, own expectation check. Split them. https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:113: 18u, fec_header_writer_->FecHeaderSize( may be kUlpfecMaxHeaderSize insteaf of 18? https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:118: TEST_F(UlpfecPacketizationTest, ReaderWriterTest) { Same, omit 'Test' in test name, but put more description of the tested scenario: 'WriteAndReadWithShortMask', 'ReadMatchWriteWithShortMask'; https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:177: new ForwardErrorCorrection::Packet(); // deleted by scoped_refptr in Remove the comment and make the type scoped_refptr instead of the raw pointer. Perfomance decrease would be negligible compare to readability increase.
Patchset #10 (id:200001) has been deleted
Feedback response.
Patchset #10 (id:220001) has been deleted
https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:49: packet_mask_() {} On 2016/08/24 16:41:27, danilchap wrote: > needed? Nope, gone. https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:52: void Bit15Clear(); On 2016/08/24 16:41:26, danilchap wrote: > May be have just two functions instead: > void ClearBit(size_t index) { > packet_mask_[index / 8] &= ~(1 << (7 - index % 8)); > } > void SetBit(size_t index) { > packet_mask_[index / 8] |= (1 << (7 - index % 8)); > } > Or one: > void SetBit(size_t bit_index, bool set) { > size_t byte_index = bit_index / 8; > uint8_t bit = (1 << (7 - bit_index % 8)); > if (set) > packet_mask_[byte_index] |= bit; > else > packet_mask_[byte_index] &= ~bit; > } > Two looks cleaner. Nice, thanks! https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:60: void GenerateUlpfecPacketMask(size_t packet_mask_size); On 2016/08/24 16:41:27, danilchap wrote: > Name of this function hints it is ulp specific. May be move it into > UlpfecPacketizationTest? Or rename to GenerateRandomPacketMask(size_t > packet_mask_size); It sounds like it is Ulpfec specific, but it's not. The parity erasure code currently always uses Ulpfec packet masks, even though the FEC header formatting might be done using Flexfec. So this function simply generates a packet mask according to the 'Ulpfec format', which may then be used by either Ulpfec or Flexfec. Therefore, it belongs in the general class. Renamed to 'GenerateUlpfecFormattedPacketMask' to reduce confusion. https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:62: void ReadWriteAndVerifyHeaders(size_t ulpfec_packet_mask_size, On 2016/08/24 16:41:27, danilchap wrote: > Parameter name hints it is ulp specific. Same thing here. ulpfec_packet_mask_size corresponds to the size of the Ulpfec packet mask, which is not the same as the 'Flexfecized' version of the packet mask. Renamed to 'ulpfec_formatted_packet_mask_size'. https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:78: uint8_t packet_mask_[14]; // Only filled with ULPFEC packet mask (6 bytes), On 2016/08/24 16:41:26, danilchap wrote: > may be add constant kMaxPacketMaskSize instead of 14 since that constants also > used in one of the tests. Done. https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:95: ForwardErrorCorrection::ReceivedFecPacket* fec_packet) { On 2016/08/24 16:41:26, danilchap wrote: > override Hm, seems that the compiler does not warn about missing overrides when the function is inlined? https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:99: size_t ExpectedPacketMaskOffset() { return 12u; } On 2016/08/24 16:41:26, danilchap wrote: > add override > (otherwise it raise question why you need a function instead of a constant) Done. https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:103: TEST_F(UlpfecPacketizationTest, HeaderSizeTest) { On 2016/08/24 16:41:27, danilchap wrote: > using 'Test' in Test name feels unnecessary (there is already word 'Test' in the > class name). > But test name could be more descriptive: > in addition to what it is about (HeaderSize), what is checked > (CalculatedCorrectly) > it also may hint setup, when it is calcuated: > TEST_F(UlpfecPacketizationTest, HeaderWriterCalculatesMinPacketSize) > or is it HeaderWriterCalcuatesShortMaskHeaderSize? Good point! Renamed. https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:103: TEST_F(UlpfecPacketizationTest, HeaderSizeTest) { On 2016/08/24 16:41:27, danilchap wrote: > put tests below helper functions. > It is also ok to 'inline' functions in the _unittest.cc files. > (They will appear in one .cc and thus in single .o anyway) Moved helper functions. I don't like the clutter of the inlined functions, so I keep the definitions outside the class declaration. https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:106: EXPECT_EQ(14u, fec_header_writer_->FecHeaderSize( On 2016/08/24 16:41:26, danilchap wrote: > why 14 is a correct value? May be name the constant. Done. https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:109: On 2016/08/24 16:41:26, danilchap wrote: > Theese are two separate tests, > each with own setup, own run, own expectation check. > Split them. Done. https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:113: 18u, fec_header_writer_->FecHeaderSize( On 2016/08/24 16:41:26, danilchap wrote: > may be kUlpfecMaxHeaderSize insteaf of 18? Done. https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:118: TEST_F(UlpfecPacketizationTest, ReaderWriterTest) { On 2016/08/24 16:41:27, danilchap wrote: > Same, omit 'Test' in test name, but put more description of the tested scenario: > 'WriteAndReadWithShortMask', 'ReadMatchWriteWithShortMask'; Done. https://codereview.webrtc.org/2260803002/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:177: new ForwardErrorCorrection::Packet(); // deleted by scoped_refptr in On 2016/08/24 16:41:26, danilchap wrote: > Remove the comment and make the type scoped_refptr instead of the raw pointer. > Perfomance decrease would be negligible compare to readability increase. Good idea!! This isn't the way it's done in the current FEC code, but that definitely makes it more readable. Of course, these packet classes will be refactored later on...
some more comments https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:35: // These are pulled in from forward_error_correction_internal.h Might be better to remove those constants or forward: constexpr size_t kUlpFecPacketMaskSizeLBitClear = internal::kUlpFecPacketMaskSizeLBitClear; // 2. or may be forward with renaming how they used locally. constexpr size_t kShortMask = internal::kUlpFecPacketMaskSizeLBitClear; // 2. constexpr size_t kLongMask = internal::kUlpFecPacketMaskSizeLBitSet; // 6. https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:39: constexpr size_t kFlexfecPacketMaskSize2K = 14; what does 2K means? is it kFlexfexMaxPacketMaskSize? https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:44: using Packet = ForwardErrorCorrection::Packet; prefer full names: using Packet = ::webrtc::ForwardErrorCorrection::Packet; https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:92: class UlpfecPacketizationTest : public FecPacketizationTest { any reason why this is in same file instead of own test file? ulpfec_header_reader_writer_unittests.cc Might also help to rename it to UlpfecHeaderReaderWriterTest to stress what classes are under test. https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:118: RTC_DCHECK_LE(packet_mask_size, 6u); kUlpfecMaxPacketMaskSize instead of 6 https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:119: memset(packet_mask_, 0u, 14); kFlexfecPacketMaskSize2K instead of 14 https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:134: media_packet_generator_.GetFecSeqNum() - kNumMediaPackets; it is not obvious why it is correct. may be instead: uint16_t media_seq_num = kMyTestMediaSeqNum; (or random_.Rand<uint16_t>();) PacketList media_packets = media_packet_generator_.ConstructMediaPackets(kNumMediaPackets, media_seq_num); https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:77: received_fec_packets_() {} remove received_fec_packets_ initialize packet_mask_size_ https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:136: // Unpack packet masks. Since the precomputed packet masks are tailored while it might be interesting to know, code already generalized so that it doesn't rely on just two masks. Probably better to remove comment since it no longer describes this particlar function. https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:578: bool ForwardErrorCorrection::RecoverPacket(ReceivedFecPacket* fec_packet, why fec_packet no longer const? https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:246: // This constructor is used by the unit tests. CreateUlpfec uses it too. Adjust the comment. (or remove it - there is only one constructor in this class) https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction_internal.cc (right): https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction_internal.cc:399: RTC_DCHECK_LE(seq_num_spread, 8 * kUlpfecPacketMaskSizeLBitSet); that is kUlpfecMaxMediaPackets? may be add semipublic constants constexpr size_t kUlpfecMaxPacketMaskSize = std::max(kUlpFecPacketMaskSizeLBitSet, kUlpFecPacketMaskSizeLBitClear); constexpr size_t kUlpfecMaxMediaPackets = 8 * kUlpfecPacketMaskSizeMax; // 48. and make kUlpFecPacketMaskSizeLBitSet, kUlpFecPacketMaskSizeLBitClear more internal. (does normal reader need to know what LBit is?) https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction_internal.cc:402: } else { do not use else after return: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction_internal.h (right): https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction_internal.h:21: constexpr size_t kUlpfecMaxMediaPackets = 48; Probably put these constants into internal too. since they are in internal file, they shouldn't be used too much outside. https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/producer_fec.cc (right): https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/producer_fec.cc:152: // Generic FEC can only protect up to kMaxMediaPackets packets. kUlpfecMaxMediaPackets packets. https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/test/testFec/test_packet_masks_metrics.cc (right): https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/test/testFec/test_packet_masks_metrics.cc:738: const int mask_bytes_fec_packet = (num_media_packets > 16) may be use internal::PacketMaskSize(num_media_packets) here instead.
https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... 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 17:49:15, danilchap wrote: > Might be better to remove those constants or forward: > constexpr size_t kUlpFecPacketMaskSizeLBitClear = > internal::kUlpFecPacketMaskSizeLBitClear; // 2. > or may be forward with renaming how they used locally. > constexpr size_t kShortMask = internal::kUlpFecPacketMaskSizeLBitClear; // 2. > constexpr size_t kLongMask = internal::kUlpFecPacketMaskSizeLBitSet; // 6. Removed them. https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:39: constexpr size_t kFlexfecPacketMaskSize2K = 14; On 2016/08/25 17:49:15, danilchap wrote: > what does 2K means? > is it kFlexfexMaxPacketMaskSize? Yep. Added and renamed these in the next CL. https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:44: using Packet = ForwardErrorCorrection::Packet; On 2016/08/25 17:49:15, danilchap wrote: > prefer full names: > using Packet = ::webrtc::ForwardErrorCorrection::Packet; Done. https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:92: class UlpfecPacketizationTest : public FecPacketizationTest { On 2016/08/25 17:49:15, danilchap wrote: > any reason why this is in same file instead of own test file? > ulpfec_header_reader_writer_unittests.cc > > Might also help to rename it to UlpfecHeaderReaderWriterTest to stress what > classes are under test. Move and renamed. https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:118: RTC_DCHECK_LE(packet_mask_size, 6u); On 2016/08/25 17:49:15, danilchap wrote: > kUlpfecMaxPacketMaskSize instead of 6 Good catch, thanks. https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:119: memset(packet_mask_, 0u, 14); On 2016/08/25 17:49:15, danilchap wrote: > kFlexfecPacketMaskSize2K instead of 14 Done. https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_header_reader_writer_unittest.cc:134: media_packet_generator_.GetFecSeqNum() - kNumMediaPackets; On 2016/08/25 17:49:15, danilchap wrote: > it is not obvious why it is correct. > may be instead: > uint16_t media_seq_num = kMyTestMediaSeqNum; (or random_.Rand<uint16_t>();) > PacketList media_packets = > media_packet_generator_.ConstructMediaPackets(kNumMediaPackets, > media_seq_num); Done. (See next patchset.) https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:77: received_fec_packets_() {} On 2016/08/25 17:49:15, danilchap wrote: > remove received_fec_packets_ > initialize packet_mask_size_ Done. https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:136: // Unpack packet masks. Since the precomputed packet masks are tailored On 2016/08/25 17:49:15, danilchap wrote: > while it might be interesting to know, code already generalized so that it > doesn't rely on just two masks. > Probably better to remove comment since it no longer describes this particlar > function. The code still relies on the two versions of ULPFEC formatted packet masks, (which are then "Flexfecized" by the FlexfecHeaderWriter), but I removed the comment because it didn't add a lot of value. https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:578: bool ForwardErrorCorrection::RecoverPacket(ReceivedFecPacket* fec_packet, On 2016/08/25 17:49:15, danilchap wrote: > why fec_packet no longer const? No idea. I've added constness in some places in the new patch. Also added staticness to some (former) member functions. https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:246: // This constructor is used by the unit tests. On 2016/08/25 17:49:15, danilchap wrote: > CreateUlpfec uses it too. Adjust the comment. > (or remove it - there is only one constructor in this class) I'll just remove it. The comment was supposed to describe why the constructor was protected, rather than private. https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction_internal.cc (right): https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction_internal.cc:399: RTC_DCHECK_LE(seq_num_spread, 8 * kUlpfecPacketMaskSizeLBitSet); On 2016/08/25 17:49:15, danilchap wrote: > that is kUlpfecMaxMediaPackets? > > may be add semipublic constants > constexpr size_t kUlpfecMaxPacketMaskSize = > std::max(kUlpFecPacketMaskSizeLBitSet, kUlpFecPacketMaskSizeLBitClear); > constexpr size_t kUlpfecMaxMediaPackets = 8 * kUlpfecPacketMaskSizeMax; // 48. > and make > kUlpFecPacketMaskSizeLBitSet, kUlpFecPacketMaskSizeLBitClear more internal. > (does normal reader need to know what LBit is?) This is a really nice idea, but seems that std::max is only constexpr in C++14... :( http://en.cppreference.com/w/cpp/algorithm/min When it comes to reading the packet mask code, I think the normal reader will need to check the header format, and thus know what the L bit is. https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction_internal.cc:402: } else { On 2016/08/25 17:49:15, danilchap wrote: > do not use else after return: > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > Done. https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction_internal.h (right): https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction_internal.h:21: constexpr size_t kUlpfecMaxMediaPackets = 48; On 2016/08/25 17:49:15, danilchap wrote: > Probably put these constants into internal too. > since they are in internal file, they shouldn't be used too much outside. I will get rid of the internal namespace, since the packet masks are now used by both ForwardErrorCorrection (to be renamed ParityErasureCode{Encoder,Decoder} as well as {Flex,Ulp}fecHeader{Reader,Writer}. So I'll leave these constants outside the internal namespace. https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/producer_fec.cc (right): https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/producer_fec.cc:152: // Generic FEC can only protect up to kMaxMediaPackets packets. On 2016/08/25 17:49:15, danilchap wrote: > kUlpfecMaxMediaPackets packets. Done. https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/test/testFec/test_packet_masks_metrics.cc (right): https://codereview.webrtc.org/2260803002/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/test/testFec/test_packet_masks_metrics.cc:738: const int mask_bytes_fec_packet = (num_media_packets > 16) On 2016/08/25 17:49:15, danilchap wrote: > may be use internal::PacketMaskSize(num_media_packets) here instead. Done. https://codereview.webrtc.org/2260803002/diff/260001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2260803002/diff/260001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:285: void InsertFecPacket(const RecoveredPacketList& recovered_packets, Changed signatures a bit here, to adhere better to style.
description mention no unique_ptr for ForwardErrorCorrection. That is not true any more. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:175: uint32_t pkt_mask_idx = i * packet_mask_size_; since you touch almost all lines in this function, change this type to size_t too. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:181: uint32_t media_pkt_idx = 0; size_t https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:190: ByteWriter<uint16_t>::WriteBigEndian(media_payload_length_network_order, this now used in if (first_protected_packet) only. May be move it here and WriteBigEndian directly into &fec_packet->data[2] https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:215: fec_packet_length - fec_header_size, fec_header_size, maybe media_payload_length instead of fec_packet_length - fec_header_size to make more consistent with if (first_protected_packet) block https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:242: uint16_t total_missing_seq_nums = this variable represent number of, not a sequence number, so make it size_t. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:243: static_cast<uint16_t>(last_seq_num - first_seq_num) - num_media_packets + this static_cast shouldn't be needed now: uint16_t - uint16_t = uint16_t. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:250: if (total_missing_seq_nums + num_media_packets > kUlpfecMaxMediaPackets) { probably fec_header_writer_->MaxMediaPackets() instead of kUlpfecMaxMediaPackets https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:269: if (new_bit_index == kUlpfecMaxMediaPackets) { ditto https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:568: RTC_DCHECK_LE(src_offset + payload_length, sizeof(src.data)); may be src.length/dst->length instead of sizeof to check you do not read/write beyound size instead of beyond capacity. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (left): https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:115: virtual ~ForwardErrorCorrection(); bring destructor back! https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:140: uint32_t rtp_ssrc; why rename it to rtp_ssrc from ssrc? do not think there are any other ssrcs in this module. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:300: // |payload_length| bytes are XORed. nope, in total header + |payload_length| bytes are XORed. May be split this function into two: XorHeader(const Packet& src, Packet* dst); XorBuffer/XorPayload(const uint8_t* src, size_t size, uint8_t* dst); https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:301: static void XorPackets(const Packet& src, There is logic when functions were declared in order 'Start', 'Xor', 'Finish' (since that normal order to call this functions) now you reorder them 'Xor', 'Start', 'Finish'. Why is it better? https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:340: uint8_t packet_masks_[kUlpfecMaxMediaPackets * kUlpfecPacketMaskSizeLBitSet]; kUlpfecMaxPacketMaskSize instead of kUlpfecPacketMaskSizeLBitSet https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:373: FecHeaderWriter(size_t max_media_packets, size_t max_fec_packets); This one miss an implementation. Is it in use? https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction_internal.h (right): https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction_internal.h:75: // Given a sequence number spread (i.e., the difference between the largest 'plus one' in the comment feels strange. may be instead of describing how to calculate it, describe what it represent e.g. number of packet with sequence number between smallest and largest. Or is it just number of sequence numbers between smallest and largest? Can also change the parameter name to 'num_protected_packets' or 'num_packets' https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc (right): https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc:39: size_t FecHeaderSizeImpl(size_t packet_mask_size) { Impl more often added to a class that implements an interface. May be call this function UlpfecHeaderSize https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc:99: void UlpfecHeaderWriter::FinalizeFecHeader( Maybe WriteFecHeader? it seems this function is simmetric to ReadFecHeader: it writes as much as that function reads. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc:100: const ForwardErrorCorrection::PacketList& media_packets, it seems you do not need full media packets list: only first packet is checked. (in flexfec too) May be pass just the 1st packet? https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:50: kTransportOverhead - swap -kTranposrtOverhead and -kRtpHeaderSize to match how this bytes are used in the packet: 1st transport headers, then rtp header, then ulpfec header. might name this constant as well (kMaxRtpPayloadSize?) by calculating it in the unnamed namespace. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:62: void WriteReadAndVerifyHeaders(size_t original_packet_mask_size, possible to split this function into WriteRead and Verify? to have clearer border-line betwen setup/run and expectation phases of a test. as well as clarify what is passed between this phases. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:102: ReceivedFecPacket received_fec_packet; there is no sending or receiving in this function. Only writing and reading. may be call variable just 'fec_packet' https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:105: Packet* sent_fec_packet = received_fec_packet.pkt.get(); is it raw_fec_packet? or unparsed_fec_packet https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:120: EXPECT_EQ(received_fec_packet.pkt->length - expected_fec_header_size, which variable is tested this line? received_fec_packet.protection_length. may be use sent_fec_packet instead of received_fec_packet.pkt to stress this variable is known, not under test. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:125: size_t offset = stream_packet_mask_info.offset; may be name local variables mask_offset/mask_size https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:131: EXPECT_EQ(0, memcmp(&sent_fec_packet->data[expected_fec_header_size], since sent_fec_packet = received_fec_packet.pkt (same refcpy) this test doesn't look like test anything. Can you clarify part I miss. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:137: GeneratePacketMask(kUlpfecPacketMaskSizeLBitClear); Does randomness of packet mask itself affects FecHeaderSize function in any way? Does it for Flexfec? https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/test/testFec/test_packet_masks_metrics.cc (right): https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/test/testFec/test_packet_masks_metrics.cc:734: new uint8_t[packet_mask_max * kUlpfecPacketMaskSizeLBitSet]; kUlpfecMaxPacketMaskSize probably make more sense when used as buffer size
Patchset #13 (id:300001) has been deleted
Patchset #13 (id:320001) has been deleted
Description was changed from ========== 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. - Generalize ForwardErrorCorrection::XorPackets so that it can be used for both encoding and decoding. (In the previous implementation, the XORing was implemented twice.) - Make ForwardErrorCorrection moveable (but not copyable), thus removing further unnecessary unique_ptr's. - Rename AttemptRecover -> AttemptRecovery in ForwardErrorCorrection. BUG=webrtc:5654 ========== to ========== 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 ==========
Thanks for feedback! Comments have been adressed. The FlexFEC header formatting CL has been rebased and updated to reflect the comments in this CL, as well. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/BUILD.gn File webrtc/modules/BUILD.gn (right): https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/BUILD.gn#... webrtc/modules/BUILD.gn:202: "audio_processing/intelligibility/intelligibility_enhancer_unittest.cc", Due to (accidental) rebase. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:175: uint32_t pkt_mask_idx = i * packet_mask_size_; On 2016/08/29 12:26:02, danilchap wrote: > since you touch almost all lines in this function, change this type to size_t > too. Done. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:181: uint32_t media_pkt_idx = 0; On 2016/08/29 12:26:02, danilchap wrote: > size_t Done. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:190: ByteWriter<uint16_t>::WriteBigEndian(media_payload_length_network_order, On 2016/08/29 12:26:02, danilchap wrote: > this now used in if (first_protected_packet) only. > May be move it here and WriteBigEndian directly into &fec_packet->data[2] Done. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:215: fec_packet_length - fec_header_size, fec_header_size, On 2016/08/29 12:26:02, danilchap wrote: > maybe media_payload_length instead of fec_packet_length - fec_header_size > to make more consistent with if (first_protected_packet) block Done. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:242: uint16_t total_missing_seq_nums = On 2016/08/29 12:26:02, danilchap wrote: > this variable represent number of, not a sequence number, so make it size_t. Done. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... 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/29 12:26:02, danilchap wrote: > this static_cast shouldn't be needed now: > uint16_t - uint16_t = uint16_t. Seems to still be needed for correct wrapping, after change of |total_missing_seq_nums| to size_t. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:250: if (total_missing_seq_nums + num_media_packets > kUlpfecMaxMediaPackets) { On 2016/08/29 12:26:02, danilchap wrote: > probably fec_header_writer_->MaxMediaPackets() instead of kUlpfecMaxMediaPackets Done. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:269: if (new_bit_index == kUlpfecMaxMediaPackets) { On 2016/08/29 12:26:02, danilchap wrote: > ditto Done. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:568: RTC_DCHECK_LE(src_offset + payload_length, sizeof(src.data)); On 2016/08/29 12:26:02, danilchap wrote: > may be src.length/dst->length instead of sizeof > to check you do not read/write beyound size instead of beyond capacity. This is a good idea, but I'll wait until a later CL to do that. (The CL when I split ForwardErrorCorrection into two classes.) Currently we rely on the fact that the packet payload is zeroed out in the elements between the size and the capacity. This helps for the case when we don't know the size of the recovered packet until recovery is finished. That is, we actually do a bit too many XORs here sometimes. The reason that I will wait fixing it is that the requirements might be a bit different between the sending and receiving sides XORing, and there is not point in writing a generalized operation here, if it will be dismantled in a later CL. The current DCHECKS were added just to ensure that we are never out of bounds. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (left): https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:115: virtual ~ForwardErrorCorrection(); On 2016/08/29 12:26:02, danilchap wrote: > bring destructor back! Done. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:140: uint32_t rtp_ssrc; On 2016/08/29 12:26:02, danilchap wrote: > why rename it to rtp_ssrc from ssrc? > do not think there are any other ssrcs in this module. I think that I considered adding the SSRC of the protected stream here as well, but that never happened. Will change back to 'ssrc'. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:300: // |payload_length| bytes are XORed. On 2016/08/29 12:26:02, danilchap wrote: > nope, in total header + |payload_length| bytes are XORed. > May be split this function into two: > XorHeader(const Packet& src, Packet* dst); > XorBuffer/XorPayload(const uint8_t* src, size_t size, uint8_t* dst); Done. I find it clearer to leave the offsets in the XorPayload function however, rather than doing pointer arithmetic at the calling site. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:301: static void XorPackets(const Packet& src, On 2016/08/29 12:26:03, danilchap wrote: > There is logic when functions were declared in order 'Start', 'Xor', 'Finish' > (since that normal order to call this functions) > now you reorder them 'Xor', 'Start', 'Finish'. Why is it better? Not better, but XorPackets is also used by the sending side. Moving back so it will be in consistent order for the receiving side. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:340: uint8_t packet_masks_[kUlpfecMaxMediaPackets * kUlpfecPacketMaskSizeLBitSet]; On 2016/08/29 12:26:03, danilchap wrote: > kUlpfecMaxPacketMaskSize instead of kUlpfecPacketMaskSizeLBitSet Done. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:373: FecHeaderWriter(size_t max_media_packets, size_t max_fec_packets); On 2016/08/29 12:26:03, danilchap wrote: > This one miss an implementation. Is it in use? No, that is a mistake. Thanks for finding! https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction_internal.h (right): https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction_internal.h:75: // Given a sequence number spread (i.e., the difference between the largest On 2016/08/29 12:26:03, danilchap wrote: > 'plus one' in the comment feels strange. > may be instead of describing how to calculate it, describe what it represent > e.g. number of packet with sequence number between smallest and largest. Or is > it just number of sequence numbers between smallest and largest? > Can also change the parameter name to 'num_protected_packets' or 'num_packets' Updated comment. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc (right): https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc:39: size_t FecHeaderSizeImpl(size_t packet_mask_size) { On 2016/08/29 12:26:03, danilchap wrote: > Impl more often added to a class that implements an interface. > May be call this function UlpfecHeaderSize Done. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc:99: void UlpfecHeaderWriter::FinalizeFecHeader( On 2016/08/29 12:26:03, danilchap wrote: > Maybe WriteFecHeader? > it seems this function is simmetric to ReadFecHeader: it writes as much as that > function reads. Yep, but there are other parts that write to the FEC header as well. (E.g., the XorHeaders function.) So this function just puts the finishing touches on the header, before the full FEC packet is finished. In the current implementation, this function must be called after XorHeaders is called, because some bits overlap between the bytes that are written there and here. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc:100: const ForwardErrorCorrection::PacketList& media_packets, On 2016/08/29 12:26:03, danilchap wrote: > it seems you do not need full media packets list: > only first packet is checked. (in flexfec too) > May be pass just the 1st packet? Done. I passed the whole list for future possible generalizations in FlexFEC, but I don't think that is necessary actually. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:50: kTransportOverhead - On 2016/08/29 12:26:03, danilchap wrote: > swap -kTranposrtOverhead and -kRtpHeaderSize > to match how this bytes are used in the packet: > 1st transport headers, then rtp header, then ulpfec header. > > might name this constant as well (kMaxRtpPayloadSize?) by calculating it in the > unnamed namespace. Done. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:62: void WriteReadAndVerifyHeaders(size_t original_packet_mask_size, On 2016/08/29 12:26:03, danilchap wrote: > possible to split this function into WriteRead and Verify? > to have clearer border-line betwen setup/run and expectation phases of a test. > as well as clarify what is passed between this phases. Done. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:102: ReceivedFecPacket received_fec_packet; On 2016/08/29 12:26:03, danilchap wrote: > there is no sending or receiving in this function. Only writing and reading. > may be call variable just 'fec_packet' Renamed. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:105: Packet* sent_fec_packet = received_fec_packet.pkt.get(); On 2016/08/29 12:26:03, danilchap wrote: > is it raw_fec_packet? or unparsed_fec_packet Renamed. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:120: EXPECT_EQ(received_fec_packet.pkt->length - expected_fec_header_size, On 2016/08/29 12:26:03, danilchap wrote: > which variable is tested this line? > received_fec_packet.protection_length. > may be use sent_fec_packet instead of received_fec_packet.pkt to stress this > variable is known, not under test. Makes sense! protection_length is the variable tested here. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:125: size_t offset = stream_packet_mask_info.offset; On 2016/08/29 12:26:03, danilchap wrote: > may be name local variables mask_offset/mask_size Done. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:131: EXPECT_EQ(0, memcmp(&sent_fec_packet->data[expected_fec_header_size], On 2016/08/29 12:26:03, danilchap wrote: > since sent_fec_packet = received_fec_packet.pkt (same refcpy) this test doesn't > look like test anything. > Can you clarify part I miss. Pretty glaring mistake here! I've changed so I'm making a copy. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:137: GeneratePacketMask(kUlpfecPacketMaskSizeLBitClear); On 2016/08/29 12:26:03, danilchap wrote: > Does randomness of packet mask itself affects FecHeaderSize function in any way? > Does it for Flexfec? For Ulpfec, the contents of the packet mask does not affect the call to FecHeaderSize. For Flexfec, the contents do affect the call. This is controlled in that unit test however, and not left up to chance. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/test/testFec/test_packet_masks_metrics.cc (right): https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/test/testFec/test_packet_masks_metrics.cc:734: new uint8_t[packet_mask_max * kUlpfecPacketMaskSizeLBitSet]; On 2016/08/29 12:26:03, danilchap wrote: > kUlpfecMaxPacketMaskSize probably make more sense when used as buffer size Done.
https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... 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, brandtr wrote: > On 2016/08/29 12:26:02, danilchap wrote: > > this static_cast shouldn't be needed now: > > uint16_t - uint16_t = uint16_t. > > Seems to still be needed for correct wrapping, after change of > |total_missing_seq_nums| to size_t. Yep, I was wrong, better leave this static_cast. Not sure why it is needed from compiler point of view. So to avoid confusing humans better to leave explicit cast to stress wrapping support. https://codereview.webrtc.org/2260803002/diff/360001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc (right): https://codereview.webrtc.org/2260803002/diff/360001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc:64: fec_packet->packet_mask_infos[fec_packet->ssrc] = should protected_ssrc be used as the key here? (sure with protected_ssrc assigned to fec_packet->ssrc above this line) https://codereview.webrtc.org/2260803002/diff/360001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/360001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:57: void ClearBit(size_t index); Will these two helpers be used for ulpfec? https://codereview.webrtc.org/2260803002/diff/360001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:66: void VerifyHeaders(std::pair<std::unique_ptr<Packet>, to follow EXPECT macros style might be better to put expected values first, values under test - last. https://codereview.webrtc.org/2260803002/diff/360001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:76: PacketList* media_packets_; looks unused https://codereview.webrtc.org/2260803002/diff/360001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:107: packet_to_write->length = media_packets.front()->length; looks like length and seq_no is all your need from media_packets. May be do not generate media_packets, but add kMediaPacketLength instead. https://codereview.webrtc.org/2260803002/diff/360001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:111: const uint16_t seq_num_base = ForwardErrorCorrection::ParseSequenceNumber( is it kMediaStartSeqNum? https://codereview.webrtc.org/2260803002/diff/360001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:113: writer_.FinalizeFecHeader(seq_num_base, packet_mask_, in another comment you wrote that XorHeaders should be called before calling FinalizeFecHeader, but in the tests you do not. Is it easy to adjust tests so that they can serve as a documentation how to use writer_? https://codereview.webrtc.org/2260803002/diff/360001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:130: std::pair<std::unique_ptr<Packet>, std::unique_ptr<ReceivedFecPacket>> probably cleaner to take two parameters instead of one pair https://codereview.webrtc.org/2260803002/diff/360001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:150: EXPECT_EQ(0, memcmp(&packet_to_write->data[expected_fec_header_size], What functionality of HeaderReaderWriter is tested by this expectation? As an almost last step of WriteAndReadHeaders you do a memcpy from packet_to_write to packet_to_read. here you check that memory is equal. https://codereview.webrtc.org/2260803002/diff/360001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:171: VerifyHeaders(WriteAndReadHeaders(kUlpfecPacketMaskSizeLBitClear), for clearer separation of run and verify phases it might be better to avoid chaining, and, may be, split a bit more: auto packet_to_write = WriteHeaders(original_packet_mask_size) auto packet_to_read = ReadHeaders(packet_to_write->data, packet_to_write->length); VerifyFecHeadersAreSame(kExtraExpectationValues, *packet_to_write, *packet_to_read);
Patchset #15 (id:380001) has been deleted
Addressed comments. https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2260803002/diff/280001/webrtc/modules/rtp_rtcp/... 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 12:29:01, danilchap wrote: > On 2016/08/30 07:12:40, brandtr wrote: > > On 2016/08/29 12:26:02, danilchap wrote: > > > this static_cast shouldn't be needed now: > > > uint16_t - uint16_t = uint16_t. > > > > Seems to still be needed for correct wrapping, after change of > > |total_missing_seq_nums| to size_t. > > Yep, I was wrong, better leave this static_cast. > Not sure why it is needed from compiler point of view. > So to avoid confusing humans better to leave explicit cast to stress wrapping > support. I guess it has something to do with promotion logic during arithmetic? It compiles fine without the cast, but then some of the tests that check wrapping behaviour fail. https://codereview.webrtc.org/2260803002/diff/360001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc (right): https://codereview.webrtc.org/2260803002/diff/360001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc:64: fec_packet->packet_mask_infos[fec_packet->ssrc] = On 2016/08/30 12:29:01, danilchap wrote: > should protected_ssrc be used as the key here? > (sure with protected_ssrc assigned to fec_packet->ssrc above this line) Yep, that's a bit cleaner. https://codereview.webrtc.org/2260803002/diff/360001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/360001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:57: void ClearBit(size_t index); On 2016/08/30 12:29:01, danilchap wrote: > Will these two helpers be used for ulpfec? Nope, removed! https://codereview.webrtc.org/2260803002/diff/360001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:66: void VerifyHeaders(std::pair<std::unique_ptr<Packet>, On 2016/08/30 12:29:01, danilchap wrote: > to follow EXPECT macros style might be better to put expected values first, > values under test - last. Done. https://codereview.webrtc.org/2260803002/diff/360001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:76: PacketList* media_packets_; On 2016/08/30 12:29:01, danilchap wrote: > looks unused Removed. https://codereview.webrtc.org/2260803002/diff/360001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:107: packet_to_write->length = media_packets.front()->length; On 2016/08/30 12:29:01, danilchap wrote: > looks like length and seq_no is all your need from media_packets. > May be do not generate media_packets, but add kMediaPacketLength instead. Done. https://codereview.webrtc.org/2260803002/diff/360001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:111: const uint16_t seq_num_base = ForwardErrorCorrection::ParseSequenceNumber( On 2016/08/30 12:29:01, danilchap wrote: > is it kMediaStartSeqNum? Yep, changed. https://codereview.webrtc.org/2260803002/diff/360001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:113: writer_.FinalizeFecHeader(seq_num_base, packet_mask_, On 2016/08/30 12:29:01, danilchap wrote: > in another comment you wrote that XorHeaders should be called before calling > FinalizeFecHeader, but in the tests you do not. > Is it easy to adjust tests so that they can serve as a documentation how to use > writer_? It's not super-easy, since XorHeaders are in ForwardErrorCorrection which is one layer about these FEC formatting classes. I could move the XorHeaders to the FEC header formatters, but I should probably combine that move with moving the whole packet mask logic too then. (This is on my maybe-list.) (The FEC code had a lot of these order-dependent things when I started looking at it. It's better now.) https://codereview.webrtc.org/2260803002/diff/360001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:130: std::pair<std::unique_ptr<Packet>, std::unique_ptr<ReceivedFecPacket>> On 2016/08/30 12:29:01, danilchap wrote: > probably cleaner to take two parameters instead of one pair Done. https://codereview.webrtc.org/2260803002/diff/360001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:150: EXPECT_EQ(0, memcmp(&packet_to_write->data[expected_fec_header_size], On 2016/08/30 12:29:01, danilchap wrote: > What functionality of HeaderReaderWriter is tested by this expectation? > > As an almost last step of WriteAndReadHeaders > you do a memcpy from packet_to_write to packet_to_read. > here you check that memory is equal. This memcmp is to check that the ReadFecHeader function wouldn't tamper with the payload. (For FlexFEC, the ReadFecHeader function 'normalizes' the packet mask to Ulpfec format, so it actually does a bit more than just reading.) https://codereview.webrtc.org/2260803002/diff/360001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:171: VerifyHeaders(WriteAndReadHeaders(kUlpfecPacketMaskSizeLBitClear), On 2016/08/30 12:29:01, danilchap wrote: > for clearer separation of run and verify phases it might be better to avoid > chaining, and, may be, split a bit more: > > auto packet_to_write = WriteHeaders(original_packet_mask_size) > auto packet_to_read = ReadHeaders(packet_to_write->data, > packet_to_write->length); > > VerifyFecHeadersAreSame(kExtraExpectationValues, *packet_to_write, > *packet_to_read); Done.
https://codereview.webrtc.org/2260803002/diff/380002/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2260803002/diff/380002/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:140: uint32_t ssrc; there are 3 ssrcs in this class. It feels too many: ssrc is ssrc of the stream fec packet received in. protected_ssrc is an ssrc of 1 stream FEC protects. packet_mask_infos.keys are ssrcs of several streams FEC protects. Can protected_ssrc and packet_mask_infos.begin()->first be different? https://codereview.webrtc.org/2260803002/diff/380002/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/380002/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:74: // Write FEC header. with new helper function names this comment might be removed. (same for Read FEC header.)
Patchset #16 (id:410001) has been deleted
Patchset #16 (id:430001) has been deleted
Patchset #16 (id:450001) has been deleted
https://codereview.webrtc.org/2260803002/diff/380002/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2260803002/diff/380002/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:388: for (const auto& stream_packet_mask_info : fec_packet->packet_mask_infos) { This big diff is mainly due to the removed outer for loop and the corresponding change in indentation. https://codereview.webrtc.org/2260803002/diff/380002/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2260803002/diff/380002/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:140: uint32_t ssrc; On 2016/08/31 14:38:29, danilchap wrote: > there are 3 ssrcs in this class. It feels too many: > ssrc is ssrc of the stream fec packet received in. > protected_ssrc is an ssrc of 1 stream FEC protects. > packet_mask_infos.keys are ssrcs of several streams FEC protects. > Can protected_ssrc and packet_mask_infos.begin()->first be different? Yes, you are absolutely right. The reason for the map was to prepare for multistream protection, but I think that was a misguided attempt seeing that there will be a lot of refactoring needed (especially with the packet masks) before we can add support for that feature. So I will just remove the map here. (The answer to your question is no, protected_ssrc and packet_mask_infos.begin()->first are currently always identical.) https://codereview.webrtc.org/2260803002/diff/380002/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/380002/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:74: // Write FEC header. On 2016/08/31 14:38:29, danilchap wrote: > with new helper function names this comment might be removed. > (same for Read FEC header.) Done.
https://codereview.webrtc.org/2260803002/diff/470001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2260803002/diff/470001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:141: } packet_mask; does anonymous struct help here? i.e. may be size_t packet_mask_offset; size_t packet_mask_size; https://codereview.webrtc.org/2260803002/diff/470001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc (right): https://codereview.webrtc.org/2260803002/diff/470001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc:66: fec_packet->packet_mask = {kPacketMaskOffset, packet_mask_size}; probably better be more explicit: fec_packet->packet_mask.offset = kFecLevel0HeaderSize + 2 fec_packet->packet_mask_size = l_bit ? kUlpfecPacketMaskSizeLBitSet : kUlpfecPacketMaskSizeLBitClear; https://codereview.webrtc.org/2260803002/diff/470001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/470001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:106: EXPECT_EQ(kMediaStartSeqNum, packet_to_read.seq_num_base); may be add EXPECT_EQ(kMediaSsrc, packet_to_read.protected_ssrc)
Rebase.
https://codereview.webrtc.org/2260803002/diff/470001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2260803002/diff/470001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:141: } packet_mask; On 2016/09/01 12:58:48, danilchap wrote: > does anonymous struct help here? > i.e. may be > size_t packet_mask_offset; > size_t packet_mask_size; No big difference :) https://codereview.webrtc.org/2260803002/diff/470001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc (right): https://codereview.webrtc.org/2260803002/diff/470001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer.cc:66: fec_packet->packet_mask = {kPacketMaskOffset, packet_mask_size}; On 2016/09/01 12:58:48, danilchap wrote: > probably better be more explicit: > fec_packet->packet_mask.offset = kFecLevel0HeaderSize + 2 > fec_packet->packet_mask_size = l_bit ? kUlpfecPacketMaskSizeLBitSet : > kUlpfecPacketMaskSizeLBitClear; Removed struct, but I keep the packet_mask_size calculation above due to line 60. https://codereview.webrtc.org/2260803002/diff/470001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/470001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:106: EXPECT_EQ(kMediaStartSeqNum, packet_to_read.seq_num_base); On 2016/09/01 12:58:48, danilchap wrote: > may be add EXPECT_EQ(kMediaSsrc, packet_to_read.protected_ssrc) Good idea!
few more nits https://codereview.webrtc.org/2260803002/diff/530001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2260803002/diff/530001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:120: using ProtectedPacketList = std::list<std::unique_ptr<ProtectedPacket>>; may be move this line to other using <Packet>List = ... declarations (i.e. after ReceivedFecPacket) https://codereview.webrtc.org/2260803002/diff/530001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:256: // Writes FEC payload and some recovery fields in the FEC headers. payloadS https://codereview.webrtc.org/2260803002/diff/530001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction_internal.cc (right): https://codereview.webrtc.org/2260803002/diff/530001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction_internal.cc:209: const int res_mask_bytes = (l_bit == 1) ? kUlpfecPacketMaskSizeLBitSet May be PacketMaskSize(num_media_packets - num_fec_for_imp_packets) is better here, if you do not mind extra casts. May be leave for some next CL. https://codereview.webrtc.org/2260803002/diff/530001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction_internal.cc:250: const int num_imp_mask_bytes = (l_bit == 1) ? kUlpfecPacketMaskSizeLBitSet probably can use PacketMaskSize(num_imp_packets) here https://codereview.webrtc.org/2260803002/diff/530001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction_internal.cc:380: const int num_mask_bytes = (l_bit == 1) ? kUlpfecPacketMaskSizeLBitSet probably can use PacketMaskSize(num_media_packets) here too. https://codereview.webrtc.org/2260803002/diff/530001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/530001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:38: using Packet = ::webrtc::ForwardErrorCorrection::Packet; Using is recommended to put at the top part of the unnamed namespace: namespace { using constants Helpers } // namespace
Rebase + improved data flow in unit test.
Patchset #22 (id:590001) has been deleted
Addressed comments and rewritten unit tests to have clearer data flow. https://codereview.webrtc.org/2260803002/diff/530001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2260803002/diff/530001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:120: using ProtectedPacketList = std::list<std::unique_ptr<ProtectedPacket>>; On 2016/09/01 16:28:48, danilchap wrote: > may be move this line to other using <Packet>List = ... declarations (i.e. after > ReceivedFecPacket) It's needed by line 131 though. I guess I could get rid of this alias, but I think it's kind of nice. https://codereview.webrtc.org/2260803002/diff/530001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:256: // Writes FEC payload and some recovery fields in the FEC headers. On 2016/09/01 16:28:48, danilchap wrote: > payloadS Done. https://codereview.webrtc.org/2260803002/diff/530001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction_internal.cc (right): https://codereview.webrtc.org/2260803002/diff/530001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction_internal.cc:209: const int res_mask_bytes = (l_bit == 1) ? kUlpfecPacketMaskSizeLBitSet On 2016/09/01 16:28:48, danilchap wrote: > May be PacketMaskSize(num_media_packets - num_fec_for_imp_packets) is better > here, if you do not mind extra casts. May be leave for some next CL. Might as well do it here, while we're at it..! https://codereview.webrtc.org/2260803002/diff/530001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/530001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:38: using Packet = ::webrtc::ForwardErrorCorrection::Packet; On 2016/09/01 16:28:48, danilchap wrote: > Using is recommended to put at the top part of the unnamed namespace: > > namespace { > > using > > constants > > Helpers > } // namespace Done.
fewer more nits https://codereview.webrtc.org/2260803002/diff/570001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/570001/webrtc/modules/rtp_rtcp/... 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/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:45: std::unique_ptr<uint8_t[]> packet_mask(new uint8_t[kUlpfecMaxPacketMaskSize]); Does it have to be MaxPacketMasksSize? may be packet_mask_size is enough. https://codereview.webrtc.org/2260803002/diff/570001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:70: read_packet->protected_ssrc = kMediaSsrc; shouldn't ReadFecHeader set this? https://codereview.webrtc.org/2260803002/diff/570001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:106: writer.FecHeaderSize(writer.MinPacketMaskSize( possible to test these two methods separately? Or caller should always use them together? https://codereview.webrtc.org/2260803002/diff/570001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:119: auto packet_mask = GeneratePacketMask(kUlpfecPacketMaskSizeLBitClear, 0xabcd); may be add size_t packet_mask_size = kUlpfecPacketMaskSizeLBitClear and use that local variable inside the test. Would be make more clear why packet_mask and kUlpfecPacketMaskSizeLBitClear go together.
https://codereview.webrtc.org/2260803002/diff/570001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/570001/webrtc/modules/rtp_rtcp/... 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: > using? Nope. The compiler should complain about unused using directives :) https://codereview.webrtc.org/2260803002/diff/570001/webrtc/modules/rtp_rtcp/... 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 09:54:55, danilchap wrote: > Does it have to be MaxPacketMasksSize? may be packet_mask_size is enough. Here, that would be fine. But in the Flexfec unit tests (here: https://codereview.webrtc.org/2269903002/diff/450001/webrtc/modules/rtp_rtcp/...) I need to make the distinction. This is because of the memcmp in VerifyHeader is comparing effective packet mask sizes larger than the precomputed (i.e. Ulpfec) packet masks, and I don't want to memcmp the possibly trailing garbage. What do you think, symmetry (i.e. keep it as is) or asymmetry (make a small change here)? I think I'm biased towards symmetry, but that might be a "easy to write" thing rather than an "easy to read" thing. https://codereview.webrtc.org/2260803002/diff/570001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:70: read_packet->protected_ssrc = kMediaSsrc; On 2016/09/02 09:54:55, danilchap wrote: > shouldn't ReadFecHeader set this? Ehm, yeah. It should definitely not be here. https://codereview.webrtc.org/2260803002/diff/570001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:106: writer.FecHeaderSize(writer.MinPacketMaskSize( On 2016/09/02 09:54:55, danilchap wrote: > possible to test these two methods separately? > Or caller should always use them together? Currently, the only public caller uses these methods together. I'll split it into two expectations though. https://codereview.webrtc.org/2260803002/diff/570001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:119: auto packet_mask = GeneratePacketMask(kUlpfecPacketMaskSizeLBitClear, 0xabcd); On 2016/09/02 09:54:55, danilchap wrote: > may be add size_t packet_mask_size = kUlpfecPacketMaskSizeLBitClear and use that > local variable inside the test. > Would be make more clear why packet_mask and kUlpfecPacketMaskSizeLBitClear go > together. Done. I made it constexpr and named it accordingly however, let me know what you think.
https://codereview.webrtc.org/2260803002/diff/570001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/570001/webrtc/modules/rtp_rtcp/... 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: > On 2016/09/02 09:54:55, danilchap wrote: > > Does it have to be MaxPacketMasksSize? may be packet_mask_size is enough. > > Here, that would be fine. But in the Flexfec unit tests (here: > https://codereview.webrtc.org/2269903002/diff/450001/webrtc/modules/rtp_rtcp/...) > I need to make the distinction. This is because of the memcmp in VerifyHeader is > comparing effective packet mask sizes larger than the precomputed (i.e. Ulpfec) > packet masks, and I don't want to memcmp the possibly trailing garbage. > > What do you think, symmetry (i.e. keep it as is) or asymmetry (make a small > change here)? I think I'm biased towards symmetry, but that might be a "easy to > write" thing rather than an "easy to read" thing. I would vote for asymmetry in this case: Since it is different for ulpfec and flexfec, make it look different. Do not ask reader to guess or look into different file to understand why capacity and size are different., specially that max size constant has a word 'Ulpfec' in it. https://codereview.webrtc.org/2260803002/diff/570001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:119: auto packet_mask = GeneratePacketMask(kUlpfecPacketMaskSizeLBitClear, 0xabcd); On 2016/09/02 11:09:14, brandtr wrote: > On 2016/09/02 09:54:55, danilchap wrote: > > may be add size_t packet_mask_size = kUlpfecPacketMaskSizeLBitClear and use > that > > local variable inside the test. > > Would be make more clear why packet_mask and kUlpfecPacketMaskSizeLBitClear go > > together. > > Done. I made it constexpr and named it accordingly however, let me know what you > think. I think using non-constexpr variable packet_mask_size would be cleaner because that would better align with packet_mask variable. It is not really a constant because it take different values in different tests. If you want to stress it will not change during the test, make it const: const size_t packet_mask_size = kUlpfecPacketMaskSizeLBitClear;
https://codereview.webrtc.org/2260803002/diff/570001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/570001/webrtc/modules/rtp_rtcp/... 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: > On 2016/09/02 11:09:14, brandtr wrote: > > On 2016/09/02 09:54:55, danilchap wrote: > > > Does it have to be MaxPacketMasksSize? may be packet_mask_size is enough. > > > > Here, that would be fine. But in the Flexfec unit tests (here: > > > https://codereview.webrtc.org/2269903002/diff/450001/webrtc/modules/rtp_rtcp/...) > > I need to make the distinction. This is because of the memcmp in VerifyHeader > is > > comparing effective packet mask sizes larger than the precomputed (i.e. > Ulpfec) > > packet masks, and I don't want to memcmp the possibly trailing garbage. > > > > What do you think, symmetry (i.e. keep it as is) or asymmetry (make a small > > change here)? I think I'm biased towards symmetry, but that might be a "easy > to > > write" thing rather than an "easy to read" thing. > > I would vote for asymmetry in this case: Since it is different for ulpfec and > flexfec, make it look different. > Do not ask reader to guess or look into different file to understand why > capacity and size are different., specially that max size constant has a word > 'Ulpfec' in it. Done. https://codereview.webrtc.org/2260803002/diff/570001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:119: auto packet_mask = GeneratePacketMask(kUlpfecPacketMaskSizeLBitClear, 0xabcd); On 2016/09/02 11:34:58, danilchap wrote: > On 2016/09/02 11:09:14, brandtr wrote: > > On 2016/09/02 09:54:55, danilchap wrote: > > > may be add size_t packet_mask_size = kUlpfecPacketMaskSizeLBitClear and use > > that > > > local variable inside the test. > > > Would be make more clear why packet_mask and kUlpfecPacketMaskSizeLBitClear > go > > > together. > > > > Done. I made it constexpr and named it accordingly however, let me know what > you > > think. > > I think using non-constexpr variable packet_mask_size would be cleaner because > that would better align with packet_mask variable. > It is not really a constant because it take different values in different tests. > If you want to stress it will not change during the test, make it const: > const size_t packet_mask_size = kUlpfecPacketMaskSizeLBitClear; Done.
lgtm
Rebase.
https://codereview.webrtc.org/2260803002/diff/650001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2260803002/diff/650001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:347: class FecHeaderReader { Have you considered moving these to the top instead to avoid having to forward declare? https://codereview.webrtc.org/2260803002/diff/650001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/650001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:137: } Maybe add one test where you have a small "correct" header which you compare to what is produced by the writer. You can also read it with the reader and ensure that it's parsed correctly. This avoids issues where the writer and the reader have the same bugs.
Patchset #25 (id:670001) has been deleted
Patchset #25 (id:690001) has been deleted
Patchset #25 (id:710001) has been deleted
Patchset #25 (id:730001) has been deleted
Patchset #25 (id:750001) has been deleted
https://codereview.webrtc.org/2260803002/diff/650001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2260803002/diff/650001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:347: class FecHeaderReader { On 2016/09/13 08:49:34, stefan-webrtc (holmer) wrote: > Have you considered moving these to the top instead to avoid having to forward > declare? I did consider that. However, then I would have had to forward declare the Packet types that live in ForwardErrorCorrection but are used in this class. Trying to do so gave me compiler errors, like ../../webrtc/modules/rtp_rtcp/source/forward_error_correction.h:33:7: error: incomplete type 'webrtc::ForwardErrorCorrection' named in nested name specifier class ForwardErrorCorrection::Packet; ^~~~~~~~~~~~~~~~~~~~~~~~ ../../webrtc/modules/rtp_rtcp/source/forward_error_correction.h:32:7: note: forward declaration of 'webrtc::ForwardErrorCorrection' class ForwardErrorCorrection; So I found it easier (albeit less nice) to define these classes below the erasure code class. https://codereview.webrtc.org/2260803002/diff/650001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc (right): https://codereview.webrtc.org/2260803002/diff/650001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/ulpfec_header_reader_writer_unittest.cc:137: } On 2016/09/13 08:49:34, stefan-webrtc (holmer) wrote: > Maybe add one test where you have a small "correct" header which you compare to > what is produced by the writer. You can also read it with the reader and ensure > that it's parsed correctly. This avoids issues where the writer and the reader > have the same bugs. Done.
Rebase after gyp deprecation.
Patchset #27 (id:810001) has been deleted
Rebase after gyp deprecation.
Comments have been adressed, PTAL again holmer :)
lgtm
The CQ bit was checked by brandtr@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/8519)
The CQ bit was checked by brandtr@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/16924)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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://crrev.com/78db1582e514b5ecd5bf9a38ef6a1613ba3a5752 Cr-Commit-Position: refs/heads/master@{#14316} ==========
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/78db1582e514b5ecd5bf9a38e... ==========
Message was sent while issue was closed.
Patchset 29 (id:??) landed as https://crrev.com/78db1582e514b5ecd5bf9a38ef6a1613ba3a5752 Cr-Commit-Position: refs/heads/master@{#14316}
Message was sent while issue was closed.
Committed patchset #29 (id:870001) manually as 78db1582e514b5ecd5bf9a38ef6a1613ba3a5752 (presubmit successful). |
