Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
3004553002:
When Ulpfec recovers a packet, set |returned| flag earlier. (Closed)
|
Created:
3 years, 3 months ago by nisse-webrtc Modified:
3 years, 3 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, danilchap, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionWhen Ulpfec recovers a packet, set |returned| flag earlier.
This avoids infinite recursion in case the recovered packet carries a
RED header.
BUG=chromium:754748
Review-Url: https://codereview.webrtc.org/3004553002
Cr-Commit-Position: refs/heads/master@{#19525}
Committed: https://chromium.googlesource.com/external/webrtc/+/41476e014c8364adc15b90238d54a8aef91d7f56
Patch Set 1 #Patch Set 2 : Add unittest. Not yet working. #Patch Set 3 : Revert "Add unittest. Not yet working." #
Messages
Total messages: 17 (6 generated)
nisse@webrtc.org changed reviewers: + brandtr@webrtc.org, eladalon@webrtc.org, stefan@webrtc.org
I'd appreciate advice on where and how to add a corresponding unit test. Can we copy some existing unit test and change it to add the RED header before input to the FEC machinery?
I think that should be possible. Look at these tests: https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/sour...
You can look at the corresponding FlexFEC test for inspiration: https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/sour.... If you need to generate a RED packet, you can use maybe use this class: https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/sour...
Description was changed from ========== When Ulpfec recovers a packet, set |returned| flag earlier. This avoids infinite recursion in case the recovered packet carries a RED header. BUG=chromium:754748 ========== to ========== When Ulpfec recovers a packet, set |returned| flag earlier. This avoids infinite recursion in case the recovered packet carries a RED header. BUG=chromium:754748 ==========
On 2017/08/24 12:52:32, brandtr wrote: > You can look at the corresponding FlexFEC test for inspiration: > https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/sour.... > > If you need to generate a RED packet, you can use maybe use this class: > https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/sour... I've given it a try. The simple approach of having OnRecoveredPacket unconditionally call ProcessReceivedFec doesn't work, because OnRecoveredPacket is invoked also for decapsulated RED media packets, and we get an unrelated infinite loop there (we might want to reject red-over-red-over-red, but that's a separate issue). So OnRecoveredPacket has to parse the packets and switch on payload type. And then to replicate the problem, we really need the test case to add the RED header before FEC. I've tried to implement that, but it's not yet working. Fails like [ RUN ] UlpfecReceiverTest.RecoveryCallbackDoesNotLoopInfinitely ../../webrtc/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc:589: Failure Expected: 1u Which is: 1 To be equal to: counter.num_recovered_packets Which is: 0 [ FAILED ] UlpfecReceiverTest.RecoveryCallbackDoesNotLoopInfinitely (0 ms) which I think means that the test isn't recovering any packets, and hence doesn't cover what we want it to cover.
On 2017/08/24 14:40:09, nisse-webrtc wrote: > On 2017/08/24 12:52:32, brandtr wrote: > > You can look at the corresponding FlexFEC test for inspiration: > > > https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/sour.... > > > > If you need to generate a RED packet, you can use maybe use this class: > > > https://cs.chromium.org/chromium/src/third_party/webrtc/modules/rtp_rtcp/sour... > > I've given it a try. The simple approach of having OnRecoveredPacket > unconditionally call ProcessReceivedFec doesn't work, because OnRecoveredPacket > is invoked also for decapsulated RED media packets, and we get an unrelated > infinite loop there (we might want to reject red-over-red-over-red, but that's a > separate issue). > > So OnRecoveredPacket has to parse the packets and switch on payload type. And > then to replicate the problem, we really need the test case to add the RED > header before FEC. I've tried to implement that, but it's not yet working. Fails > like > > [ RUN ] UlpfecReceiverTest.RecoveryCallbackDoesNotLoopInfinitely > ../../webrtc/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc:589: Failure > Expected: 1u > Which is: 1 > To be equal to: counter.num_recovered_packets > Which is: 0 > [ FAILED ] UlpfecReceiverTest.RecoveryCallbackDoesNotLoopInfinitely (0 ms) > > which I think means that the test isn't recovering any packets, and hence > doesn't > cover what we want it to cover. Should we land this and follow up with a test separately?
On 2017/08/25 13:38:10, stefan-webrtc wrote: > > Should we land this and follow up with a test separately? If you agree to lgtm ps#1, I can land try to land it (I guess I'll have to upload a ps#3 which is identical to ps#1).
On 2017/08/25 13:56:34, nisse-webrtc wrote: > On 2017/08/25 13:38:10, stefan-webrtc wrote: > > > > Should we land this and follow up with a test separately? > > If you agree to lgtm ps#1, I can land try to land it (I guess I'll have to > upload a ps#3 which is identical to ps#1). ps#1 lgtm
ps#1 also lgtm
The CQ bit was checked by stefan@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/3004553002/#ps40001 (title: "Revert "Add unittest. Not yet working."")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
After-the-fact lgtm. :-)
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1503672319374650,
"parent_rev": "a7a4beb419612e16e781e143f9bc0c812e044f75", "commit_rev":
"41476e014c8364adc15b90238d54a8aef91d7f56"}
Message was sent while issue was closed.
Description was changed from ========== When Ulpfec recovers a packet, set |returned| flag earlier. This avoids infinite recursion in case the recovered packet carries a RED header. BUG=chromium:754748 ========== to ========== When Ulpfec recovers a packet, set |returned| flag earlier. This avoids infinite recursion in case the recovered packet carries a RED header. BUG=chromium:754748 Review-Url: https://codereview.webrtc.org/3004553002 Cr-Commit-Position: refs/heads/master@{#19525} Committed: https://chromium.googlesource.com/external/webrtc/+/41476e014c8364adc15b90238... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/webrtc/+/41476e014c8364adc15b90238... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
