Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2888093002:
New class RtxReceiveStream. (Closed)
|
Created:
3 years, 7 months ago by nisse-webrtc Modified:
3 years, 7 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun, stefan-webrtc, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionNew class RtxReceiveStream.
BUG=webrtc:7135
Review-Url: https://codereview.webrtc.org/2888093002
Cr-Commit-Position: refs/heads/master@{#18212}
Committed: https://chromium.googlesource.com/external/webrtc/+/eed52bff8d3e2d83207da624ec0a8f6441f1c98f
Patch Set 1 #
Total comments: 18
Patch Set 2 : Address comments. #
Total comments: 2
Patch Set 3 : Delete left-over const. #Patch Set 4 : Add a few tests. #
Total comments: 14
Patch Set 5 : Add test of RTP header extension. #Patch Set 6 : Address comments. #
Total comments: 6
Patch Set 7 : Address more comments. #
Messages
Total messages: 25 (9 generated)
nisse@webrtc.org changed reviewers: + danilchap@webrtc.org
PTAL. Not yet wired up.
looks good, just some simple unittests will be nice. https://codereview.webrtc.org/2888093002/diff/1/webrtc/call/rtx_receive_strea... File webrtc/call/rtx_receive_stream.cc (right): https://codereview.webrtc.org/2888093002/diff/1/webrtc/call/rtx_receive_strea... webrtc/call/rtx_receive_stream.cc:18: const std::map<int, int>& rtx_payload_type_map, may be pass by value and move when settings - might save extra copy: rtx_payload_type_map_(std::move(rtx_payload_type_map)), https://codereview.webrtc.org/2888093002/diff/1/webrtc/call/rtx_receive_strea... webrtc/call/rtx_receive_stream.cc:28: return; may be add a comment why drop packets silently (Think there was one in current rtx unwind code) https://codereview.webrtc.org/2888093002/diff/1/webrtc/call/rtx_receive_strea... webrtc/call/rtx_receive_stream.cc:36: media_packet.CopyHeaderFrom(rtx_packet); This is good: CopyHeaderFrom copies all extensions too. https://codereview.webrtc.org/2888093002/diff/1/webrtc/call/rtx_receive_strea... webrtc/call/rtx_receive_stream.cc:39: media_packet.SetSequenceNumber((payload[0] << 8) + payload[1]); uint8_t + uint8_t will give you uint8_t, not uint16_t may be use ByteReader from rtp_rtcp/source/byte_io.h if build rules allow to include it: SetSequenceNumber(ByteReader<uint16_t>::ReadBigEndian(payload.data())); https://codereview.webrtc.org/2888093002/diff/1/webrtc/call/rtx_receive_strea... webrtc/call/rtx_receive_stream.cc:41: may be add payload = payload.subview(kRtxHeaderSize); after sequence_number extracted from 1st two bytes. Then code below doesn't need to add/remove kRtxHeaderSize both for payload.data() and payload.size() https://codereview.webrtc.org/2888093002/diff/1/webrtc/call/rtx_receive_strea... webrtc/call/rtx_receive_stream.cc:45: return; may be RTC_DCHECK or LOG(LS_ERROR): This might happen only in very strange setup: when rtx_packet has larger size and capacity than default capacity (media_packet created with).
Addressed most comments. But no tests yet. https://codereview.webrtc.org/2888093002/diff/1/webrtc/call/rtx_receive_strea... File webrtc/call/rtx_receive_stream.cc (right): https://codereview.webrtc.org/2888093002/diff/1/webrtc/call/rtx_receive_strea... webrtc/call/rtx_receive_stream.cc:18: const std::map<int, int>& rtx_payload_type_map, On 2017/05/17 12:13:05, danilchap wrote: > may be pass by value and move when settings - might save extra copy: > rtx_payload_type_map_(std::move(rtx_payload_type_map)), Done. A bit counter-intuitive, though. Typical case will be passing VideoReceiveStream::Config::Rtp::rtc_payload_types. https://codereview.webrtc.org/2888093002/diff/1/webrtc/call/rtx_receive_strea... webrtc/call/rtx_receive_stream.cc:28: return; On 2017/05/17 12:13:05, danilchap wrote: > may be add a comment why drop packets silently (Think there was one in current > rtx unwind code) I could add a comment, but I find no guidance in RTPPayloadRegistry::RestoreOriginalPacket. It does have some logic to log unknown payload types, which I haven't copied over. https://codereview.webrtc.org/2888093002/diff/1/webrtc/call/rtx_receive_strea... webrtc/call/rtx_receive_stream.cc:36: media_packet.CopyHeaderFrom(rtx_packet); On 2017/05/17 12:13:05, danilchap wrote: > This is good: CopyHeaderFrom copies all extensions too. Including the result of IdentifyExtensions? https://codereview.webrtc.org/2888093002/diff/1/webrtc/call/rtx_receive_strea... webrtc/call/rtx_receive_stream.cc:39: media_packet.SetSequenceNumber((payload[0] << 8) + payload[1]); On 2017/05/17 12:13:05, danilchap wrote: > uint8_t + uint8_t will give you uint8_t, not uint16_t Are you sure? Check the section on integer promotions on http://en.cppreference.com/w/cpp/language/implicit_conversion In particular, arithmetic operators do not accept types smaller than int as arguments, and integral promotions are automatically applied after lvalue-to-rvalue conversion, if applicable. This conversion always preserves the value. As I read this, it works the same as plain C: Everything is automatically promoted to int, which should be large enough. https://codereview.webrtc.org/2888093002/diff/1/webrtc/call/rtx_receive_strea... webrtc/call/rtx_receive_stream.cc:41: On 2017/05/17 12:13:05, danilchap wrote: > may be add > payload = payload.subview(kRtxHeaderSize); > after sequence_number extracted from 1st two bytes. > Then code below doesn't need to add/remove kRtxHeaderSize both for > payload.data() and payload.size() Done, but with a new variable |rtx_payload|. https://codereview.webrtc.org/2888093002/diff/1/webrtc/call/rtx_receive_strea... webrtc/call/rtx_receive_stream.cc:45: return; On 2017/05/17 12:13:05, danilchap wrote: > may be RTC_DCHECK or LOG(LS_ERROR): > > This might happen only in very strange setup: when rtx_packet has larger size > and capacity than default capacity (media_packet created with). Done.
https://codereview.webrtc.org/2888093002/diff/1/webrtc/call/rtx_receive_strea... File webrtc/call/rtx_receive_stream.cc (right): https://codereview.webrtc.org/2888093002/diff/1/webrtc/call/rtx_receive_strea... webrtc/call/rtx_receive_stream.cc:18: const std::map<int, int>& rtx_payload_type_map, On 2017/05/17 13:19:34, nisse-webrtc wrote: > On 2017/05/17 12:13:05, danilchap wrote: > > may be pass by value and move when settings - might save extra copy: > > rtx_payload_type_map_(std::move(rtx_payload_type_map)), > > Done. A bit counter-intuitive, though. Typical case will be passing > VideoReceiveStream::Config::Rtp::rtc_payload_types. std::move probably is counter-intuitive [that is implementation details though], but I can argue that passing by value rather than by const& is more intuitive. Anyway, it is not that important in this case, just might be good to have habbit to accept parameters by value when it might reduce number of copies at the cost of few cheap moves. https://codereview.webrtc.org/2888093002/diff/1/webrtc/call/rtx_receive_strea... webrtc/call/rtx_receive_stream.cc:28: return; On 2017/05/17 13:19:34, nisse-webrtc wrote: > On 2017/05/17 12:13:05, danilchap wrote: > > may be add a comment why drop packets silently (Think there was one in current > > rtx unwind code) > > I could add a comment, but I find no guidance in > RTPPayloadRegistry::RestoreOriginalPacket. > > It does have some logic to log unknown payload types, which I haven't copied > over. Sorry, can't find it either, probably I mixed it up with another comment. I think the reason why it is normal and thus doesn't require any log is because rtx packet without rtp payload are pure padding, processed by CC elsewhere and shouldn't be forwarded to the media sink. https://codereview.webrtc.org/2888093002/diff/1/webrtc/call/rtx_receive_strea... webrtc/call/rtx_receive_stream.cc:36: media_packet.CopyHeaderFrom(rtx_packet); On 2017/05/17 13:19:34, nisse-webrtc wrote: > On 2017/05/17 12:13:05, danilchap wrote: > > This is good: CopyHeaderFrom copies all extensions too. > > Including the result of IdentifyExtensions? yes. (You can verify that with a test) https://codereview.webrtc.org/2888093002/diff/1/webrtc/call/rtx_receive_strea... webrtc/call/rtx_receive_stream.cc:39: media_packet.SetSequenceNumber((payload[0] << 8) + payload[1]); On 2017/05/17 13:19:34, nisse-webrtc wrote: > On 2017/05/17 12:13:05, danilchap wrote: > > uint8_t + uint8_t will give you uint8_t, not uint16_t > > Are you sure? Check the section on integer promotions on > http://en.cppreference.com/w/cpp/language/implicit_conversion > > In particular, arithmetic operators do not accept types smaller > than int as arguments, and integral promotions are automatically > applied after lvalue-to-rvalue conversion, if applicable. This > conversion always preserves the value. > > As I read this, it works the same as plain C: Everything is automatically > promoted to int, which should be large enough. You right, I'm not sure. guess current code good as is. https://codereview.webrtc.org/2888093002/diff/1/webrtc/call/rtx_receive_strea... webrtc/call/rtx_receive_stream.cc:41: On 2017/05/17 13:19:34, nisse-webrtc wrote: > On 2017/05/17 12:13:05, danilchap wrote: > > may be add > > payload = payload.subview(kRtxHeaderSize); > > after sequence_number extracted from 1st two bytes. > > Then code below doesn't need to add/remove kRtxHeaderSize both for > > payload.data() and payload.size() > > Done, but with a new variable |rtx_payload|. That is even better! https://codereview.webrtc.org/2888093002/diff/20001/webrtc/call/rtx_receive_s... File webrtc/call/rtx_receive_stream.h (right): https://codereview.webrtc.org/2888093002/diff/20001/webrtc/call/rtx_receive_s... webrtc/call/rtx_receive_stream.h:23: const std::map<int, int> rtx_payload_type_map, remove const (it is helpful for const&, but when passing by value it is recommended not to write it at least in the declaration)
https://codereview.webrtc.org/2888093002/diff/20001/webrtc/call/rtx_receive_s... File webrtc/call/rtx_receive_stream.h (right): https://codereview.webrtc.org/2888093002/diff/20001/webrtc/call/rtx_receive_s... webrtc/call/rtx_receive_stream.h:23: const std::map<int, int> rtx_payload_type_map, On 2017/05/17 13:43:56, danilchap wrote: > remove const > (it is helpful for const&, but when passing by value it is recommended not to > write it at least in the declaration) Ooop, I left this in as an experiment, I would have expected std::move on a const value to give a compilation error, but I didn't get any (instead, things are always copied). See http://stackoverflow.com/questions/28595117/why-can-we-use-stdmove-on-a-const.... I'm removing const.
https://codereview.webrtc.org/2888093002/diff/1/webrtc/call/rtx_receive_strea... File webrtc/call/rtx_receive_stream.cc (right): https://codereview.webrtc.org/2888093002/diff/1/webrtc/call/rtx_receive_strea... webrtc/call/rtx_receive_stream.cc:36: media_packet.CopyHeaderFrom(rtx_packet); On 2017/05/17 13:43:55, danilchap wrote: > On 2017/05/17 13:19:34, nisse-webrtc wrote: > > On 2017/05/17 12:13:05, danilchap wrote: > > > This is good: CopyHeaderFrom copies all extensions too. > > > > Including the result of IdentifyExtensions? > > yes. > (You can verify that with a test) I've added a few tests, but extensions still missing.
On 2017/05/19 07:43:44, nisse-webrtc wrote: > I've added a few tests, but extensions still missing. And now also a test involving the CVO extension. PTAL.
lgtm More features (like logging missed payload with supression, or ensuring extensions are copied too [even if it is just adding a test]) can be added in follow up CLs. https://codereview.webrtc.org/2888093002/diff/60001/webrtc/call/rtx_receive_s... File webrtc/call/rtx_receive_stream_unittest.cc (right): https://codereview.webrtc.org/2888093002/diff/60001/webrtc/call/rtx_receive_s... webrtc/call/rtx_receive_stream_unittest.cc:27: constexpr int kMediaSSRC = 0x3333333; may be uint32_t for SSRC https://codereview.webrtc.org/2888093002/diff/60001/webrtc/call/rtx_receive_s... webrtc/call/rtx_receive_stream_unittest.cc:28: might be helpful to add constants for the original sequence number and payload too. (to avoid browsing up and bottom to ensure correct value are expected in the individual tests) Not a problem now, but might be an issue when more tests will be added. https://codereview.webrtc.org/2888093002/diff/60001/webrtc/call/rtx_receive_s... webrtc/call/rtx_receive_stream_unittest.cc:31: 98, // Payload type likely add dots at the end of the comments. https://codereview.webrtc.org/2888093002/diff/60001/webrtc/call/rtx_receive_s... webrtc/call/rtx_receive_stream_unittest.cc:41: std::map<int, int> PTMapping() { it is easier to read if you do not abbreviate: so may be name it PayloadTypeMapping() https://google.github.io/styleguide/cppguide.html#General_Naming_Rules https://codereview.webrtc.org/2888093002/diff/60001/webrtc/call/rtx_receive_s... webrtc/call/rtx_receive_stream_unittest.cc:42: std::map<int, int> m; I wonder if return {{kRtxPayloadType, kMediaPayloadType}}; would compile on all platforms. https://codereview.webrtc.org/2888093002/diff/60001/webrtc/call/rtx_receive_s... webrtc/call/rtx_receive_stream_unittest.cc:55: testing::StrictMock<MockRtpPacketSink> media_sink; personally I prefer to add using ::testing::_; using ::testing::StrictMock; and everything else used from testing namespace at the top of the unnamed namespace. https://codereview.webrtc.org/2888093002/diff/60001/webrtc/call/rtx_receive_s... webrtc/call/rtx_receive_stream_unittest.cc:61: OnRtpPacket(testing::AllOf( Alternative suggestion (not sure it is better though): EXPECT_CALL(media_sink, OnRtpPacket(_)).WillOnce(testing::Invoke([](const RtpPacketReceived& packet) { EXPECT_EQ(packet.SequenceNumber(), 0x5657); EXPECT_EQ(packet.Ssrc(), kMediaSSRC); EXPECT_THAT(packet.payload(), ElementsAre(0xee)); }); https://codereview.webrtc.org/2888093002/diff/60001/webrtc/call/rtx_receive_s... webrtc/call/rtx_receive_stream_unittest.cc:65: testing::Truly([](const RtpPacketReceived& packet) { Or... yet another (again, not sure it is more readable): instead of testing::Truly testing::Property(&RtpPacketReceived::Ssrc, kMediaSSRC)
https://codereview.webrtc.org/2888093002/diff/60001/webrtc/call/rtx_receive_s... File webrtc/call/rtx_receive_stream_unittest.cc (right): https://codereview.webrtc.org/2888093002/diff/60001/webrtc/call/rtx_receive_s... webrtc/call/rtx_receive_stream_unittest.cc:27: constexpr int kMediaSSRC = 0x3333333; On 2017/05/19 08:39:02, danilchap wrote: > may be uint32_t for SSRC Done. https://codereview.webrtc.org/2888093002/diff/60001/webrtc/call/rtx_receive_s... webrtc/call/rtx_receive_stream_unittest.cc:28: On 2017/05/19 08:39:03, danilchap wrote: > might be helpful to add constants for the original sequence number and payload > too. Added constant for the seqno. https://codereview.webrtc.org/2888093002/diff/60001/webrtc/call/rtx_receive_s... webrtc/call/rtx_receive_stream_unittest.cc:31: 98, // Payload type On 2017/05/19 08:39:03, danilchap wrote: > likely add dots at the end of the comments. Done. https://codereview.webrtc.org/2888093002/diff/60001/webrtc/call/rtx_receive_s... webrtc/call/rtx_receive_stream_unittest.cc:41: std::map<int, int> PTMapping() { On 2017/05/19 08:39:03, danilchap wrote: > it is easier to read if you do not abbreviate: so may be name it > PayloadTypeMapping() > https://google.github.io/styleguide/cppguide.html#General_Naming_Rules Done. https://codereview.webrtc.org/2888093002/diff/60001/webrtc/call/rtx_receive_s... webrtc/call/rtx_receive_stream_unittest.cc:55: testing::StrictMock<MockRtpPacketSink> media_sink; On 2017/05/19 08:39:02, danilchap wrote: > personally I prefer to add > using ::testing::_; > using ::testing::StrictMock; > and everything else used from testing namespace at the top of the unnamed > namespace. Done for some of the symbols. https://codereview.webrtc.org/2888093002/diff/60001/webrtc/call/rtx_receive_s... webrtc/call/rtx_receive_stream_unittest.cc:61: OnRtpPacket(testing::AllOf( On 2017/05/19 08:39:02, danilchap wrote: > Alternative suggestion (not sure it is better though): > EXPECT_CALL(media_sink, OnRtpPacket(_)).WillOnce(testing::Invoke([](const > RtpPacketReceived& packet) { > EXPECT_EQ(packet.SequenceNumber(), 0x5657); > EXPECT_EQ(packet.Ssrc(), kMediaSSRC); > EXPECT_THAT(packet.payload(), ElementsAre(0xee)); > }); I've changed to this alternative. It provides clearer messages when expectations fail.
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org Link to the patchset: https://codereview.webrtc.org/2888093002/#ps100001 (title: "Address comments.")
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: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/17215)
few more optional comments https://codereview.webrtc.org/2888093002/diff/100001/webrtc/call/rtx_receive_... File webrtc/call/rtx_receive_stream_unittest.cc (right): https://codereview.webrtc.org/2888093002/diff/100001/webrtc/call/rtx_receive_... webrtc/call/rtx_receive_stream_unittest.cc:84: EXPECT_EQ(kMediaPayloadType, packet.PayloadType()); quote from gtest docs: "Historical note: Before February 2016 *_EQ had a convention of calling it as ASSERT_EQ(expected, actual), so lots of existing code uses this order. Now *_EQ treats both parameters in the same way." so may be write EXPECT_EQ(packet.PayloadType(), kMediaPayloadType) for consistency with EXPECT_THAT. https://codereview.webrtc.org/2888093002/diff/100001/webrtc/call/rtx_receive_... webrtc/call/rtx_receive_stream_unittest.cc:108: TEST(RtxReceiveStreamTest, RestoresCVOExtension) { May be CopiesRtpHeaderExtenions? (even if you test with one extension, rtx still copies all of them equally. Since one of the use of test names is to describe what class able to do, might be better to make the name more generic) https://codereview.webrtc.org/2888093002/diff/100001/webrtc/call/rtx_receive_... webrtc/call/rtx_receive_stream_unittest.cc:116: rtx_packet.IdentifyExtensions(extension_map); instead you may create rtx_packet with known extension map: RtpPacketReceived rtx_packet(&extension_map); EXPECT_TRUE(rtx_packet.Parse(AV(kRtxPacketWithCVO)));
nisse@webrtc.org changed reviewers: + stefan@webrtc.org
Stefan: PTAL. https://codereview.webrtc.org/2888093002/diff/100001/webrtc/call/rtx_receive_... File webrtc/call/rtx_receive_stream_unittest.cc (right): https://codereview.webrtc.org/2888093002/diff/100001/webrtc/call/rtx_receive_... webrtc/call/rtx_receive_stream_unittest.cc:84: EXPECT_EQ(kMediaPayloadType, packet.PayloadType()); On 2017/05/19 11:16:01, danilchap wrote: > quote from gtest docs: > "Historical note: Before February 2016 *_EQ had a convention of calling it as > ASSERT_EQ(expected, actual), so lots of existing code uses this order. Now *_EQ > treats both parameters in the same way." > > so may be write EXPECT_EQ(packet.PayloadType(), kMediaPayloadType) for > consistency with EXPECT_THAT. Done. https://codereview.webrtc.org/2888093002/diff/100001/webrtc/call/rtx_receive_... webrtc/call/rtx_receive_stream_unittest.cc:108: TEST(RtxReceiveStreamTest, RestoresCVOExtension) { On 2017/05/19 11:16:01, danilchap wrote: > May be CopiesRtpHeaderExtenions? > (even if you test with one extension, rtx still copies all of them equally. > Since one of the use of test names is to describe what class able to do, might > be better to make the name more generic) Done. https://codereview.webrtc.org/2888093002/diff/100001/webrtc/call/rtx_receive_... webrtc/call/rtx_receive_stream_unittest.cc:116: rtx_packet.IdentifyExtensions(extension_map); On 2017/05/19 11:16:01, danilchap wrote: > instead you may create rtx_packet with known extension map: > RtpPacketReceived rtx_packet(&extension_map); > EXPECT_TRUE(rtx_packet.Parse(AV(kRtxPacketWithCVO))); Done.
lgtm
The CQ bit was checked by nisse@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org Link to the patchset: https://codereview.webrtc.org/2888093002/#ps120001 (title: "Address more comments.")
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": 1495198595906600,
"parent_rev": "31bd224f356b6dff18664370abd475351de4c683", "commit_rev":
"eed52bff8d3e2d83207da624ec0a8f6441f1c98f"}
Message was sent while issue was closed.
Description was changed from ========== New class RtxReceiveStream. BUG=webrtc:7135 ========== to ========== New class RtxReceiveStream. BUG=webrtc:7135 Review-Url: https://codereview.webrtc.org/2888093002 Cr-Commit-Position: refs/heads/master@{#18212} Committed: https://chromium.googlesource.com/external/webrtc/+/eed52bff8d3e2d83207da624e... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/external/webrtc/+/eed52bff8d3e2d83207da624e... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
