Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2271273004:
Generalize UlpfecPacketGenerator into AugmentedPacketGenerator. (pt. 8) (Closed)
|
Created:
4 years, 3 months ago by brandtr Modified:
4 years, 2 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, danilchap, stefan-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@style_fixes_fec_receiver_producer_fec Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionGeneralize UlpfecPacketGenerator into AugmentedPacketGenerator.
Main changes:
- Split out general functionality from UlpfecPacketGenerator
into a new class AugmentedPacketGenerator. This will allow
for the addition of a FlexfecPacketGenerator that inherits
from AugmentedPacketGenerator.
- Rename RawRtpPacket to AugmentedPacket. This name is more
reflective of its purpose, i.e., an FEC packet with an
additional WebRtcRTPHeader.
- Return std::unique_ptr's instead of raw pointers.
Minor changes:
- Update names, based on RawRtpPacket -> AugmentedPacket name
change, in FEC unit tests.
- Rename |generator_| to |packet_generator_|, in FEC unit tests.
- Change some int's to size_t's in the packet generator classes.
- Use std::unique_ptr in ProducerFec unittest.
No functionality or performance changes are expected
due to this CL.
BUG=webrtc:5654
Committed: https://crrev.com/0aabdac7431b578ad8dbec73c1bc5ca5d9a779a0
Cr-Commit-Position: refs/heads/master@{#14477}
Patch Set 1 : Rebase. #
Total comments: 11
Patch Set 2 : Review response 1. #
Total comments: 4
Patch Set 3 : Curly braces... #Patch Set 4 : Rebase. #Patch Set 5 : Rebase. #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 33 (16 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Rebase.
Description was changed from ========== Generalize UlpfecPacketGenerator. BUG=webrtc:5654 ========== to ========== Generalize UlpfecPacketGenerator into AugmentedPacketGenerator. This is in preparation for a new subclass, FlexfecPacketGenerator. BUG=webrtc:5654 ==========
Rebase
Rebase.
Patchset #2 (id:60001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Generalize UlpfecPacketGenerator into AugmentedPacketGenerator. This is in preparation for a new subclass, FlexfecPacketGenerator. BUG=webrtc:5654 ========== to ========== Generalize UlpfecPacketGenerator into AugmentedPacketGenerator. Main changes: - Split out general functionality from UlpfecPacketGenerator into a new class AugmentedPacketGenerator. This will allow for the addition of a FlexfecPacketGenerator that inherits from AugmentedPacketGenerator. - Rename RawRtpPacket to AugmentedPacket. This name is more reflective of its purpose, i.e., an FEC packet with an additional WebRtcRTPHeader. - Return std::unique_ptr's instead of raw pointers. Minor changes: - Use std::unique_ptr in ProducerFec unittest. - Rename |generator_| to |packet_generator_| in FEC unit tests. No functionality or performance changes are expected due to this CL. BUG=webrtc:5654 ==========
brandtr@webrtc.org changed reviewers: + philipel@webrtc.org, stefan@webrtc.org
Description was changed from ========== Generalize UlpfecPacketGenerator into AugmentedPacketGenerator. Main changes: - Split out general functionality from UlpfecPacketGenerator into a new class AugmentedPacketGenerator. This will allow for the addition of a FlexfecPacketGenerator that inherits from AugmentedPacketGenerator. - Rename RawRtpPacket to AugmentedPacket. This name is more reflective of its purpose, i.e., an FEC packet with an additional WebRtcRTPHeader. - Return std::unique_ptr's instead of raw pointers. Minor changes: - Use std::unique_ptr in ProducerFec unittest. - Rename |generator_| to |packet_generator_| in FEC unit tests. No functionality or performance changes are expected due to this CL. BUG=webrtc:5654 ========== to ========== Generalize UlpfecPacketGenerator into AugmentedPacketGenerator. Main changes: - Split out general functionality from UlpfecPacketGenerator into a new class AugmentedPacketGenerator. This will allow for the addition of a FlexfecPacketGenerator that inherits from AugmentedPacketGenerator. - Rename RawRtpPacket to AugmentedPacket. This name is more reflective of its purpose, i.e., an FEC packet with an additional WebRtcRTPHeader. - Return std::unique_ptr's instead of raw pointers. Minor changes: - Update names, based on RawRtpPacket -> AugmentedPacket name change, in FEC unit tests. - Rename |generator_| to |packet_generator_|, in FEC unit tests. - Use std::unique_ptr in ProducerFec unittest. No functionality or performance changes are expected due to this CL. BUG=webrtc:5654 ==========
Description was changed from ========== Generalize UlpfecPacketGenerator into AugmentedPacketGenerator. Main changes: - Split out general functionality from UlpfecPacketGenerator into a new class AugmentedPacketGenerator. This will allow for the addition of a FlexfecPacketGenerator that inherits from AugmentedPacketGenerator. - Rename RawRtpPacket to AugmentedPacket. This name is more reflective of its purpose, i.e., an FEC packet with an additional WebRtcRTPHeader. - Return std::unique_ptr's instead of raw pointers. Minor changes: - Update names, based on RawRtpPacket -> AugmentedPacket name change, in FEC unit tests. - Rename |generator_| to |packet_generator_|, in FEC unit tests. - Use std::unique_ptr in ProducerFec unittest. No functionality or performance changes are expected due to this CL. BUG=webrtc:5654 ========== to ========== Generalize UlpfecPacketGenerator into AugmentedPacketGenerator. Main changes: - Split out general functionality from UlpfecPacketGenerator into a new class AugmentedPacketGenerator. This will allow for the addition of a FlexfecPacketGenerator that inherits from AugmentedPacketGenerator. - Rename RawRtpPacket to AugmentedPacket. This name is more reflective of its purpose, i.e., an FEC packet with an additional WebRtcRTPHeader. - Return std::unique_ptr's instead of raw pointers. Minor changes: - Update names, based on RawRtpPacket -> AugmentedPacket name change, in FEC unit tests. - Rename |generator_| to |packet_generator_|, in FEC unit tests. - Change some int's to size_t's in the packet generator classes. - Use std::unique_ptr in ProducerFec unittest. No functionality or performance changes are expected due to this CL. BUG=webrtc:5654 ==========
Rebase.
Patchset #1 (id:120001) has been deleted
PTAL at this "support CL", which refactors some helper code used in the FEC unit tests. This CL is dependent on previous CLs in Rietveld, but that's more of a technical dependency rather than an actual dependency.
https://codereview.webrtc.org/2271273004/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/fec_test_helper.cc (right): https://codereview.webrtc.org/2271273004/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_test_helper.cc:109: for (size_t i = 0; i < length; ++i) { remove {} https://codereview.webrtc.org/2271273004/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/fec_test_helper.h (right): https://codereview.webrtc.org/2271273004/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_test_helper.h:72: // Increment and return the sequence number. // Increment and returns the newly incremented sequence number. https://codereview.webrtc.org/2271273004/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_test_helper.h:73: uint16_t NextSeqNum(); I think NextPacketSeqNum might be a better name in this case, WDYT? https://codereview.webrtc.org/2271273004/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_test_helper.h:82: size_t num_packets_; // Number of packets to generate. Move comment above |num_packets_| https://codereview.webrtc.org/2271273004/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc (right): https://codereview.webrtc.org/2271273004/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc:31: using ::webrtc::test::fec::AugmentedPacket; remove ::webrtc::
https://codereview.webrtc.org/2271273004/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/fec_test_helper.cc (right): https://codereview.webrtc.org/2271273004/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_test_helper.cc:109: for (size_t i = 0; i < length; ++i) { On 2016/09/22 11:29:30, philipel wrote: > remove {} Do I have to? :( https://codereview.webrtc.org/2271273004/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/fec_test_helper.h (right): https://codereview.webrtc.org/2271273004/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_test_helper.h:72: // Increment and return the sequence number. On 2016/09/22 11:29:30, philipel wrote: > // Increment and returns the newly incremented sequence number. Done. https://codereview.webrtc.org/2271273004/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_test_helper.h:73: uint16_t NextSeqNum(); On 2016/09/22 11:29:30, philipel wrote: > I think NextPacketSeqNum might be a better name in this case, WDYT? Yep, that works. https://codereview.webrtc.org/2271273004/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_test_helper.h:82: size_t num_packets_; // Number of packets to generate. On 2016/09/22 11:29:30, philipel wrote: > Move comment above |num_packets_| Done. https://codereview.webrtc.org/2271273004/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc (right): https://codereview.webrtc.org/2271273004/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc:31: using ::webrtc::test::fec::AugmentedPacket; On 2016/09/22 11:29:30, philipel wrote: > remove ::webrtc:: Already done in upstream CL: https://codereview.webrtc.org/2273353002/diff/180001/webrtc/modules/rtp_rtcp/...
lgtm with nits https://codereview.webrtc.org/2271273004/diff/140001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/fec_test_helper.cc (right): https://codereview.webrtc.org/2271273004/diff/140001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_test_helper.cc:109: for (size_t i = 0; i < length; ++i) { On 2016/09/22 14:21:38, brandtr wrote: > On 2016/09/22 11:29:30, philipel wrote: > > remove {} > > Do I have to? :( Yes, also remove from for loop at line 67 :) https://codereview.webrtc.org/2271273004/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc (right): https://codereview.webrtc.org/2271273004/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:28: using ::testing::_; Using should be outside the anonymous namespace.
https://codereview.webrtc.org/2271273004/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc (right): https://codereview.webrtc.org/2271273004/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:28: using ::testing::_; On 2016/09/23 11:52:14, philipel wrote: > Using should be outside the anonymous namespace. why? I have strong reason to believe it should be inside.
https://codereview.webrtc.org/2271273004/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc (right): https://codereview.webrtc.org/2271273004/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:28: using ::testing::_; On 2016/09/23 12:41:47, danilchap wrote: > On 2016/09/23 11:52:14, philipel wrote: > > Using should be outside the anonymous namespace. > > why? I have strong reason to believe it should be inside. Just looking through the codebase I can't find any other place where using statements are inside an anonymous namespace, but maybe there is a good reason to have them inside the namespace?
https://codereview.webrtc.org/2271273004/diff/160001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc (right): https://codereview.webrtc.org/2271273004/diff/160001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:28: using ::testing::_; On 2016/09/23 13:14:41, philipel wrote: > On 2016/09/23 12:41:47, danilchap wrote: > > On 2016/09/23 11:52:14, philipel wrote: > > > Using should be outside the anonymous namespace. > > > > why? I have strong reason to believe it should be inside. > > Just looking through the codebase I can't find any other place where using > statements are inside an anonymous namespace, but maybe there is a good reason > to have them inside the namespace? After some offline discussion with Danil I now think the using statements should stay inside the anonymous namespace. :)
lgtm
Rebase.
Rebase.
The CQ bit was checked by brandtr@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2271273004/#ps220001 (title: "Rebase.")
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 ========== Generalize UlpfecPacketGenerator into AugmentedPacketGenerator. Main changes: - Split out general functionality from UlpfecPacketGenerator into a new class AugmentedPacketGenerator. This will allow for the addition of a FlexfecPacketGenerator that inherits from AugmentedPacketGenerator. - Rename RawRtpPacket to AugmentedPacket. This name is more reflective of its purpose, i.e., an FEC packet with an additional WebRtcRTPHeader. - Return std::unique_ptr's instead of raw pointers. Minor changes: - Update names, based on RawRtpPacket -> AugmentedPacket name change, in FEC unit tests. - Rename |generator_| to |packet_generator_|, in FEC unit tests. - Change some int's to size_t's in the packet generator classes. - Use std::unique_ptr in ProducerFec unittest. No functionality or performance changes are expected due to this CL. BUG=webrtc:5654 ========== to ========== Generalize UlpfecPacketGenerator into AugmentedPacketGenerator. Main changes: - Split out general functionality from UlpfecPacketGenerator into a new class AugmentedPacketGenerator. This will allow for the addition of a FlexfecPacketGenerator that inherits from AugmentedPacketGenerator. - Rename RawRtpPacket to AugmentedPacket. This name is more reflective of its purpose, i.e., an FEC packet with an additional WebRtcRTPHeader. - Return std::unique_ptr's instead of raw pointers. Minor changes: - Update names, based on RawRtpPacket -> AugmentedPacket name change, in FEC unit tests. - Rename |generator_| to |packet_generator_|, in FEC unit tests. - Change some int's to size_t's in the packet generator classes. - Use std::unique_ptr in ProducerFec unittest. No functionality or performance changes are expected due to this CL. BUG=webrtc:5654 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Generalize UlpfecPacketGenerator into AugmentedPacketGenerator. Main changes: - Split out general functionality from UlpfecPacketGenerator into a new class AugmentedPacketGenerator. This will allow for the addition of a FlexfecPacketGenerator that inherits from AugmentedPacketGenerator. - Rename RawRtpPacket to AugmentedPacket. This name is more reflective of its purpose, i.e., an FEC packet with an additional WebRtcRTPHeader. - Return std::unique_ptr's instead of raw pointers. Minor changes: - Update names, based on RawRtpPacket -> AugmentedPacket name change, in FEC unit tests. - Rename |generator_| to |packet_generator_|, in FEC unit tests. - Change some int's to size_t's in the packet generator classes. - Use std::unique_ptr in ProducerFec unittest. No functionality or performance changes are expected due to this CL. BUG=webrtc:5654 ========== to ========== Generalize UlpfecPacketGenerator into AugmentedPacketGenerator. Main changes: - Split out general functionality from UlpfecPacketGenerator into a new class AugmentedPacketGenerator. This will allow for the addition of a FlexfecPacketGenerator that inherits from AugmentedPacketGenerator. - Rename RawRtpPacket to AugmentedPacket. This name is more reflective of its purpose, i.e., an FEC packet with an additional WebRtcRTPHeader. - Return std::unique_ptr's instead of raw pointers. Minor changes: - Update names, based on RawRtpPacket -> AugmentedPacket name change, in FEC unit tests. - Rename |generator_| to |packet_generator_|, in FEC unit tests. - Change some int's to size_t's in the packet generator classes. - Use std::unique_ptr in ProducerFec unittest. No functionality or performance changes are expected due to this CL. BUG=webrtc:5654 Committed: https://crrev.com/0aabdac7431b578ad8dbec73c1bc5ca5d9a779a0 Cr-Commit-Position: refs/heads/master@{#14477} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/0aabdac7431b578ad8dbec73c1bc5ca5d9a779a0 Cr-Commit-Position: refs/heads/master@{#14477} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
