Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1635093002:
Add transport sequence number on the non-pacer path of the rtp sender. (Closed)
|
Created:
4 years, 11 months ago by stefan-webrtc Modified:
4 years, 11 months ago Reviewers:
sprang_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd transport sequence number on the non-pacer path of the rtp sender.
BUG=4173
R=sprang@webrtc.org
Committed: https://crrev.com/f5dca48dc0bce89f18b9a90f5c7c1a03bcf95233
Cr-Commit-Position: refs/heads/master@{#11395}
Patch Set 1 #Patch Set 2 : Also make sure padding can be sent prior to video in the transport seq num case. #
Total comments: 8
Patch Set 3 : Comments addressed. #
Messages
Total messages: 21 (9 generated)
stefan@webrtc.org changed reviewers: + sprang@webrtc.org
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1635093002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1635093002/1
Also make sure padding can be sent prior to video in the transport seq num case.
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1635093002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1635093002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_libfuzzer_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
lgtm with nits https://codereview.webrtc.org/1635093002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/1635093002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1095: if (using_transport_seq && transport_feedback_observer_) { Nit: Wrap in same if-block as if (using_transport_seq) { above and just switch on transport_feedback_observer_. Is that even a valid case btw? Should we just DCHECK on it instead? https://codereview.webrtc.org/1635093002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right): https://codereview.webrtc.org/1635093002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:490: uint8_t payload[] = {47, 11, 32, 93, 89}; There's no special meaning to these values, right? https://codereview.webrtc.org/1635093002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:493: .WillOnce(testing::Return(17)); Name 17 and check it at the end. https://codereview.webrtc.org/1635093002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:495: 0, rtp_sender_->SendOutgoingData(kVideoFrameKey, payload_type, 1234, 4321, Nit: Perhaps name or comment on what 1234 and 4321 is?
Comments addressed.
https://codereview.webrtc.org/1635093002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/1635093002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:1095: if (using_transport_seq && transport_feedback_observer_) { On 2016/01/27 11:32:17, språng wrote: > Nit: Wrap in same if-block as if (using_transport_seq) { above > and just switch on transport_feedback_observer_. > Is that even a valid case btw? Should we just DCHECK on it instead? Done. https://codereview.webrtc.org/1635093002/diff/20001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right): https://codereview.webrtc.org/1635093002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:490: uint8_t payload[] = {47, 11, 32, 93, 89}; On 2016/01/27 11:32:17, språng wrote: > There's no special meaning to these values, right? Nope :) https://codereview.webrtc.org/1635093002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:493: .WillOnce(testing::Return(17)); On 2016/01/27 11:32:17, språng wrote: > Name 17 and check it at the end. Done. https://codereview.webrtc.org/1635093002/diff/20001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:495: 0, rtp_sender_->SendOutgoingData(kVideoFrameKey, payload_type, 1234, 4321, On 2016/01/27 11:32:17, språng wrote: > Nit: Perhaps name or comment on what 1234 and 4321 is? Done.
The CQ bit was checked by stefan@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org Link to the patchset: https://codereview.webrtc.org/1635093002/#ps40001 (title: "Comments addressed.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1635093002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1635093002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_libfuzzer_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
Message was sent while issue was closed.
Description was changed from ========== Add transport sequence number on the non-pacer path of the rtp sender. BUG=4173 ========== to ========== Add transport sequence number on the non-pacer path of the rtp sender. BUG=4173 R=sprang@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/f5dca48dc0bce89f18b9a90f5... ==========
Message was sent while issue was closed.
Description was changed from ========== Add transport sequence number on the non-pacer path of the rtp sender. BUG=4173 ========== to ========== Add transport sequence number on the non-pacer path of the rtp sender. BUG=4173 R=sprang@webrtc.org Committed: https://crrev.com/f5dca48dc0bce89f18b9a90f5c7c1a03bcf95233 Cr-Commit-Position: refs/heads/master@{#11395} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f5dca48dc0bce89f18b9a90f5c7c1a03bcf95233 Cr-Commit-Position: refs/heads/master@{#11395}
Message was sent while issue was closed.
Committed patchset #3 (id:40001) manually as f5dca48dc0bce89f18b9a90f5c7c1a03bcf95233 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
