Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1226093002:
Don't create unsignalled receive streams for RTX and ULPFEC packets. (Closed)
|
Created:
5 years, 5 months ago by noahric Modified:
5 years, 5 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), Andrew MacDonald Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionDon't create unsignalled receive streams for RTX, RED RTX, and ULPFEC packets.
BUG=webrtc:4389
Committed: https://crrev.com/d10a68e7974a29b26d6c926e6f137255f3c173be
Cr-Commit-Position: refs/heads/master@{#9566}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Also ignore RED RTX and ULPFEC. #
Total comments: 8
Patch Set 3 : Refactored to separate tests with a shared test method #
Total comments: 4
Patch Set 4 : ASSERT_EQ->EXPECT_EQ #
Messages
Total messages: 17 (5 generated)
noahric@chromium.org changed reviewers: + pbos@chromium.org, pthatcher@google.com
pbos: let me know if you want me to reverse the check, though I think the other way is more complicated since it'd want to accept fec/red payload types, and anything else that uses the same ssrc but different pts. I'm not sure about red rtx, so I can add that explicitly if you think it's necessary. Thanks!
pthatcher@webrtc.org changed reviewers: + pthatcher@webrtc.org
https://codereview.webrtc.org/1226093002/diff/1/talk/media/webrtc/webrtcvideo... File talk/media/webrtc/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1226093002/diff/1/talk/media/webrtc/webrtcvideo... talk/media/webrtc/webrtcvideoengine2.cc:1393: } You probably want to ignore RTX, RED RTX, and FEC, but not RED. So, drop if it matches codec.fec.ulpfec_payload_type or codec.fec.red_rtx_payload_type.
https://codereview.webrtc.org/1226093002/diff/1/talk/media/webrtc/webrtcvideo... File talk/media/webrtc/webrtcvideoengine2.cc (right): https://codereview.webrtc.org/1226093002/diff/1/talk/media/webrtc/webrtcvideo... talk/media/webrtc/webrtcvideoengine2.cc:1393: } On 2015/07/08 22:30:46, pthatcher1 wrote: > You probably want to ignore RTX, RED RTX, and FEC, but not RED. So, drop if it > matches codec.fec.ulpfec_payload_type or codec.fec.red_rtx_payload_type. Do they come on different ssrcs? If not, I'd think they could actually be handled correctly. RTX is just special because it lacks the information to figure out what it really associates with (besides sequence number).
On 2015/07/08 22:33:26, noahric wrote: > https://codereview.webrtc.org/1226093002/diff/1/talk/media/webrtc/webrtcvideo... > File talk/media/webrtc/webrtcvideoengine2.cc (right): > > https://codereview.webrtc.org/1226093002/diff/1/talk/media/webrtc/webrtcvideo... > talk/media/webrtc/webrtcvideoengine2.cc:1393: } > On 2015/07/08 22:30:46, pthatcher1 wrote: > > You probably want to ignore RTX, RED RTX, and FEC, but not RED. So, drop if > it > > matches codec.fec.ulpfec_payload_type or codec.fec.red_rtx_payload_type. > > Do they come on different ssrcs? If not, I'd think they could actually be > handled correctly. RTX is just special because it lacks the information to > figure out what it really associates with (besides sequence number). Ok, added RED RTX and ULPFEC. PTAL. Thanks!
pbos@webrtc.org changed reviewers: + pbos@webrtc.org
https://codereview.webrtc.org/1226093002/diff/20001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/1226093002/diff/20001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2_unittest.cc:2588: VideoCodec::CreateRtxCodec(125, kDefaultRedPlType); Name this 125 constant and use it below as well. https://codereview.webrtc.org/1226093002/diff/20001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2_unittest.cc:2591: const std::vector<uint32> rtx_ssrcs = MAKE_VECTOR(kRtxSsrcs1); Just use a static const int kIncomingUnsignalledSsrc = 0xC0FFEE. This isn't a RTX SSRC specifically, and unsignalled streams have nothing to do with RTX/non-RTX SSRCs, since they can't be distinguished between (so I think the test is confusing, as it looks like it should handle the RTX SSRC differently). https://codereview.webrtc.org/1226093002/diff/20001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2_unittest.cc:2600: for (int payload_type : { kDefaultRtxVp8PlType, kDefaultUlpfecType, 125 }) { Looks like you can't get away with this sir. :) https://codereview.webrtc.org/1226093002/diff/20001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2_unittest.cc:2607: ASSERT_EQ(0u, fake_call_->GetVideoReceiveStreams().size()) Is there a test asserting that we do create it for VP8? If there is I'd like you to consolidate them into one (if not, add one for VP8): void WebRtcVideoChannel2Test::ReceiveUnsignalledSsrc(uint8_t payload_type, bool expect_created_receive_stream); Then do four different instantiations, but expect only the codec one to succeed, so fail on RTX, FEC etc. ?
Thanks! PTAL. https://codereview.webrtc.org/1226093002/diff/20001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/1226093002/diff/20001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2_unittest.cc:2588: VideoCodec::CreateRtxCodec(125, kDefaultRedPlType); On 2015/07/09 10:38:43, pbos-webrtc wrote: > Name this 125 constant and use it below as well. Done. https://codereview.webrtc.org/1226093002/diff/20001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2_unittest.cc:2591: const std::vector<uint32> rtx_ssrcs = MAKE_VECTOR(kRtxSsrcs1); On 2015/07/09 10:38:43, pbos-webrtc wrote: > Just use a static const int kIncomingUnsignalledSsrc = 0xC0FFEE. This isn't a > RTX SSRC specifically, and unsignalled streams have nothing to do with > RTX/non-RTX SSRCs, since they can't be distinguished between (so I think the > test is confusing, as it looks like it should handle the RTX SSRC differently). Done. https://codereview.webrtc.org/1226093002/diff/20001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2_unittest.cc:2600: for (int payload_type : { kDefaultRtxVp8PlType, kDefaultUlpfecType, 125 }) { On 2015/07/09 10:38:43, pbos-webrtc wrote: > Looks like you can't get away with this sir. :) Aww, sadtimes. It's gone now anyways. https://codereview.webrtc.org/1226093002/diff/20001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2_unittest.cc:2607: ASSERT_EQ(0u, fake_call_->GetVideoReceiveStreams().size()) On 2015/07/09 10:38:43, pbos-webrtc wrote: > Is there a test asserting that we do create it for VP8? If there is I'd like you > to consolidate them into one (if not, add one for VP8): > > void WebRtcVideoChannel2Test::ReceiveUnsignalledSsrc(uint8_t payload_type, bool > expect_created_receive_stream); > > Then do four different instantiations, but expect only the codec one to succeed, > so fail on RTX, FEC etc. ? DefaultReceiveStreamReconfiguresToUseRtx does, but I can make an explicit set and refactor it to use a shared function. PTAL.
lgtm, thanks Noah :) https://codereview.webrtc.org/1226093002/diff/40001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/1226093002/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2_unittest.cc:2610: if (expect_created_receive_stream) { EXPECT_EQ https://codereview.webrtc.org/1226093002/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2_unittest.cc:2615: ASSERT_EQ(0u, fake_call_->GetVideoReceiveStreams().size()) EXPECT_EQ
Done, thanks! https://codereview.webrtc.org/1226093002/diff/40001/talk/media/webrtc/webrtcv... File talk/media/webrtc/webrtcvideoengine2_unittest.cc (right): https://codereview.webrtc.org/1226093002/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2_unittest.cc:2610: if (expect_created_receive_stream) { On 2015/07/09 23:03:09, pbos-webrtc wrote: > EXPECT_EQ Done. https://codereview.webrtc.org/1226093002/diff/40001/talk/media/webrtc/webrtcv... talk/media/webrtc/webrtcvideoengine2_unittest.cc:2615: ASSERT_EQ(0u, fake_call_->GetVideoReceiveStreams().size()) On 2015/07/09 23:03:09, pbos-webrtc wrote: > EXPECT_EQ Done.
The CQ bit was checked by noahric@chromium.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/1226093002/#ps60001 (title: "ASSERT_EQ->EXPECT_EQ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1226093002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/d10a68e7974a29b26d6c926e6f137255f3c173be Cr-Commit-Position: refs/heads/master@{#9566} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
