Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2973323002:
Call should allow pass through of keep-alive packets. (Closed)
|
Created:
3 years, 5 months ago by sprang_webrtc Modified:
3 years, 5 months ago Reviewers:
pbos-webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun, stefan-webrtc, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionCall should allow pass through of keep-alive packets.
Don't force requirement of media type for those packets.
BUG=webrtc:7964
Review-Url: https://codereview.webrtc.org/2973323002
Cr-Commit-Position: refs/heads/master@{#18966}
Committed: https://chromium.googlesource.com/external/webrtc/+/c1abde7e8eaaa9d458459d84f696b312a33fcdd3
Patch Set 1 #
Total comments: 3
Patch Set 2 : Added comment #Messages
Total messages: 15 (7 generated)
sprang@webrtc.org changed reviewers: + pbos@webrtc.org
Bonus fix! Please?
lgtm https://codereview.webrtc.org/2973323002/diff/1/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2973323002/diff/1/webrtc/call/call.cc#newcode1306 webrtc/call/call.cc:1306: const bool is_keep_alive_packet = Can you put some reference to keepalive packets to motivate this check? Should this also be verifying a payload type?
https://codereview.webrtc.org/2973323002/diff/1/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2973323002/diff/1/webrtc/call/call.cc#newcode1306 webrtc/call/call.cc:1306: const bool is_keep_alive_packet = On 2017/07/10 22:36:31, pbos-webrtc wrote: > Can you put some reference to keepalive packets to motivate this check? Should > this also be verifying a payload type? Reading the other CL, please check config_.rtp.keepalive.payload_type here too (instead? does payload_size() matter here?), if this is signalled from the other side and not just a send-payload-type attribute.
https://codereview.webrtc.org/2973323002/diff/1/webrtc/call/call.cc File webrtc/call/call.cc (right): https://codereview.webrtc.org/2973323002/diff/1/webrtc/call/call.cc#newcode1306 webrtc/call/call.cc:1306: const bool is_keep_alive_packet = On 2017/07/10 22:39:16, pbos-webrtc wrote: > On 2017/07/10 22:36:31, pbos-webrtc wrote: > > Can you put some reference to keepalive packets to motivate this check? Should > > this also be verifying a payload type? > > Reading the other CL, please check config_.rtp.keepalive.payload_type here too > (instead? does payload_size() matter here?), if this is signalled from the other > side and not just a send-payload-type attribute. This is based on https://tools.ietf.org/html/rfc6263#section-4.6 The intent is that the packets should have an explicitly unknown payload type. RFC3550 states "a receiver MUST ignore packets with payload types that it does not understand". The packets should also have zero payload length, since we have a special case for such keep-alive packets in the receiver; if the payload length is non-zero, we will log a warning. So we can't check the payload type on the receiver. (This method is receive side only, right?) I'll add a comment.
The CQ bit was checked by sprang@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from pbos@webrtc.org Link to the patchset: https://codereview.webrtc.org/2973323002/#ps20001 (title: "Added comment")
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: android_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/25733)
The CQ bit was checked by sprang@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": 20001, "attempt_start_ts": 1499767434283950,
"parent_rev": "7fc3f156860285126a0ef24d48e9dfb626ce8145", "commit_rev":
"c1abde7e8eaaa9d458459d84f696b312a33fcdd3"}
Message was sent while issue was closed.
Description was changed from ========== Call should allow pass through of keep-alive packets. Don't force requirement of media type for those packets. BUG=webrtc:7964 ========== to ========== Call should allow pass through of keep-alive packets. Don't force requirement of media type for those packets. BUG=webrtc:7964 Review-Url: https://codereview.webrtc.org/2973323002 Cr-Commit-Position: refs/heads/master@{#18966} Committed: https://chromium.googlesource.com/external/webrtc/+/c1abde7e8eaaa9d458459d84f... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/external/webrtc/+/c1abde7e8eaaa9d458459d84f... |
