Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1261323003:
Use RtcpPacket to send FIR in RtcpSender (Closed)
|
Created:
5 years, 4 months ago by sprang_webrtc Modified:
5 years, 4 months ago Reviewers:
åsapersson CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, stefan-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionUse RtcpPacket to send FIR in RtcpSender
BUG=webrtc:2450
Committed: https://crrev.com/62dae190985454188de112e35a16e35fc6e912a4
Cr-Commit-Position: refs/heads/master@{#9677}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Comment #Patch Set 3 : Increment seq even if truncated #Messages
Total messages: 18 (6 generated)
sprang@webrtc.org changed reviewers: + asapersson@webrtc.org
https://codereview.webrtc.org/1261323003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right): https://codereview.webrtc.org/1261323003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:580: sequence_number_fir_ = seq; // Only update if we actually sent packet. Will the caller know if the FIR request is not sent due to truncation? If not, then a repeat for this request will correspond to the previous request.
The CQ bit was checked by sprang@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1261323003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1261323003/20001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
https://codereview.webrtc.org/1261323003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_sender.cc (right): https://codereview.webrtc.org/1261323003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_sender.cc:580: sequence_number_fir_ = seq; // Only update if we actually sent packet. On 2015/08/04 12:07:27, asapersson wrote: > Will the caller know if the FIR request is not sent due to truncation? If not, > then a repeat for this request will correspond to the previous request. I updated the comment to the more clear "Update state only if we built the packet." That is, we don't want to update the state of the sequence number if we don't even have space enough to build the packet. The behavior is the same as before, we just need this temporary since the old code would return early in the truncation case.
Made change as discussed offline.
The CQ bit was checked by sprang@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1261323003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1261323003/40001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
lgtm
The CQ bit was checked by sprang@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1261323003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1261323003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/62dae190985454188de112e35a16e35fc6e912a4 Cr-Commit-Position: refs/heads/master@{#9677} |
