Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2099243003:
Use std::unique_ptr in ForwardErrorCorrection. (Closed)
|
Created:
4 years, 5 months ago by brandtr Modified:
4 years, 4 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, danilchap, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@flexfec-pt1a_mini-fixes-in-ULPFEC Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionUse std::unique_ptr in ForwardErrorCorrection.
This CL converts the ForwardErrorCorrection class to use std::unique_ptr
for memory management, rather than manually delete'ing allocated memory.
It further renames some data structures and types to distinguish between
generated FEC packets (i.e. coming from the encode operation) and
received FEC packets (i.e. coming in over the wire, intended for the
decode operation).
BUG=webrtc:5654
Committed: https://crrev.com/35c480cf18122a6533c3c934d8794049f97ffb09
Cr-Commit-Position: refs/heads/master@{#13687}
Patch Set 1 : Initial. #Patch Set 2 : Rebase on top of FlexFEC pt. 1a. #
Total comments: 22
Patch Set 3 : Rebase on top of FlexFEC pt. 1a. #Patch Set 4 : Feedback response. #
Total comments: 30
Patch Set 5 : Response to PS4 feedback. #
Total comments: 2
Patch Set 6 : Remove _list suffix from header as well. #
Total comments: 4
Patch Set 7 : Feedback response: const auto ref and int n #Patch Set 8 : Rebase on top of master. #Patch Set 9 : Lint fix and updated comment. #Patch Set 10 : Rebase. #
Total comments: 20
Patch Set 11 : Lint fixes. #Patch Set 12 : Response to feedback. #
Total comments: 8
Patch Set 13 : Feedback response. #
Total comments: 4
Patch Set 14 : Final nit from danilchap. #
Total comments: 6
Messages
Total messages: 40 (15 generated)
brandtr@webrtc.org changed reviewers: + danilchap@webrtc.org, stefan@webrtc.org
I see now that many of the comments I had in the previous CL have been fixed here. Those that have you can ignore in the previous one. https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:452: while (it != recovered_packet_list->end()) { Perhaps use range-based loop instead? https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:462: new RecoveredPacket); new RecoveredPacket() https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:465: // Inserted Media packet is already sent to VCM. I think you can replace VCM with jitter buffer, or simply write "passed on" https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:506: std::unique_ptr<ReceivedFecPacket> fec_packet(new ReceivedFecPacket); new ReceivedFecPacket() https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:521: std::unique_ptr<ProtectedPacket> protected_packet(new ProtectedPacket); () https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:539: if (received_fec_packet_list_.size() > kMaxFecPackets) { Feel free to remove {} since this is a two line if statement. https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:560: while (it_p != protected_packets->end() && it_r != recovered_packets->end()) { Does this implement intersect? Is it better than calling std::set_intersection as before? Should it be broken out to a separate function? https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/producer_fec.cc (right): https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/producer_fec.cc:143: new ForwardErrorCorrection::Packet); () https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:923: void RtpFecTest::DeepCopyAndRemoveHalf(const PacketList& src, DeepCopyEveryOtherPacket() is better I think. Or maybe generalize to DeepCopyEveryNthPacket()? https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/t... File webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc (right): https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc:65: new ForwardErrorCorrection::ReceivedPacket); ()
Patchset #2 (id:20001) has been deleted
Description was changed from ========== FlexFEC pt. 1b: Use std::unique_ptr in ForwardErrorCorrection. This CL converts the ForwardErrorCorrection class (i.e. the parity erasure code) to use std::unique_ptr for memory management, rather than manually delete'ing allocated memory. It further renames some data structures and types to distinguish between generated FEC packets (i.e. coming from the encode operation) and received FEC packets (i.e. coming in over the wire, intended for the decode operation). BUG=webrtc:5654 ========== to ========== Use std::unique_ptr in ForwardErrorCorrection. This CL converts the ForwardErrorCorrection class (i.e. the parity erasure code) to use std::unique_ptr for memory management, rather than manually delete'ing allocated memory. It further renames some data structures and types to distinguish between generated FEC packets (i.e. coming from the encode operation) and received FEC packets (i.e. coming in over the wire, intended for the decode operation). BUG=webrtc:5654 ==========
Addressed comments and rebased on top of "1a". https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:452: while (it != recovered_packet_list->end()) { On 2016/06/29 19:36:18, stefan-webrtc (holmer) wrote: > Perhaps use range-based loop instead? Done. https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:462: new RecoveredPacket); On 2016/06/29 19:36:18, stefan-webrtc (holmer) wrote: > new RecoveredPacket() Done. https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:465: // Inserted Media packet is already sent to VCM. On 2016/06/29 19:36:18, stefan-webrtc (holmer) wrote: > I think you can replace VCM with jitter buffer, or simply write "passed on" Done. https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:506: std::unique_ptr<ReceivedFecPacket> fec_packet(new ReceivedFecPacket); On 2016/06/29 19:36:18, stefan-webrtc (holmer) wrote: > new ReceivedFecPacket() Done. https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:521: std::unique_ptr<ProtectedPacket> protected_packet(new ProtectedPacket); On 2016/06/29 19:36:18, stefan-webrtc (holmer) wrote: > () Done. https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:539: if (received_fec_packet_list_.size() > kMaxFecPackets) { On 2016/06/29 19:36:18, stefan-webrtc (holmer) wrote: > Feel free to remove {} since this is a two line if statement. I'm happy to keep the {}, as it conforms with the rest of the code in the FEC subsystem. (Discussed this during today's standup.) https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:560: while (it_p != protected_packets->end() && it_r != recovered_packets->end()) { On 2016/06/29 19:36:18, stefan-webrtc (holmer) wrote: > Does this implement intersect? Is it better than calling std::set_intersection > as before? Should it be broken out to a separate function? Yes, these lines correspond to the previous call to std::set_intersection and the following updating of the recovered packet's pkt member. There should be no difference in functionality. I would've like to keep the previous code, but was unable to figure out how to do it; the problem is that the types, when wrapped by std::unique_ptr, are not natively covariant, i.e. the fact that a RecoveredPacket is a SortablePacket does not mean that a std::unique_ptr<RecoveredPacket> is a std::unique_ptr<SortablePacket>. This breaks the call to the comparator in std::set_intersection (see e.g. http://en.cppreference.com/w/cpp/algorithm/set_intersection). My solution was to reimplement the set intersection manually and use a parametric comparator (see row 56). I discussed the issue of the invariant std::unique_ptr's with some team members, and their advice was to reimplement the set intersection manually. I'm happy to change back to the previous solution, if you have an idea on how to get it to work :) I don't believe that this needs to be it's own function, as this code is only used in this function, whose sole purpose is to do these operations. https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/producer_fec.cc (right): https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/producer_fec.cc:143: new ForwardErrorCorrection::Packet); On 2016/06/29 19:36:18, stefan-webrtc (holmer) wrote: > () Done. https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:923: void RtpFecTest::DeepCopyAndRemoveHalf(const PacketList& src, On 2016/06/29 19:36:18, stefan-webrtc (holmer) wrote: > DeepCopyEveryOtherPacket() is better I think. Or maybe generalize to > DeepCopyEveryNthPacket()? Done. https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/t... File webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc (right): https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc:65: new ForwardErrorCorrection::ReceivedPacket); On 2016/06/29 19:36:18, stefan-webrtc (holmer) wrote: > () Done.
https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:560: while (it_p != protected_packets->end() && it_r != recovered_packets->end()) { On 2016/06/30 14:05:54, brandtr wrote: > On 2016/06/29 19:36:18, stefan-webrtc (holmer) wrote: > > Does this implement intersect? Is it better than calling std::set_intersection > > as before? Should it be broken out to a separate function? > > Yes, these lines correspond to the previous call to std::set_intersection and > the following updating of the recovered packet's pkt member. There should be no > difference in functionality. > > I would've like to keep the previous code, but was unable to figure out how to > do it; the problem is that the types, when wrapped by std::unique_ptr, are not > natively covariant, i.e. the fact that a RecoveredPacket is a SortablePacket > does not mean that a std::unique_ptr<RecoveredPacket> is a > std::unique_ptr<SortablePacket>. This breaks the call to the comparator in > std::set_intersection (see e.g. > http://en.cppreference.com/w/cpp/algorithm/set_intersection). My solution was to > reimplement the set intersection manually and use a parametric comparator (see > row 56). > > I discussed the issue of the invariant std::unique_ptr's with some team members, > and their advice was to reimplement the set intersection manually. I'm happy to > change back to the previous solution, if you have an idea on how to get it to > work :) > > I don't believe that this needs to be it's own function, as this code is only > used in this function, whose sole purpose is to do these operations. I see, didn't think of that issue. I still think a separate function would be good to make the code more readable.
https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:115: for (auto& media_packet : media_packet_list) { const auto& https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:270: const PacketList& media_packet_list, why stress packets are stored as a list? https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:437: RTC_DCHECK(recovered_packet_list->empty()); probably no point to check std container is empty after clear https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:494: for (auto& existing_fec_packet : received_fec_packet_list_) { const auto& https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:555: auto cmp = SortablePacket::LessThan(); may be SortablePacket::LessThan cmp; https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:557: t1 = cmp(*it_p, *it_r); t2 = cmp(*it_r, *it_p); do not put two commands on same line also, do not predeclare t1/t2: bool t1 = cmp(*it_p, *it_r); bool t2 = cmp(*it_r, *it_p); Or, since you have to compare intersection manually anyway, you may be more explicit: uint16_t protected_seq_num = (*it_p)->seq_num; uint16_t recoevered_seq_num = (*it_r)->seq_num; if (protected_seq_num == recoevered_seq_num) { ... } else if (IsNewer(one, another)) { ... } else { ... } https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:601: // (The latter is linked to using the reference-counted |pkt| member.) may be just reduce comment: // Delete the received packet "wrapper". https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:773: RTC_DCHECK(recovered_packet_list->size() <= kMaxMediaPackets); this check looks unnecceray https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:214: class ProtectedPacket; do you need to forward declare them? May be just their declaration here. Though you might move declaration into .cc file. Then this forward declaration would be neccecary. https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:217: typedef std::list<std::unique_ptr<ProtectedPacket>> ProtectedPacketList; prefer using ProtectedPacketList = std::list<std::unique_ptr<ProtectedPacket>>; https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:333: std::vector<Packet> generated_fec_packet_list_; what is motivation for this variable rename? https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/producer_fec.cc (right): https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/producer_fec.cc:255: assert(media_packets_fec_.empty()); remove this assert https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/t... File webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc (right): https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc:429: RTC_DCHECK(media_packet_list.empty()); this DCHECK looks pointless after the simplification https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc:438: RTC_DCHECK(received_packet_list.empty()); Same here
https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2099243003/diff/40001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:560: while (it_p != protected_packets->end() && it_r != recovered_packets->end()) { On 2016/06/30 14:18:35, stefan-webrtc (holmer) wrote: > On 2016/06/30 14:05:54, brandtr wrote: > > On 2016/06/29 19:36:18, stefan-webrtc (holmer) wrote: > > > Does this implement intersect? Is it better than calling > std::set_intersection > > > as before? Should it be broken out to a separate function? > > > > Yes, these lines correspond to the previous call to std::set_intersection and > > the following updating of the recovered packet's pkt member. There should be > no > > difference in functionality. > > > > I would've like to keep the previous code, but was unable to figure out how to > > do it; the problem is that the types, when wrapped by std::unique_ptr, are not > > natively covariant, i.e. the fact that a RecoveredPacket is a SortablePacket > > does not mean that a std::unique_ptr<RecoveredPacket> is a > > std::unique_ptr<SortablePacket>. This breaks the call to the comparator in > > std::set_intersection (see e.g. > > http://en.cppreference.com/w/cpp/algorithm/set_intersection). My solution was > to > > reimplement the set intersection manually and use a parametric comparator (see > > row 56). > > > > I discussed the issue of the invariant std::unique_ptr's with some team > members, > > and their advice was to reimplement the set intersection manually. I'm happy > to > > change back to the previous solution, if you have an idea on how to get it to > > work :) > > > > I don't believe that this needs to be it's own function, as this code is only > > used in this function, whose sole purpose is to do these operations. > > I see, didn't think of that issue. I still think a separate function would be > good to make the code more readable. This would entail moving the whole body of this function to a new function though, since (in the current implementation) the set intersection and the updating of the packet pointers (see row 564) are intertwined. I could decouple the set intersection and pointer updating, thus being able to split out the set intersection into its own function. That would require iterating through the packets in the intersection twice, however.
Patchset #5 (id:100001) has been deleted
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:140001) has been deleted
Patchset #5 (id:160001) has been deleted
https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:115: for (auto& media_packet : media_packet_list) { On 2016/06/30 19:58:37, danilchap wrote: > const auto& Done. https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:270: const PacketList& media_packet_list, On 2016/06/30 19:58:37, danilchap wrote: > why stress packets are stored as a list? I don't see a point actually. I've changed the names accordingly. This will also help if I change the backing data structures in the future. [I had a CL where I changed to std::deque's, but terelius had a good point about my benchmarking not being appropriate, so I scrapped that for now.] https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:437: RTC_DCHECK(recovered_packet_list->empty()); On 2016/06/30 19:58:37, danilchap wrote: > probably no point to check std container is empty after clear Done. https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:494: for (auto& existing_fec_packet : received_fec_packet_list_) { On 2016/06/30 19:58:37, danilchap wrote: > const auto& Done. https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:555: auto cmp = SortablePacket::LessThan(); On 2016/06/30 19:58:37, danilchap wrote: > may be > SortablePacket::LessThan cmp; Done. https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:557: t1 = cmp(*it_p, *it_r); t2 = cmp(*it_r, *it_p); On 2016/06/30 19:58:37, danilchap wrote: > do not put two commands on same line > also, do not predeclare t1/t2: > bool t1 = cmp(*it_p, *it_r); > bool t2 = cmp(*it_r, *it_p); > Or, since you have to compare intersection manually anyway, you may be more > explicit: > uint16_t protected_seq_num = (*it_p)->seq_num; > uint16_t recoevered_seq_num = (*it_r)->seq_num; > if (protected_seq_num == recoevered_seq_num) { > ... > } else if (IsNewer(one, another)) { > ... > } else { > ... > } Changed as according to the first comment. SortablePacket::LessThan is used for sorting in other places, so I think it's nice to reuse it here as well. https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:601: // (The latter is linked to using the reference-counted |pkt| member.) On 2016/06/30 19:58:37, danilchap wrote: > may be just reduce comment: > // Delete the received packet "wrapper". Done. https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:773: RTC_DCHECK(recovered_packet_list->size() <= kMaxMediaPackets); On 2016/06/30 19:58:37, danilchap wrote: > this check looks unnecceray It is, but my feeling was that this check was there to show intent? Btw, this doesn't compile if I change it to the corresponding RTC_DCHECK_LE, do you have any idea why? https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:214: class ProtectedPacket; On 2016/06/30 19:58:37, danilchap wrote: > do you need to forward declare them? May be just their declaration here. > Though you might move declaration into .cc file. Then this forward declaration > would be neccecary. Not strictly necessary, but I think I did it to try to follow the declaration order specified in the style guide. I have now changed order of the declarations, but that means splitting up the two 'using' (former 'typedef') lines into two parts. I moved the declarations of these internal classes to the header file because the compiler was complaining about ReceivedFecPacketList being an incomplete type at row 334, when the declaration was in the .cc file. (I think this might have to do with the std::unique_ptr's, whereas a normal pointer to an incomplete type was OK?) https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:217: typedef std::list<std::unique_ptr<ProtectedPacket>> ProtectedPacketList; On 2016/06/30 19:58:37, danilchap wrote: > prefer > using ProtectedPacketList = std::list<std::unique_ptr<ProtectedPacket>>; Done. https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:333: std::vector<Packet> generated_fec_packet_list_; On 2016/06/30 19:58:37, danilchap wrote: > what is motivation for this variable rename? For symmetry with the name below. I agree with your other comment though, that there is no point in explicitly naming all the variables "xxx_list". https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/producer_fec.cc (right): https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/producer_fec.cc:255: assert(media_packets_fec_.empty()); On 2016/06/30 19:58:37, danilchap wrote: > remove this assert Done. https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/t... File webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc (right): https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc:429: RTC_DCHECK(media_packet_list.empty()); On 2016/06/30 19:58:37, danilchap wrote: > this DCHECK looks pointless after the simplification Removed. https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/t... webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc:438: RTC_DCHECK(received_packet_list.empty()); On 2016/06/30 19:58:37, danilchap wrote: > Same here Done.
Patchset #6 (id:200001) has been deleted
https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:773: RTC_DCHECK(recovered_packet_list->size() <= kMaxMediaPackets); On 2016/07/01 10:45:34, brandtr wrote: > On 2016/06/30 19:58:37, danilchap wrote: > > this check looks unnecceray > > It is, but my feeling was that this check was there to show intent? > Guess you right about intent: DCHECK can be used not just to tell about input parameters, but also to quickly show result of the function. > Btw, this doesn't compile if I change it to the corresponding RTC_DCHECK_LE, do > you have any idea why? I guess using _LE doesn't link. Yes, I know why: DCHECK take parameters by const&, so it tries to get address of the kMaxMediaPackets. Use this trick: https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#the-c... (it didn't work well with const on windows though, but works with constexpr) https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2099243003/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:214: class ProtectedPacket; On 2016/07/01 10:45:34, brandtr wrote: > On 2016/06/30 19:58:37, danilchap wrote: > > do you need to forward declare them? May be just their declaration here. > > Though you might move declaration into .cc file. Then this forward declaration > > would be neccecary. > > Not strictly necessary, but I think I did it to try to follow the declaration > order specified in the style guide. I have now changed order of the > declarations, but that means splitting up the two 'using' (former 'typedef') > lines into two parts. > > I moved the declarations of these internal classes to the header file because > the compiler was complaining about ReceivedFecPacketList being an incomplete > type at row 334, when the declaration was in the .cc file. (I think this might > have to do with the std::unique_ptr's, whereas a normal pointer to an incomplete > type was OK?) Style guide (https://google.github.io/styleguide/cppguide.html#Declaration_Order) tells that using, typedef and enums should go first. member class look like typedef to me (can't find any better category for it), so having member class, then using seems legal. Intermixing them seems legal too, but may be slightly less readable.
https://codereview.webrtc.org/2099243003/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc (right): https://codereview.webrtc.org/2099243003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc:237: for(auto& recovered_packet : recovered_packet_list_) { for (const auto& https://codereview.webrtc.org/2099243003/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/2099243003/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:67: void DeepCopyEveryNthPacket(const PacketList& src, PacketList* dst, int N); int n https://codereview.webrtc.org/2099243003/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:914: PacketList* dst, int N) { int n here too
Addressed feedback. https://codereview.webrtc.org/2099243003/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc (right): https://codereview.webrtc.org/2099243003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_receiver_impl.cc:237: for(auto& recovered_packet : recovered_packet_list_) { On 2016/07/05 08:51:38, stefan-webrtc (holmer) wrote: > for (const auto& Done. https://codereview.webrtc.org/2099243003/diff/220001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/2099243003/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:67: void DeepCopyEveryNthPacket(const PacketList& src, PacketList* dst, int N); On 2016/07/05 08:51:38, stefan-webrtc (holmer) wrote: > int n Done. https://codereview.webrtc.org/2099243003/diff/220001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:914: PacketList* dst, int N) { On 2016/07/05 08:51:38, stefan-webrtc (holmer) wrote: > int n here too Done.
Description was changed from ========== Use std::unique_ptr in ForwardErrorCorrection. This CL converts the ForwardErrorCorrection class (i.e. the parity erasure code) to use std::unique_ptr for memory management, rather than manually delete'ing allocated memory. It further renames some data structures and types to distinguish between generated FEC packets (i.e. coming from the encode operation) and received FEC packets (i.e. coming in over the wire, intended for the decode operation). BUG=webrtc:5654 ========== to ========== Use std::unique_ptr in ForwardErrorCorrection. This CL converts the ForwardErrorCorrection class to use std::unique_ptr for memory management, rather than manually delete'ing allocated memory. It further renames some data structures and types to distinguish between generated FEC packets (i.e. coming from the encode operation) and received FEC packets (i.e. coming in over the wire, intended for the decode operation). BUG=webrtc:5654 ==========
https://codereview.webrtc.org/2099243003/diff/300001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2099243003/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:554: SortablePacket::LessThan cmp; less might be better name: cmp (compare) usually return 3 values (0 when equal, negative when less, positive when more), but this functor can only check if one is less than another. https://codereview.webrtc.org/2099243003/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:558: if (!t1 && !t2) { may be this if switch would be clearer without temporary variables: while(...) { if (less(*it_p, *it_r) { // Comment '*it_p < *it_r' looks redundant then and can be removed ... } else if (less(*it_r, *it_p) { ... } else { // Equal. ... } } https://codereview.webrtc.org/2099243003/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:561: ++it_p; ++it_r; split this line into two. https://codereview.webrtc.org/2099243003/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:772: RTC_DCHECK(recovered_packets->size() <= kMaxMediaPackets); RTC_DCHECK_LE (add line constexpr size_t ForwardErrorCorrection::kMaxMediaPackets; near other constexpr in this file to make it work) https://codereview.webrtc.org/2099243003/diff/300001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/2099243003/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:793: using PacketRef = this is not a Reference to Packet, it is a Reference to Pointer to Packet. May be name PacketPtr would be slightly less confusing? using PacketPtr = std::unique_ptr<ForwardErrorCorrection::Packet>; using RecoveredPacketPtr = ...; auto cmp = [](const PacketPtr& media_packet, const RecoveredPacketPtr& recovered packet) { Though with usage so local it is hardly confusing with any name. https://codereview.webrtc.org/2099243003/diff/300001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc (right): https://codereview.webrtc.org/2099243003/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc:292: for (auto& media_packet : media_packet_list) { possible to add const? https://codereview.webrtc.org/2099243003/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc:390: for (auto& received_packet : to_decode_list) { const auto& https://codereview.webrtc.org/2099243003/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc:403: for (auto& media_packet : media_packet_list) { const auto&
https://codereview.webrtc.org/2099243003/diff/300001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc (right): https://codereview.webrtc.org/2099243003/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:59: std::unique_ptr<ForwardErrorCorrection::Packet>( Should we std::move here too? https://codereview.webrtc.org/2099243003/diff/300001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/producer_fec.cc (right): https://codereview.webrtc.org/2099243003/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/producer_fec.cc:253: void ProducerFec::DeletePackets() { Rename to ClearMediaPackets() ?
Patchset #11 (id:320001) has been deleted
Patchset #12 (id:360001) has been deleted
Thanks for the feedback; comments have been addressed. Android and libfuzzer failures should be independent of this CL. https://codereview.webrtc.org/2099243003/diff/300001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc (right): https://codereview.webrtc.org/2099243003/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:59: std::unique_ptr<ForwardErrorCorrection::Packet>( On 2016/07/20 09:50:35, stefan-webrtc (holmer) wrote: > Should we std::move here too? I changed to emplace_back. The RawRtpPackets are created by FrameGenerator::NextPacket, but deleted by the std::unique_ptr in |media_packets|. (A RawRtpPacket is a Packet.) Recall that before this change, we manually delete'd the Packets in |media_packets| at the end of each test (see, e.g., https://codereview.webrtc.org/2099243003/diff/340001/webrtc/modules/rtp_rtcp/...). https://codereview.webrtc.org/2099243003/diff/300001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2099243003/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:554: SortablePacket::LessThan cmp; On 2016/07/19 12:58:36, danilchap wrote: > less might be better name: cmp (compare) usually return 3 values (0 when equal, > negative when less, positive when more), but this functor can only check if one > is less than another. Done. https://codereview.webrtc.org/2099243003/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:558: if (!t1 && !t2) { On 2016/07/19 12:58:37, danilchap wrote: > may be this if switch would be clearer without temporary variables: > while(...) { > if (less(*it_p, *it_r) { // Comment '*it_p < *it_r' looks redundant then and can > be removed > ... > } else if (less(*it_r, *it_p) { > ... > } else { // Equal. > ... > } > } Yes, this is a nicer way to do it. Thanks! https://codereview.webrtc.org/2099243003/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:561: ++it_p; ++it_r; On 2016/07/19 12:58:37, danilchap wrote: > split this line into two. Done. https://codereview.webrtc.org/2099243003/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:772: RTC_DCHECK(recovered_packets->size() <= kMaxMediaPackets); On 2016/07/19 12:58:37, danilchap wrote: > RTC_DCHECK_LE (add line > constexpr size_t ForwardErrorCorrection::kMaxMediaPackets; > near other constexpr in this file to make it work) Great tip, thanks! https://codereview.webrtc.org/2099243003/diff/300001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/producer_fec.cc (right): https://codereview.webrtc.org/2099243003/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/producer_fec.cc:253: void ProducerFec::DeletePackets() { On 2016/07/20 09:50:35, stefan-webrtc (holmer) wrote: > Rename to ClearMediaPackets() ? Sort of done here: https://codereview.webrtc.org/2110763002/diff/280001/webrtc/modules/rtp_rtcp/... (Perhaps I made TOO granular CLs, for all of these semi-related style changes.) I can change it to "Clear" rather than "Delete" there, if you find that better? https://codereview.webrtc.org/2099243003/diff/300001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/2099243003/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:793: using PacketRef = On 2016/07/19 12:58:37, danilchap wrote: > this is not a Reference to Packet, it is a Reference to Pointer to Packet. May > be name PacketPtr would be slightly less confusing? > using PacketPtr = std::unique_ptr<ForwardErrorCorrection::Packet>; > using RecoveredPacketPtr = ...; > auto cmp = [](const PacketPtr& media_packet, > const RecoveredPacketPtr& recovered packet) { > > Though with usage so local it is hardly confusing with any name. > Yes, that is nicer. Thanks :) https://codereview.webrtc.org/2099243003/diff/300001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc (right): https://codereview.webrtc.org/2099243003/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc:292: for (auto& media_packet : media_packet_list) { On 2016/07/19 12:58:37, danilchap wrote: > possible to add const? Yep, done! https://codereview.webrtc.org/2099243003/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc:390: for (auto& received_packet : to_decode_list) { On 2016/07/19 12:58:37, danilchap wrote: > const auto& Done. https://codereview.webrtc.org/2099243003/diff/300001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc:403: for (auto& media_packet : media_packet_list) { On 2016/07/19 12:58:37, danilchap wrote: > const auto& Done.
https://codereview.webrtc.org/2099243003/diff/380001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc (right): https://codereview.webrtc.org/2099243003/diff/380001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:56: media_rtp_packets->push_back( may be memory management would be less confusing if you do not call back() function to get an element that was just added: for(...) { std::unique_ptr<test::RawRtpPacket> next_packet(generator_->NextPacket(...)); media_rtp_packets->push_back(next_packet.get()); media_packets->push_back(std::move(next_packet)); } https://codereview.webrtc.org/2099243003/diff/380001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2099243003/diff/380001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:65: class LessThan { from google c++ style: "For consistency with STL, you can use struct instead of class for functors and traits." (though 'can' doesn't mean you 'must') https://codereview.webrtc.org/2099243003/diff/380001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/2099243003/diff/380001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:914: void RtpFecTest::DeepCopyEveryNthPacket(const PacketList& src, Reorder n before dst (e.g. put n 1st): "When defining a function, parameter order is: inputs, then outputs." https://codereview.webrtc.org/2099243003/diff/380001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:918: for (auto& packet : src) { const auto&
https://codereview.webrtc.org/2099243003/diff/380001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc (right): https://codereview.webrtc.org/2099243003/diff/380001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:56: media_rtp_packets->push_back( On 2016/07/21 13:06:30, danilchap wrote: > may be memory management would be less confusing if you do not call back() > function to get an element that was just added: > for(...) { > std::unique_ptr<test::RawRtpPacket> next_packet(generator_->NextPacket(...)); > media_rtp_packets->push_back(next_packet.get()); > media_packets->push_back(std::move(next_packet)); > } Good idea, thanks! https://codereview.webrtc.org/2099243003/diff/380001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2099243003/diff/380001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:65: class LessThan { On 2016/07/21 13:06:30, danilchap wrote: > from google c++ style: "For consistency with STL, you can use struct instead of > class for functors and traits." > (though 'can' doesn't mean you 'must') Cool, didn't know that functors as structs was the way to go in STL land :) https://codereview.webrtc.org/2099243003/diff/380001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/2099243003/diff/380001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:914: void RtpFecTest::DeepCopyEveryNthPacket(const PacketList& src, On 2016/07/21 13:06:30, danilchap wrote: > Reorder n before dst (e.g. put n 1st): > "When defining a function, parameter order is: inputs, then outputs." Done. https://codereview.webrtc.org/2099243003/diff/380001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:918: for (auto& packet : src) { On 2016/07/21 13:06:30, danilchap wrote: > const auto& Done.
lgtm % nit https://codereview.webrtc.org/2099243003/diff/400001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2099243003/diff/400001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:66: public: the advantage of struct over class (for small classes) that method(s) are public by default. i.e. this line can be removed. https://codereview.webrtc.org/2099243003/diff/400001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/2099243003/diff/400001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:794: const std::unique_ptr<ForwardErrorCorrection::Packet>; const here looks meaningless, likely better without it. (you may try write using PacketPtr = std::unque_ptr<const ForwardErrorCorrection::Packet> to stress no data beyound pointer is changed.) const added when PacketPtr/RecoveredPacketPtr are used, is likely all you meant.
https://codereview.webrtc.org/2099243003/diff/400001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2099243003/diff/400001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:66: public: On 2016/07/22 12:14:03, danilchap wrote: > the advantage of struct over class (for small classes) that method(s) are public > by default. i.e. this line can be removed. Done. https://codereview.webrtc.org/2099243003/diff/400001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/2099243003/diff/400001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:794: const std::unique_ptr<ForwardErrorCorrection::Packet>; On 2016/07/22 12:14:03, danilchap wrote: > const here looks meaningless, likely better without it. > (you may try write > using PacketPtr = std::unque_ptr<const ForwardErrorCorrection::Packet> to stress > no data beyound pointer is changed.) > > const added when PacketPtr/RecoveredPacketPtr are used, is likely all you meant. Yep, that's what I meant :) Removed the const however. Using std::unique_ptr<const ...> doesn't work however :/
https://codereview.webrtc.org/2099243003/diff/420001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2099243003/diff/420001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:557: SortablePacket::LessThan less_than; I might be mistaking myself, but if the function was static you wouldn't need this instantiation, right? https://codereview.webrtc.org/2099243003/diff/420001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2099243003/diff/420001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:67: bool operator() (const S& first, const T& second); This should be static, right? https://codereview.webrtc.org/2099243003/diff/420001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc (right): https://codereview.webrtc.org/2099243003/diff/420001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc:918: if (i % n == 0) { Remove {}
https://codereview.webrtc.org/2099243003/diff/420001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2099243003/diff/420001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:67: bool operator() (const S& first, const T& second); On 2016/08/01 11:29:51, stefan-webrtc (holmer) wrote: > This should be static, right? it wouldn't compile: error: overloaded 'operator()' cannot be a static member function
https://codereview.webrtc.org/2099243003/diff/420001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.h (right): https://codereview.webrtc.org/2099243003/diff/420001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.h:67: bool operator() (const S& first, const T& second); On 2016/08/01 13:29:07, danilchap wrote: > On 2016/08/01 11:29:51, stefan-webrtc (holmer) wrote: > > This should be static, right? > > it wouldn't compile: > error: overloaded 'operator()' cannot be a static member function Good point. But maybe we can make it a static function instead? Or does it have to be a functor?
https://codereview.webrtc.org/2099243003/diff/420001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): https://codereview.webrtc.org/2099243003/diff/420001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:474: recovered_packets->sort(SortablePacket::LessThan()); here SortablePacket::LessThan has to be a functor.
On 2016/08/01 14:39:24, danilchap wrote: > https://codereview.webrtc.org/2099243003/diff/420001/webrtc/modules/rtp_rtcp/... > File webrtc/modules/rtp_rtcp/source/forward_error_correction.cc (right): > > https://codereview.webrtc.org/2099243003/diff/420001/webrtc/modules/rtp_rtcp/... > webrtc/modules/rtp_rtcp/source/forward_error_correction.cc:474: > recovered_packets->sort(SortablePacket::LessThan()); > here SortablePacket::LessThan has to be a functor. In that case, lgtm
The CQ bit was checked by brandtr@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org Link to the patchset: https://codereview.webrtc.org/2099243003/#ps420001 (title: "Final nit from danilchap.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Use std::unique_ptr in ForwardErrorCorrection. This CL converts the ForwardErrorCorrection class to use std::unique_ptr for memory management, rather than manually delete'ing allocated memory. It further renames some data structures and types to distinguish between generated FEC packets (i.e. coming from the encode operation) and received FEC packets (i.e. coming in over the wire, intended for the decode operation). BUG=webrtc:5654 ========== to ========== Use std::unique_ptr in ForwardErrorCorrection. This CL converts the ForwardErrorCorrection class to use std::unique_ptr for memory management, rather than manually delete'ing allocated memory. It further renames some data structures and types to distinguish between generated FEC packets (i.e. coming from the encode operation) and received FEC packets (i.e. coming in over the wire, intended for the decode operation). BUG=webrtc:5654 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:420001)
Message was sent while issue was closed.
Description was changed from ========== Use std::unique_ptr in ForwardErrorCorrection. This CL converts the ForwardErrorCorrection class to use std::unique_ptr for memory management, rather than manually delete'ing allocated memory. It further renames some data structures and types to distinguish between generated FEC packets (i.e. coming from the encode operation) and received FEC packets (i.e. coming in over the wire, intended for the decode operation). BUG=webrtc:5654 ========== to ========== Use std::unique_ptr in ForwardErrorCorrection. This CL converts the ForwardErrorCorrection class to use std::unique_ptr for memory management, rather than manually delete'ing allocated memory. It further renames some data structures and types to distinguish between generated FEC packets (i.e. coming from the encode operation) and received FEC packets (i.e. coming in over the wire, intended for the decode operation). BUG=webrtc:5654 Committed: https://crrev.com/35c480cf18122a6533c3c934d8794049f97ffb09 Cr-Commit-Position: refs/heads/master@{#13687} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/35c480cf18122a6533c3c934d8794049f97ffb09 Cr-Commit-Position: refs/heads/master@{#13687} |
