Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2080553003:
Style updates for ForwardErrorCorrection and related classes (Closed)
|
Created:
4 years, 6 months ago by brandtr Modified:
4 years, 5 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. |
DescriptionStyle updates for ForwardErrorCorrection and related classes.
This CL mainly updates the FEC code to use more C++11 features and
to be more in line with the style guide. These changes should
have no impact on the functionality provided by the FEC.
Summary of style fixes:
- Use range-based for loops, where applicable.
- Use auto type deduction for iterator type names.
- Use RTC_DCHECK instead of assert.
- Rename FEC to Fec, where applicable.
- Update test_fec.cc to use variable_names rather than variableNames.
- Avoid redefining the PacketList types outside ForwardErrorCorrection.
Another minor change is that storage for the packet masks, as these
are generated, now is provided by a member variable, rather than
being dynamically allocated on every call to GenerateFec.
BUG=webrtc:5654
R=danilchap@webrtc.org, stefan@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/ae4f7674e472f21701c1e65bc2c9b28e421dee8e
Patch Set 1 : Initial fixes. #Patch Set 2 : Replace single-member enum with constant for consistency. #Patch Set 3 : Update variable names in test_fec.cc in accordance with style guide, and use auto in places. #Patch Set 4 : Use auto for iterator type names in unit tests. #Patch Set 5 : Avoid redefining PacketList type outside of ForwardErrorCorrection. #Patch Set 6 : Make it clearer that the set intersection leaves the two original sets untouched. #Patch Set 7 : Rebase. #Patch Set 8 : Re-add explicit constructors. #
Total comments: 39
Patch Set 9 : Response to feedback, changed asserts -> RTC_DCHECKs, removed some unused header includes. #Patch Set 10 : Line length and fec_packet_list.clear(). #Patch Set 11 : Re-add <iterator> in forward_error_correction.cc. #
Total comments: 24
Patch Set 12 : Response to feedback. #
Total comments: 5
Patch Set 13 : Switch from {0} -> () initializers of arrays. #
Total comments: 2
Patch Set 14 : Rebase to try to fix the patch errors. #Patch Set 15 : Rebase 2. #Patch Set 16 : Rebase. #Messages
Total messages: 41 (19 generated)
Description was changed from ========== FlexFEC pt. 1: Mini-fixes in ForwardErrorCorrection. This is the first CL in the endeavour towards implementing FlexFEC as specified by draft-ietf-payload-flexible-fec-scheme-02. In this CL, some smaller fixes are introduced to the ForwardErrorCorrection class. This class currently is responsible for parsing/writing the ULPFEC headers and for encoding/decoding the information and repair stream using a parity erasure code. The general work plan for the FlexFEC efforts will follow this bottoms-up approach: * Refactor ForwardErrorCorrection into ParityErasureCode and split out the ULPFEC specific parts into an ULP header parser/writer. This should allow for reusing the underlying erasure code in both ULPFEC as well as FlexFEC. * Write new FlexFecConsumer and FlexFecProducer classes to act as buffers between the ParityErasureCode and higher layers. * Implement higher layers: sending and receiving repair packets on a separate SSRC. Updating the logic for selecting the FEC rates based on bandwidth estimation and other signals. BUG=webrtc:5654 ========== to ========== FlexFEC pt. 1: Mini-fixes in ForwardErrorCorrection. This is the first CL in the endeavour towards implementing FlexFEC as specified by draft-ietf-payload-flexible-fec-scheme-02. In this CL, some smaller fixes are introduced to the ForwardErrorCorrection class. This class currently is responsible for parsing/writing the ULPFEC headers and for encoding/decoding the information and repair stream using a parity erasure code. The general work plan for the FlexFEC efforts will follow this bottoms-up approach: * Refactor ForwardErrorCorrection into ParityErasureCode and split out the ULPFEC specific parts into an ULP header parser/writer. This should allow for reusing the underlying erasure code in both ULPFEC as well as FlexFEC. * Write new FlexFecConsumer and FlexFecProducer classes to act as buffers between the ParityErasureCode and higher layers. * Implement higher layers: sending and receiving repair packets on a separate SSRC. Updating the logic for selecting the FEC rates based on bandwidth estimation and other signals. BUG=webrtc:5654 ==========
Description was changed from ========== FlexFEC pt. 1: Mini-fixes in ForwardErrorCorrection. This is the first CL in the endeavour towards implementing FlexFEC as specified by draft-ietf-payload-flexible-fec-scheme-02. In this CL, some smaller fixes are introduced to the ForwardErrorCorrection class. This class currently is responsible for parsing/writing the ULPFEC headers and for encoding/decoding the information and repair stream using a parity erasure code. The general work plan for the FlexFEC efforts will follow this bottoms-up approach: * Refactor ForwardErrorCorrection into ParityErasureCode and split out the ULPFEC specific parts into an ULP header parser/writer. This should allow for reusing the underlying erasure code in both ULPFEC as well as FlexFEC. * Write new FlexFecConsumer and FlexFecProducer classes to act as buffers between the ParityErasureCode and higher layers. * Implement higher layers: sending and receiving repair packets on a separate SSRC. Updating the logic for selecting the FEC rates based on bandwidth estimation and other signals. BUG=webrtc:5654 ========== to ========== FlexFEC pt. 1a: Mini-fixes in ULPFEC code. This is the first CL in the endeavour towards implementing FlexFEC as specified by draft-ietf-payload-flexible-fec-scheme-02. In this CL, some smaller things are fixed in the ULPFEC code, i.e. the ForwardErrorCorrection class, the buffers in FecReceiver and ProducerFec, and the corresponding tests. The general work plan for the FlexFEC efforts will follow this bottoms-up approach: * Refactor ForwardErrorCorrection into ParityErasureCode and split out the ULPFEC specific parts into an ULP header parser/writer. This should allow for reusing the underlying erasure code in both ULPFEC as well as FlexFEC. * Write new FlexFecConsumer and FlexFecProducer classes to act as buffers between the ParityErasureCode and higher layers. * Implement higher layers: sending and receiving repair packets on a separate SSRC. Updating the logic for selecting the FEC rates based on bandwidth estimation and other signals. BUG=webrtc:5654 ==========
brandtr@webrtc.org changed reviewers: + danilchap@webrtc.org, stefan@webrtc.org
Please have a look :) The following three CLs are related to this one, and split up according to some semi-logical order.
Nice to see so many style improvements. Would like to see more. https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc (right): https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc:241: auto it = recovered_packet_list_.begin(); make one more step and use c++11 loop: for (auto* recoverd_packet : recovered_packet_list_) { s/(*it)/recoverd_packet/ } https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:41: const uint8_t kMaxFecPackets = ForwardErrorCorrection::kMaxMediaPackets; prefer size_t for this variable. https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:110: // N.B. that any potential RED headers are added/removed before calling what is N.B. ? https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:213: assert(!media_packet_list.empty()); use RTC_DCHECK instead of assert https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:420: Packet* first_media_packet = media_packet_list.front(); may be add RTC_DCHECK(!media_packet_list.empty()) before this line. https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:421: assert(first_media_packet != NULL); RTC_DCHECK(first_media_packet); https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:466: FecPacket* fec_packet = *fec_packet_list_it; may be reduce a bit futher: FecPacket* fec_packet = fec_packet_list_.front(); https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:511: for (auto it = fec_packet_list_.cbegin(); this loop seems better to reduce to c++11 loop: for (auto* fec_packet : fec_packet_list_) { s/(*it)/fec_packet/ } https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:530: while (fec_packet_list_it != fec_packet_list_.end()) { reduce this one to c++11 loop too. https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:730: auto protected_it = fec_packet->protected_pkt_list.cbegin(); ditto https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:795: auto it = fec_packet->protected_pkt_list.cbegin(); ditto https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:34: static const uint8_t kMaxMediaPackets = 48u; using small int types because value is small is discouraged. prefer size_t for constants describing size (even if it is small). Might as well use constexpr instead of const. https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:281: static bool InitRecoveryOfPacket(const FecPacket* fec_packet, maybe InitPacketRecovery or StartPacketRecovery (since word 'Start' better match word 'Finish' in the next function) https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:289: static bool FinishRecoveryOfPacket(RecoveredPacket* recovered); may be FinishPacketRecovery? just a suggestion, feel free to disagree if you prefer your name. https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc (right): https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc:120: ForwardErrorCorrection::Packet* media_packet = NULL; = nullptr; https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc:139: random_seed_file = NULL; ditto https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc:292: ForwardErrorCorrection::ReceivedPacket* received_packet; do not leave uninitialized: either initialized with nullptr, or (probably better) - move declaration to initialization. auto as type should be ok there. https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc:318: ForwardErrorCorrection::Packet* fec_packet; ditto https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc:360: uint8_t* fec_mask; ditto
Patchset #11 (id:200001) has been deleted
https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc (right): https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc:241: auto it = recovered_packet_list_.begin(); On 2016/06/29 10:31:52, danilchap wrote: > make one more step and use c++11 loop: > for (auto* recoverd_packet : recovered_packet_list_) { > s/(*it)/recoverd_packet/ > } Done. https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:41: const uint8_t kMaxFecPackets = ForwardErrorCorrection::kMaxMediaPackets; On 2016/06/29 10:31:52, danilchap wrote: > prefer size_t for this variable. Done. Would it make sense to change all of these constants to constexpr as well? https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:110: // N.B. that any potential RED headers are added/removed before calling On 2016/06/29 10:31:53, danilchap wrote: > what is N.B. ? "Nota Bene" == "note well" in latin. It's a bit academese, so I changed it to "Note". https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:213: assert(!media_packet_list.empty()); On 2016/06/29 10:31:52, danilchap wrote: > use RTC_DCHECK instead of assert Done. https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:420: Packet* first_media_packet = media_packet_list.front(); On 2016/06/29 10:31:53, danilchap wrote: > may be add RTC_DCHECK(!media_packet_list.empty()) before this line. Done. https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:421: assert(first_media_packet != NULL); On 2016/06/29 10:31:52, danilchap wrote: > RTC_DCHECK(first_media_packet); Done. https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:466: FecPacket* fec_packet = *fec_packet_list_it; On 2016/06/29 10:31:52, danilchap wrote: > may be reduce a bit futher: > FecPacket* fec_packet = fec_packet_list_.front(); Done. https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:511: for (auto it = fec_packet_list_.cbegin(); On 2016/06/29 10:31:52, danilchap wrote: > this loop seems better to reduce to c++11 loop: > for (auto* fec_packet : fec_packet_list_) { > s/(*it)/fec_packet/ > } Done. https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:530: while (fec_packet_list_it != fec_packet_list_.end()) { On 2016/06/29 10:31:52, danilchap wrote: > reduce this one to c++11 loop too. Done. https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:730: auto protected_it = fec_packet->protected_pkt_list.cbegin(); On 2016/06/29 10:31:52, danilchap wrote: > ditto Done. https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:795: auto it = fec_packet->protected_pkt_list.cbegin(); On 2016/06/29 10:31:52, danilchap wrote: > ditto Done. https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:34: static const uint8_t kMaxMediaPackets = 48u; On 2016/06/29 10:31:53, danilchap wrote: > using small int types because value is small is discouraged. > prefer size_t for constants describing size (even if it is small). > Might as well use constexpr instead of const. Acknowledged. https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:281: static bool InitRecoveryOfPacket(const FecPacket* fec_packet, On 2016/06/29 10:31:53, danilchap wrote: > maybe InitPacketRecovery or StartPacketRecovery (since word 'Start' better match > word 'Finish' in the next function) I like that. Done! https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:289: static bool FinishRecoveryOfPacket(RecoveredPacket* recovered); On 2016/06/29 10:31:53, danilchap wrote: > may be FinishPacketRecovery? > just a suggestion, feel free to disagree if you prefer your name. Done. https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc (right): https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc:120: ForwardErrorCorrection::Packet* media_packet = NULL; On 2016/06/29 10:31:53, danilchap wrote: > = nullptr; Done. https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc:139: random_seed_file = NULL; On 2016/06/29 10:31:53, danilchap wrote: > ditto Done. https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc:292: ForwardErrorCorrection::ReceivedPacket* received_packet; On 2016/06/29 10:31:53, danilchap wrote: > do not leave uninitialized: either initialized with nullptr, or (probably > better) - move declaration to initialization. > auto as type should be ok there. Acknowledged. https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc:318: ForwardErrorCorrection::Packet* fec_packet; On 2016/06/29 10:31:53, danilchap wrote: > ditto Acknowledged. https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc:360: uint8_t* fec_mask; On 2016/06/29 10:31:53, danilchap wrote: > ditto Acknowledged.
Reduce CL description to reflect only changes in the CL, since that description would be visible in the code history. plans better write into the tracking bug or as a message/comment. https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2080553003/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:41: const uint8_t kMaxFecPackets = ForwardErrorCorrection::kMaxMediaPackets; On 2016/06/29 14:24:10, brandtr wrote: > On 2016/06/29 10:31:52, danilchap wrote: > > prefer size_t for this variable. > > Done. > > Would it make sense to change all of these constants to constexpr as well? For new code should use constexpr, for old code like this one - up to you. Same for moving this constants into unnamed namespace. https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc (left): https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc:19: #include <string.h> you need one of those headers for memcpy, another one for time() (used for random seed).
Mention in description if this CL is expected to not have any functional changes. https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:109: int32_t ForwardErrorCorrection::GenerateFEC(const PacketList& media_packet_list, Feel free to rename all FEC to Fec. https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:315: new uint8_t[num_fec_packets * kMaskSizeLBitSet]); Could we allocate this once and store it as a member instead? Or allocate a large enough buffer statically to avoid the dynamic allocation? https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:420: uint16_t seq_num = ParseSequenceNumber(first_media_packet->data); Maybe move this parsing down to where the seq_num is actually used? https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:724: for (auto* protected_packet : fec_packet->protected_pkt_list) { const auto* https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:787: for (auto* protected_packet : fec_packet->protected_pkt_list) { const auto* https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:607: for (auto it = media_packet_list_.begin(); it != media_packet_list_.end(); Should be able to change this to a range-based loop and increment inside the body instead https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:672: ++it, ++i) { Same here https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:743: ++it, ++i) { And here https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:826: while (media_packet_list_it != media_packet_list_.end()) { Make this a range-based loop https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:859: while (packet_list_it != packet_list.end()) { and this, I think? https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc (right): https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc:148: uint8_t* packet_mask = new uint8_t[packet_mask_max * kNumMaskBytesL1]; unique_ptr, perhaps
Description was changed from ========== FlexFEC pt. 1a: Mini-fixes in ULPFEC code. This is the first CL in the endeavour towards implementing FlexFEC as specified by draft-ietf-payload-flexible-fec-scheme-02. In this CL, some smaller things are fixed in the ULPFEC code, i.e. the ForwardErrorCorrection class, the buffers in FecReceiver and ProducerFec, and the corresponding tests. The general work plan for the FlexFEC efforts will follow this bottoms-up approach: * Refactor ForwardErrorCorrection into ParityErasureCode and split out the ULPFEC specific parts into an ULP header parser/writer. This should allow for reusing the underlying erasure code in both ULPFEC as well as FlexFEC. * Write new FlexFecConsumer and FlexFecProducer classes to act as buffers between the ParityErasureCode and higher layers. * Implement higher layers: sending and receiving repair packets on a separate SSRC. Updating the logic for selecting the FEC rates based on bandwidth estimation and other signals. BUG=webrtc:5654 ========== to ========== Style updates for ForwardErrorCorrection and related classes. This CL mainly updates the FEC code to use more C++11 features and to be more in line with the style guide. These changes should have no impact on the functionality provided by the FEC. Summary of style fixes: - Use range-based for loops, where applicable. - Use auto type deduction for iterator type names. - Use RTC_DCHECK instead of assert. - Rename FEC to Fec, where applicable. - Update test_fec.cc to use variable_names rather than variableNames. - Avoid redefining the PacketList types outside ForwardErrorCorrection. Another minor change is that storage for the packet masks, as these are generated, now is provided by a member variable, rather than being dynamically allocated on every call to GenerateFec. BUG=webrtc:5654 ==========
Comments addressed. Update to "1b" CL coming soon. https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:109: int32_t ForwardErrorCorrection::GenerateFEC(const PacketList& media_packet_list, On 2016/06/29 16:14:03, stefan-webrtc (holmer) wrote: > Feel free to rename all FEC to Fec. Done. https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:315: new uint8_t[num_fec_packets * kMaskSizeLBitSet]); On 2016/06/29 16:14:03, stefan-webrtc (holmer) wrote: > Could we allocate this once and store it as a member instead? Or allocate a > large enough buffer statically to avoid the dynamic allocation? I changed to using member variables for this. With RFC 5109 ULPFEC, we need to (at most) support generator matrices of 48x48, i.e. the packet mask member variable is 288 bytes. https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:420: uint16_t seq_num = ParseSequenceNumber(first_media_packet->data); On 2016/06/29 16:14:03, stefan-webrtc (holmer) wrote: > Maybe move this parsing down to where the seq_num is actually used? Moved it down a couple of lines; no point re-parsing in every iteration of the loop :) https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:724: for (auto* protected_packet : fec_packet->protected_pkt_list) { On 2016/06/29 16:14:03, stefan-webrtc (holmer) wrote: > const auto* Done. https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:787: for (auto* protected_packet : fec_packet->protected_pkt_list) { On 2016/06/29 16:14:03, stefan-webrtc (holmer) wrote: > const auto* Done. https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:607: for (auto it = media_packet_list_.begin(); it != media_packet_list_.end(); On 2016/06/29 16:14:03, stefan-webrtc (holmer) wrote: > Should be able to change this to a range-based loop and increment inside the > body instead See "1b" CL. https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:672: ++it, ++i) { On 2016/06/29 16:14:03, stefan-webrtc (holmer) wrote: > Same here See "1b" CL. https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:743: ++it, ++i) { On 2016/06/29 16:14:03, stefan-webrtc (holmer) wrote: > And here See "1b" CL. https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:826: while (media_packet_list_it != media_packet_list_.end()) { On 2016/06/29 16:14:03, stefan-webrtc (holmer) wrote: > Make this a range-based loop Rewrote using std::equal and custom predicate instead. https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:859: while (packet_list_it != packet_list.end()) { On 2016/06/29 16:14:03, stefan-webrtc (holmer) wrote: > and this, I think? Done. https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc (left): https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc:19: #include <string.h> On 2016/06/29 15:18:24, danilchap wrote: > you need one of those headers for memcpy, another one for time() (used for > random seed). Done. https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc (right): https://codereview.webrtc.org/2080553003/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc:148: uint8_t* packet_mask = new uint8_t[packet_mask_max * kNumMaskBytesL1]; On 2016/06/29 16:14:03, stefan-webrtc (holmer) wrote: > unique_ptr, perhaps Done.
https://codereview.webrtc.org/2080553003/diff/240001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2080553003/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:89: packet_mask_{0}, tmp_packet_mask_{0} {} Never seen this type of initialization before. Do you know if it's ok according to style etc? https://codereview.webrtc.org/2080553003/diff/240001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/2080553003/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:849: for (const auto* packet : packet_list) { I prefer not using auto when the type is fairly simple (e.g., not a long iterator or something like that). https://codereview.webrtc.org/2080553003/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:851: auto received_packet = new ForwardErrorCorrection::ReceivedPacket(); Same here.
https://codereview.webrtc.org/2080553003/diff/240001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2080553003/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:89: packet_mask_{0}, tmp_packet_mask_{0} {} On 2016/06/30 13:58:34, stefan-webrtc (holmer) wrote: > Never seen this type of initialization before. Do you know if it's ok according > to style etc? I did check, and it is acceptable: https://google.github.io/styleguide/cppguide.html#Braced_Initializer_List I did this to initialize all entries to the array to 0. But IIUC that would also happen if I used the () form, so that might be better. (The arrays are zeroed out before being used.) https://codereview.webrtc.org/2080553003/diff/240001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/2080553003/diff/240001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:849: for (const auto* packet : packet_list) { On 2016/06/30 13:58:34, stefan-webrtc (holmer) wrote: > I prefer not using auto when the type is fairly simple (e.g., not a long > iterator or something like that). Agree! Not sure why I changed it here, but I think I did it to simplify the rebasing in the other CL: https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... Over there I feel that the type name would be pretty long without auto.
lgtm
lgtm too. more improvements probably better do in next CLs https://codereview.webrtc.org/2080553003/diff/260001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2080553003/diff/260001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:837: return 0; this function always return 0 for now. may be make it void https://codereview.webrtc.org/2080553003/diff/260001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2080553003/diff/260001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:162: int GenerateFec(const PacketList& media_packet_list, with two possible values where one means success and another one - failure might be better to return bool. Though that change should be done carefully because bool and int implicitly convertible to each other, but raw values have opposite meanings. (i.e. "if (GenerateFec() == 0) DoGood();" would still compile but would be an error)
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: ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/14952)
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: ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/11071) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/14953)
Patchset #14 (id:280001) has been deleted
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: ios64_sim_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/8757) ios_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/10999) ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/16309) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/14956) linux_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/16414)
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: mac_compile_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
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: ios_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/16333) ios_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/14980) linux_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...) win_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/16309)
Message was sent while issue was closed.
Description was changed from ========== Style updates for ForwardErrorCorrection and related classes. This CL mainly updates the FEC code to use more C++11 features and to be more in line with the style guide. These changes should have no impact on the functionality provided by the FEC. Summary of style fixes: - Use range-based for loops, where applicable. - Use auto type deduction for iterator type names. - Use RTC_DCHECK instead of assert. - Rename FEC to Fec, where applicable. - Update test_fec.cc to use variable_names rather than variableNames. - Avoid redefining the PacketList types outside ForwardErrorCorrection. Another minor change is that storage for the packet masks, as these are generated, now is provided by a member variable, rather than being dynamically allocated on every call to GenerateFec. BUG=webrtc:5654 ========== to ========== Style updates for ForwardErrorCorrection and related classes. This CL mainly updates the FEC code to use more C++11 features and to be more in line with the style guide. These changes should have no impact on the functionality provided by the FEC. Summary of style fixes: - Use range-based for loops, where applicable. - Use auto type deduction for iterator type names. - Use RTC_DCHECK instead of assert. - Rename FEC to Fec, where applicable. - Update test_fec.cc to use variable_names rather than variableNames. - Avoid redefining the PacketList types outside ForwardErrorCorrection. Another minor change is that storage for the packet masks, as these are generated, now is provided by a member variable, rather than being dynamically allocated on every call to GenerateFec. BUG=webrtc:5654 R=danilchap@webrtc.org, stefan@webrtc.org Committed: https://crrev.com/ae4f7674e472f21701c1e65bc2c9b28e421dee8e Cr-Commit-Position: refs/heads/master@{#13403} ==========
Message was sent while issue was closed.
Description was changed from ========== Style updates for ForwardErrorCorrection and related classes. This CL mainly updates the FEC code to use more C++11 features and to be more in line with the style guide. These changes should have no impact on the functionality provided by the FEC. Summary of style fixes: - Use range-based for loops, where applicable. - Use auto type deduction for iterator type names. - Use RTC_DCHECK instead of assert. - Rename FEC to Fec, where applicable. - Update test_fec.cc to use variable_names rather than variableNames. - Avoid redefining the PacketList types outside ForwardErrorCorrection. Another minor change is that storage for the packet masks, as these are generated, now is provided by a member variable, rather than being dynamically allocated on every call to GenerateFec. BUG=webrtc:5654 ========== to ========== Style updates for ForwardErrorCorrection and related classes. This CL mainly updates the FEC code to use more C++11 features and to be more in line with the style guide. These changes should have no impact on the functionality provided by the FEC. Summary of style fixes: - Use range-based for loops, where applicable. - Use auto type deduction for iterator type names. - Use RTC_DCHECK instead of assert. - Rename FEC to Fec, where applicable. - Update test_fec.cc to use variable_names rather than variableNames. - Avoid redefining the PacketList types outside ForwardErrorCorrection. Another minor change is that storage for the packet masks, as these are generated, now is provided by a member variable, rather than being dynamically allocated on every call to GenerateFec. BUG=webrtc:5654 R=danilchap@webrtc.org, stefan@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/ae4f7674e472f21701c1e65bc... ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/ae4f7674e472f21701c1e65bc2c9b28e421dee8e Cr-Commit-Position: refs/heads/master@{#13403}
Message was sent while issue was closed.
Committed patchset #16 (id:340001) manually as ae4f7674e472f21701c1e65bc2c9b28e421dee8e (presubmit successful).
Message was sent while issue was closed.
Description was changed from ========== Style updates for ForwardErrorCorrection and related classes. This CL mainly updates the FEC code to use more C++11 features and to be more in line with the style guide. These changes should have no impact on the functionality provided by the FEC. Summary of style fixes: - Use range-based for loops, where applicable. - Use auto type deduction for iterator type names. - Use RTC_DCHECK instead of assert. - Rename FEC to Fec, where applicable. - Update test_fec.cc to use variable_names rather than variableNames. - Avoid redefining the PacketList types outside ForwardErrorCorrection. Another minor change is that storage for the packet masks, as these are generated, now is provided by a member variable, rather than being dynamically allocated on every call to GenerateFec. BUG=webrtc:5654 R=danilchap@webrtc.org, stefan@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/ae4f7674e472f21701c1e65bc... ========== to ========== Style updates for ForwardErrorCorrection and related classes. This CL mainly updates the FEC code to use more C++11 features and to be more in line with the style guide. These changes should have no impact on the functionality provided by the FEC. Summary of style fixes: - Use range-based for loops, where applicable. - Use auto type deduction for iterator type names. - Use RTC_DCHECK instead of assert. - Rename FEC to Fec, where applicable. - Update test_fec.cc to use variable_names rather than variableNames. - Avoid redefining the PacketList types outside ForwardErrorCorrection. Another minor change is that storage for the packet masks, as these are generated, now is provided by a member variable, rather than being dynamically allocated on every call to GenerateFec. BUG=webrtc:5654 R=danilchap@webrtc.org, stefan@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/ae4f7674e472f21701c1e65bc... ========== |
