Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
3 years, 4 months ago by eladalon Modified:
3 years, 4 months ago Reviewers:
philipel, danilchap, stefan-webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, stefan-webrtc, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd PacketRouterTest.Sanity_NoModuleRegistered_*
Add some sanity tests for PacketRouter when no modules are registered.
BUG=None
Review-Url: https://codereview.webrtc.org/2986093003
Cr-Commit-Position: refs/heads/master@{#19215}
Committed: https://chromium.googlesource.com/external/webrtc/+/32040efc61a9fde3d8d32d48f3f2cbb04a69c33d
Patch Set 1 #
Total comments: 2
Patch Set 2 : . #
Total comments: 7
Patch Set 3 : . #Patch Set 4 : Rebased and added TODO elsewhere. #
Created: 3 years, 4 months ago
Messages
Total messages: 20 (9 generated)
eladalon@webrtc.org changed reviewers: + danilchap@webrtc.org, stefan@webrtc.org
PTAL
<Previous discussion on the topic>
PacedSender::SendPacket uses that, so even if we end up removing it, that's work
for a separate CL.
I have the TODO there because it looks suspicious. If we look at that
aforementioned SendPacket(), we can see the following (where |success| is the
return value of TimeToSendPacket):
if (success) {
if (packet.priority != kHighPriority) {
// Update media bytes sent.
UpdateBudgetWithBytesSent(packet.bytes);
}
It seems suspicious to me that a packet that did not end up being sent anywhere
could lead to an updating of a budget, and merits further investigation. Then
again, it might also be that this is all highly theoretical, because the SSRC
not matching might not be possible. Wdyt about filing a bug for this and
removing this TODO from this CL?
</Previous discussion on the topic>
https://codereview.webrtc.org/2986093003/diff/1/webrtc/modules/pacing/packet_... File webrtc/modules/pacing/packet_router_unittest.cc (right): https://codereview.webrtc.org/2986093003/diff/1/webrtc/modules/pacing/packet_... webrtc/modules/pacing/packet_router_unittest.cc:50: // returns true even when no modules are found that match the SSRC. looking for documentation for the return value of the TimeToSendPacket... it is declared in PacedSender::PacketSender: "Returns false if packet cannot be sent." which implies true when packet can. That matches your discovery how return value is used. So yes, return true when there are no module to send packet with looks like a bug. Though there is another problem: packet that failed to be sent (PacedSender::Process) stays in queue, but packet that has no matching sender - shouldn't. So may be better to extend documentation for the TimeToSendPacket: "Returns false if packet cannot be send now, retry later". If sender for the packet is gone, updating budget might be wrong, but not a big deal - those packets should be few.
Description was changed from ========== Add PacketRouterTest.Sanity_NoModuleRegistered_TimeToSendPacket BUG=None ========== to ========== Add PacketRouterTest.Sanity_NoModuleRegistered_* Add some sanity tests for PacketRouter when no modules are registered. BUG=None ==========
https://codereview.webrtc.org/2986093003/diff/1/webrtc/modules/pacing/packet_... File webrtc/modules/pacing/packet_router_unittest.cc (right): https://codereview.webrtc.org/2986093003/diff/1/webrtc/modules/pacing/packet_... webrtc/modules/pacing/packet_router_unittest.cc:50: // returns true even when no modules are found that match the SSRC. On 2017/07/28 12:25:48, danilchap wrote: > looking for documentation for the return value of the TimeToSendPacket... it is > declared in PacedSender::PacketSender: > "Returns false if packet cannot be sent." > which implies true when packet can. > That matches your discovery how return value is used. > > So yes, return true when there are no module to send packet with looks like a > bug. > > Though there is another problem: packet that failed to be sent > (PacedSender::Process) stays in queue, but packet that has no matching sender - > shouldn't. > > So may be better to extend documentation for the TimeToSendPacket: "Returns > false if packet cannot be send now, retry later". > If sender for the packet is gone, updating budget might be wrong, but not a big > deal - those packets should be few. I'll use this CL to introduce the sanity tests, and keep a (non-!!!) TODO here for it. I'll push a CL based on top of this one that will fix this issue. We have two things left to discuss, then: 1. Whether to fix. 2. How to fix. About #1, I think it's worth it, because there are several cases where this would happen: 1.1. The aforementioned case of no module with the right SSRC (probably should never happen). 1.2. When (!RTPSender::SendingMedia()) 1.3. Packet somehow missing from packet history (queued for too long?). As for the fix (#2), how about returning both success/failure as well as the number of sent bytes? Success/failure can be treated as it is now, but for updating budgets, only the number of sent bytes would be used. Alternatively, we could use -1 for error and anything non-negative for success-and-sent-bytes.
https://codereview.webrtc.org/2986093003/diff/20001/webrtc/modules/pacing/pac... File webrtc/modules/pacing/packet_router_unittest.cc (right): https://codereview.webrtc.org/2986093003/diff/20001/webrtc/modules/pacing/pac... webrtc/modules/pacing/packet_router_unittest.cc:49: // TODO(eladalon): TimeToSendPacket() returning true when nothing was either phrase TODO in a from of what and when should be done. Or make it a regular comment describing current state of code. https://codereview.webrtc.org/2986093003/diff/20001/webrtc/modules/pacing/pac... webrtc/modules/pacing/packet_router_unittest.cc:51: // This will be fixed in an upcoming CL. this line of comment doesn't describe code, so probably better to put it in CL comment or CL description. https://codereview.webrtc.org/2986093003/diff/20001/webrtc/modules/pacing/pac... webrtc/modules/pacing/packet_router_unittest.cc:88: EXPECT_FALSE(packet_router_->SendTransportFeedback(&feedback)); may be packet_router instead of packet_router_ (i.e. local variable, not the pre-created member variable)
https://codereview.webrtc.org/2986093003/diff/20001/webrtc/modules/pacing/pac... File webrtc/modules/pacing/packet_router_unittest.cc (right): https://codereview.webrtc.org/2986093003/diff/20001/webrtc/modules/pacing/pac... webrtc/modules/pacing/packet_router_unittest.cc:49: // TODO(eladalon): TimeToSendPacket() returning true when nothing was On 2017/07/31 08:49:46, danilchap wrote: > either phrase TODO in a from of what and when should be done. > Or make it a regular comment describing current state of code. > > What - still under discussion. When - next CL (mentioned by the end of the TODO). https://codereview.webrtc.org/2986093003/diff/20001/webrtc/modules/pacing/pac... webrtc/modules/pacing/packet_router_unittest.cc:51: // This will be fixed in an upcoming CL. On 2017/07/31 08:49:46, danilchap wrote: > this line of comment doesn't describe code, so probably better to put it in CL > comment or CL description. This is still part of the TODO. https://codereview.webrtc.org/2986093003/diff/20001/webrtc/modules/pacing/pac... webrtc/modules/pacing/packet_router_unittest.cc:88: EXPECT_FALSE(packet_router_->SendTransportFeedback(&feedback)); On 2017/07/31 08:49:46, danilchap wrote: > may be packet_router instead of packet_router_ > (i.e. local variable, not the pre-created member variable) Actually, let me use the one from the fixture. We can probably remove it from the fixture (in a separate CL, if we choose to do it at all), but until we do, might as well use it in all of the tests that can.
lgtm https://codereview.webrtc.org/2986093003/diff/20001/webrtc/modules/pacing/pac... File webrtc/modules/pacing/packet_router_unittest.cc (right): https://codereview.webrtc.org/2986093003/diff/20001/webrtc/modules/pacing/pac... webrtc/modules/pacing/packet_router_unittest.cc:49: // TODO(eladalon): TimeToSendPacket() returning true when nothing was On 2017/07/31 08:59:10, eladalon wrote: > On 2017/07/31 08:49:46, danilchap wrote: > > either phrase TODO in a from of what and when should be done. > > Or make it a regular comment describing current state of code. > > > > > > What - still under discussion. > When - next CL (mentioned by the end of the TODO). Acknowledged.
The CQ bit was checked by eladalon@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/2986093003/#ps60001 (title: "Rebased and added TODO elsewhere.")
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
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/19783)
philipel@webrtc.org changed reviewers: + philipel@webrtc.org
rs-lgtm
The CQ bit was checked by eladalon@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1501679518837210,
"parent_rev": "88df90b1fdbe667c76329030390844ccd9faa88f", "commit_rev":
"32040efc61a9fde3d8d32d48f3f2cbb04a69c33d"}
Message was sent while issue was closed.
Description was changed from ========== Add PacketRouterTest.Sanity_NoModuleRegistered_* Add some sanity tests for PacketRouter when no modules are registered. BUG=None ========== to ========== Add PacketRouterTest.Sanity_NoModuleRegistered_* Add some sanity tests for PacketRouter when no modules are registered. BUG=None Review-Url: https://codereview.webrtc.org/2986093003 Cr-Commit-Position: refs/heads/master@{#19215} Committed: https://chromium.googlesource.com/external/webrtc/+/32040efc61a9fde3d8d32d48f... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/webrtc/+/32040efc61a9fde3d8d32d48f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
