Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2947133002:
Fix timing frames and FEC conflict (Closed)
DescriptionFix timing frames and FEC conflict
Reenable pacer_exit timestamp updates for the timing frames and
exclude timing-frames carrying packets from the FEC.
BUG=webrtc:7859
Review-Url: https://codereview.webrtc.org/2947133002
Cr-Commit-Position: refs/heads/master@{#18702}
Committed: https://chromium.googlesource.com/external/webrtc/+/10894996efb088c305db07365c1871e02c98a818
Patch Set 1 #
Total comments: 8
Patch Set 2 : Add comment about FEC and modifications of the packet in the pacer #Patch Set 3 : Modify VideoSendStream test for video timing extension #
Total comments: 2
Patch Set 4 : Added test for not sending FEC on timing frames #
Total comments: 6
Patch Set 5 : Implement Brandtr@ comments #
Total comments: 4
Patch Set 6 : Format #Patch Set 7 : Fix typo #
Messages
Total messages: 28 (12 generated)
ilnik@webrtc.org changed reviewers: + stefan@webrtc.org
Description was changed from ========== Fix timing frames and FEC conflict Reenable pacer_exit timestamp updates for the timing frames and exclude timing-frames carrying packets from the FEC. BUG=webrtc:7594 ========== to ========== Fix timing frames and FEC conflict Reenable pacer_exit timestamp updates for the timing frames and exclude timing-frames carrying packets from the FEC. BUG=webrtc:7859 ==========
ilnik@webrtc.org changed reviewers: + asapersson@webrtc.org - stefan@webrtc.org
brandtr@webrtc.org changed reviewers: + brandtr@webrtc.org
https://codereview.webrtc.org/2947133002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2947133002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:743: packet_to_send->SetExtension<AbsoluteSendTime>( Can you add a comment here, and below, describing what the problem is to change these header extensions here? And why there's a difference between AbsoluteSendTime and VideoTimingExtension. https://codereview.webrtc.org/2947133002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2947133002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:408: protect_packet = false; Can you add a unit test for this?
https://codereview.webrtc.org/2947133002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2947133002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:388: : rtc::MakeUnique<RtpPacketToSend>(*rtp_header); Maybe also check in VideoSendStreamTest.SupportsVideoTimingFrames that if a packet has the timing extension, it is the last packet in a frame (i.e marker bit set).
https://codereview.webrtc.org/2947133002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender.cc (right): https://codereview.webrtc.org/2947133002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender.cc:743: packet_to_send->SetExtension<AbsoluteSendTime>( On 2017/06/21 11:05:39, brandtr wrote: > Can you add a comment here, and below, describing what the problem is to change > these header extensions here? And why there's a difference between > AbsoluteSendTime and VideoTimingExtension. Done. https://codereview.webrtc.org/2947133002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2947133002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:408: protect_packet = false; On 2017/06/21 11:05:39, brandtr wrote: > Can you add a unit test for this? What exactly do you want me to test? What packets with timing frames are not protected by the FEC? What there are no corruptions with timing frames?
https://codereview.webrtc.org/2947133002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2947133002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:388: : rtc::MakeUnique<RtpPacketToSend>(*rtp_header); On 2017/06/21 11:14:00, åsapersson wrote: > Maybe also check in VideoSendStreamTest.SupportsVideoTimingFrames that if a > packet has the timing extension, it is the last packet in a frame (i.e marker > bit set). Done.
https://codereview.webrtc.org/2947133002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2947133002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:408: protect_packet = false; On 2017/06/21 11:17:18, ilnik wrote: > On 2017/06/21 11:05:39, brandtr wrote: > > Can you add a unit test for this? > > What exactly do you want me to test? What packets with timing frames are not > protected by the FEC? What there are no corruptions with timing frames? The former. Either through a unit test here, or using an end-to-end test that verifies that sent FEC packets are not protecting packets with timing headers. I think the unit test should be less involved to write. I will think about more ways to catch corruptions, so you don't have to add any tests for that.
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2947133002/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc (right): https://codereview.webrtc.org/2947133002/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_sender_video.cc:408: protect_packet = false; On 2017/06/21 11:28:15, brandtr wrote: > On 2017/06/21 11:17:18, ilnik wrote: > > On 2017/06/21 11:05:39, brandtr wrote: > > > Can you add a unit test for this? > > > > What exactly do you want me to test? What packets with timing frames are not > > protected by the FEC? What there are no corruptions with timing frames? > > The former. Either through a unit test here, or using an end-to-end test that > verifies that sent FEC packets are not protecting packets with timing headers. I > think the unit test should be less involved to write. > > I will think about more ways to catch corruptions, so you don't have to add any > tests for that. Done.
https://codereview.webrtc.org/2947133002/diff/40001/webrtc/video/video_send_s... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2947133002/diff/40001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:352: if (!header.markerBit) check that extension is not set?
https://codereview.webrtc.org/2947133002/diff/40001/webrtc/video/video_send_s... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2947133002/diff/40001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:352: if (!header.markerBit) On 2017/06/21 12:51:39, åsapersson wrote: > check that extension is not set? Can't do, as extension may be set on not the last packet, if it's an end of a lower spatial layer. Later, in tests, when LayerFilteringTransport truncates higher spatial layers and puts marker bit in some packets, they should also contain timing frame extension.
https://codereview.webrtc.org/2947133002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right): https://codereview.webrtc.org/2947133002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:947: TEST_P(RtpSenderTest, NoFlexForTimingFrames) { Flexfec https://codereview.webrtc.org/2947133002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:991: video_header.video_timing.is_timing_frame = true; How about sending the same frame, but with |is_timing_frame| set to false as well? video_header.video_timing.is_timing_frame = false; SendOutgoingData(); EXPECT_CALL...Times(1); video_header.video_timing.is_timing_frame = true; SendOutgoingData(); EXPECT_CALL...Times(0); ASSERT_EQ(3, transport_.packets_sent());
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/25160)
https://codereview.webrtc.org/2947133002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right): https://codereview.webrtc.org/2947133002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:947: TEST_P(RtpSenderTest, NoFlexForTimingFrames) { On 2017/06/21 13:10:27, brandtr wrote: > Flexfec Done. https://codereview.webrtc.org/2947133002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:991: video_header.video_timing.is_timing_frame = true; On 2017/06/21 13:10:26, brandtr wrote: > How about sending the same frame, but with |is_timing_frame| set to false as > well? > > video_header.video_timing.is_timing_frame = false; > SendOutgoingData(); > EXPECT_CALL...Times(1); > > video_header.video_timing.is_timing_frame = true; > SendOutgoingData(); > EXPECT_CALL...Times(0); > > ASSERT_EQ(3, transport_.packets_sent()); It's already done in RtpSenderTestWithoutPacer.SendFlexfecPackets but maybe for clarity, it makes sense. Added.
lgtm if you run 'git cl format' https://codereview.webrtc.org/2947133002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right): https://codereview.webrtc.org/2947133002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:991: video_header.video_timing.is_timing_frame = true; On 2017/06/21 13:28:06, ilnik wrote: > On 2017/06/21 13:10:26, brandtr wrote: > > How about sending the same frame, but with |is_timing_frame| set to false as > > well? > > > > video_header.video_timing.is_timing_frame = false; > > SendOutgoingData(); > > EXPECT_CALL...Times(1); > > > > video_header.video_timing.is_timing_frame = true; > > SendOutgoingData(); > > EXPECT_CALL...Times(0); > > > > ASSERT_EQ(3, transport_.packets_sent()); > > It's already done in RtpSenderTestWithoutPacer.SendFlexfecPackets but maybe for > clarity, it makes sense. Added. Right, this is just to verify that FEC actually would have been sent, if this was not a timing frame. https://codereview.webrtc.org/2947133002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right): https://codereview.webrtc.org/2947133002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:1034: EXPECT_EQ(kSeqNum+1, media_packet2.SequenceNumber()); format
lgtm https://codereview.webrtc.org/2947133002/diff/80001/webrtc/video/video_send_s... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2947133002/diff/80001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:341: class VideoRotationObserver : public test::SendTest { VideoTimingObserver (and similar in test above)
https://codereview.webrtc.org/2947133002/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right): https://codereview.webrtc.org/2947133002/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:991: video_header.video_timing.is_timing_frame = true; On 2017/06/21 13:32:00, brandtr wrote: > On 2017/06/21 13:28:06, ilnik wrote: > > On 2017/06/21 13:10:26, brandtr wrote: > > > How about sending the same frame, but with |is_timing_frame| set to false as > > > well? > > > > > > video_header.video_timing.is_timing_frame = false; > > > SendOutgoingData(); > > > EXPECT_CALL...Times(1); > > > > > > video_header.video_timing.is_timing_frame = true; > > > SendOutgoingData(); > > > EXPECT_CALL...Times(0); > > > > > > ASSERT_EQ(3, transport_.packets_sent()); > > > > It's already done in RtpSenderTestWithoutPacer.SendFlexfecPackets but maybe > for > > clarity, it makes sense. Added. > > Right, this is just to verify that FEC actually would have been sent, if this > was not a timing frame. Acknowledged. https://codereview.webrtc.org/2947133002/diff/80001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc (right): https://codereview.webrtc.org/2947133002/diff/80001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_sender_unittest.cc:1034: EXPECT_EQ(kSeqNum+1, media_packet2.SequenceNumber()); On 2017/06/21 13:32:00, brandtr wrote: > format Done. https://codereview.webrtc.org/2947133002/diff/80001/webrtc/video/video_send_s... File webrtc/video/video_send_stream_tests.cc (right): https://codereview.webrtc.org/2947133002/diff/80001/webrtc/video/video_send_s... webrtc/video/video_send_stream_tests.cc:341: class VideoRotationObserver : public test::SendTest { On 2017/06/21 13:40:14, åsapersson wrote: > VideoTimingObserver (and similar in test above) Done.
The CQ bit was checked by ilnik@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org, asapersson@webrtc.org Link to the patchset: https://codereview.webrtc.org/2947133002/#ps120001 (title: "Fix typo")
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": 120001, "attempt_start_ts": 1498053191929010,
"parent_rev": "83c97da5931956ae8596d2efcfdea282fdc86d2d", "commit_rev":
"10894996efb088c305db07365c1871e02c98a818"}
Message was sent while issue was closed.
Description was changed from ========== Fix timing frames and FEC conflict Reenable pacer_exit timestamp updates for the timing frames and exclude timing-frames carrying packets from the FEC. BUG=webrtc:7859 ========== to ========== Fix timing frames and FEC conflict Reenable pacer_exit timestamp updates for the timing frames and exclude timing-frames carrying packets from the FEC. BUG=webrtc:7859 Review-Url: https://codereview.webrtc.org/2947133002 Cr-Commit-Position: refs/heads/master@{#18702} Committed: https://chromium.googlesource.com/external/webrtc/+/10894996efb088c305db07365... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/external/webrtc/+/10894996efb088c305db07365... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
