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

Issue 2267393002: Generalize FEC unit tests and rename GenerateFec. (pt. 3) (Closed)

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

Description

Generalize FEC unit tests and rename GenerateFec. - Rename GenerateFec -> EncodeFec in ForwardErrorCorrection. This naming is more consistent with DecodeFec. - Add appropriate using directives, to reduce clutter in tests. - Move ConstructMediaPackets to fec_test_helper.{h,cc}. This will help future tests of ULPFEC/FlexFEC header formatters. - Generalize tests in rtp_fec_unittest.cc to typed tests. This will help testing ForwardErrorCorrection with both ULPFEC and FlexFEC. This CL should not impact functionality or performance. BUG=webrtc:5654 Committed: https://crrev.com/ece4aba64ed873c54ecbcb7ba92bf6ed2fcb9d98 Cr-Commit-Position: refs/heads/master@{#14314}

Patch Set 1 #

Patch Set 2 : Move using directives and unnamed namespaces into webrtc namespace. #

Patch Set 3 : Rebase. #

Total comments: 32

Patch Set 4 : Review response + 'git cl format'. #

Total comments: 2

Patch Set 5 : Review response 2. #

Patch Set 6 : Rebase fix. #

Patch Set 7 : Rebase. #

Patch Set 8 : Rebase. #

Total comments: 6

Patch Set 9 : Review response 3. #

Patch Set 10 : Rebase after gyp deprecation. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+685 lines, -556 lines) Patch
M webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc View 1 2 3 4 14 chunks +52 lines, -50 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/fec_test_helper.h View 1 2 3 4 5 6 7 8 3 chunks +42 lines, -9 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/fec_test_helper.cc View 1 2 3 4 5 6 7 8 5 chunks +83 lines, -8 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/forward_error_correction.h View 1 chunk +7 lines, -5 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/forward_error_correction.cc View 1 2 1 chunk +10 lines, -11 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/producer_fec.cc View 1 2 3 4 5 2 chunks +5 lines, -5 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/producer_fec_unittest.cc View 1 2 3 4 5 5 chunks +16 lines, -11 lines 0 comments Download
M webrtc/modules/rtp_rtcp/source/rtp_fec_unittest.cc View 1 2 3 4 5 6 7 8 13 chunks +458 lines, -450 lines 0 comments Download
M webrtc/modules/rtp_rtcp/test/testFec/test_fec.cc View 2 chunks +7 lines, -7 lines 0 comments Download
M webrtc/test/fuzzers/producer_fec_fuzzer.cc View 1 2 3 1 chunk +5 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 36 (14 generated)
brandtr
Split up previous large CL in five semi-indenpendent ones.
4 years, 4 months ago (2016-08-23 08:18:46 UTC) #5
brandtr
Moved using directives and unnamed namespaces into webrtc namespace.
4 years, 4 months ago (2016-08-23 09:45:03 UTC) #6
danilchap
https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc File webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc (right): https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc#newcode36 webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:36: using Packet = ForwardErrorCorrection::Packet; use full name here too, ...
4 years, 4 months ago (2016-08-23 13:08:50 UTC) #7
brandtr
https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc File webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc (right): https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc#newcode36 webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:36: using Packet = ForwardErrorCorrection::Packet; On 2016/08/23 13:08:49, danilchap wrote: ...
4 years, 4 months ago (2016-08-23 14:05:07 UTC) #10
danilchap
lgtm % nit https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc File webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc (right): https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc#newcode37 webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:37: using RawRtpPacket = test::fec::RawRtpPacket; On 2016/08/23 ...
4 years, 4 months ago (2016-08-23 14:51:19 UTC) #11
brandtr
Thanks for quick feedback! https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc File webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc (right): https://codereview.webrtc.org/2267393002/diff/40001/webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc#newcode37 webrtc/modules/rtp_rtcp/source/fec_receiver_unittest.cc:37: using RawRtpPacket = test::fec::RawRtpPacket; On ...
4 years, 4 months ago (2016-08-23 15:08:01 UTC) #12
brandtr
Rebase.
4 years, 3 months ago (2016-09-02 08:25:41 UTC) #13
brandtr
Rebase.
4 years, 3 months ago (2016-09-06 14:35:20 UTC) #14
stefan-webrtc
lgtm https://codereview.webrtc.org/2267393002/diff/180001/webrtc/modules/rtp_rtcp/source/fec_test_helper.cc File webrtc/modules/rtp_rtcp/source/fec_test_helper.cc (right): https://codereview.webrtc.org/2267393002/diff/180001/webrtc/modules/rtp_rtcp/source/fec_test_helper.cc#newcode109 webrtc/modules/rtp_rtcp/source/fec_test_helper.cc:109: MediaPacketGenerator::ConstructMediaPacketsSeqNum(int num_media_packets, I would probably have ommitted the ...
4 years, 3 months ago (2016-09-13 07:39:47 UTC) #15
stefan-webrtc
lgtm
4 years, 3 months ago (2016-09-13 07:39:50 UTC) #16
stefan-webrtc
Sorry, clicked the wrong button. not lgtm for now :)
4 years, 3 months ago (2016-09-13 07:40:06 UTC) #17
brandtr
https://codereview.webrtc.org/2267393002/diff/180001/webrtc/modules/rtp_rtcp/source/fec_test_helper.cc File webrtc/modules/rtp_rtcp/source/fec_test_helper.cc (right): https://codereview.webrtc.org/2267393002/diff/180001/webrtc/modules/rtp_rtcp/source/fec_test_helper.cc#newcode109 webrtc/modules/rtp_rtcp/source/fec_test_helper.cc:109: MediaPacketGenerator::ConstructMediaPacketsSeqNum(int num_media_packets, On 2016/09/13 07:39:47, stefan-webrtc (holmer) wrote: > ...
4 years, 3 months ago (2016-09-13 09:27:14 UTC) #18
brandtr
ping holmer.
4 years, 3 months ago (2016-09-20 09:16:51 UTC) #19
brandtr
Rebase after gyp deprecation.
4 years, 3 months ago (2016-09-20 09:23:50 UTC) #20
stefan-webrtc
lgtm
4 years, 3 months ago (2016-09-20 09:39:59 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2267393002/220001
4 years, 3 months ago (2016-09-20 10:51:00 UTC) #24
commit-bot: I haz the power
Exceeded global retry quota
4 years, 3 months ago (2016-09-20 12:51:36 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2267393002/220001
4 years, 3 months ago (2016-09-20 13:41:35 UTC) #28
commit-bot: I haz the power
Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL) android_dbg on ...
4 years, 3 months ago (2016-09-20 13:59:46 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/2267393002/220001
4 years, 3 months ago (2016-09-21 06:07:24 UTC) #32
commit-bot: I haz the power
Committed patchset #10 (id:220001)
4 years, 3 months ago (2016-09-21 06:16:32 UTC) #34
commit-bot: I haz the power
4 years, 3 months ago (2016-09-21 06:16:43 UTC) #36
Message was sent while issue was closed.
Patchset 10 (id:??) landed as
https://crrev.com/ece4aba64ed873c54ecbcb7ba92bf6ed2fcb9d98
Cr-Commit-Position: refs/heads/master@{#14314}

Powered by Google App Engine
This is Rietveld 408576698