Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1841453004:
RtpPacket class introduced. (Closed)
|
Created:
4 years, 8 months ago by danilchap Modified:
4 years, 8 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, stefan-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRtpPacket class introduced.
BUG=webrtc:1994, webrtc:5261
Committed: https://crrev.com/1edb7ab7bd38318e039e99e0b1577de728e9a386
Cr-Commit-Position: refs/heads/master@{#12444}
Patch Set 1 #
Total comments: 38
Patch Set 2 : #Patch Set 3 : style fixes #
Total comments: 22
Patch Set 4 : feedback #Patch Set 5 : Added unittests and fuzzer #
Total comments: 6
Patch Set 6 : const -> constexpr #
Total comments: 2
Patch Set 7 : fix compile #Patch Set 8 : fix compile, try2 #Patch Set 9 : added empty lines #Messages
Total messages: 41 (10 generated)
danilchap@webrtc.org changed reviewers: + philipel@webrtc.org, terelius@webrtc.org
Samples where this class design is not comfortable to use are welcome. This CL use existent extension manager (with few tweaks) but in a way that make it easy to switch to a different extension manager. Sample usage (on send send side) in this CL: https://codereview.webrtc.org/1739273002/
https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc (right): https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:72: case MediaType::ANY: Why does MediaType::ANY support this extension, shouldn't it be for audio only? https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:178: case MediaType::ANY: Again, should MediaType::ANY support video orientation or should it just be for video? When is MediaType::ANY used? https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_packet.cc (right): https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:86: bool Packet::Marker() const { Functions with very low cost (if it is reasonable to call it in a loop) should be named_liked_this_whatever_this_is_called(). Rename this to Packet::marker() https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:90: uint8_t Packet::PayloadType() const { payload_type() https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:95: return sequence_number_; sequence_number() https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:98: uint32_t Packet::Timestamp() const { timestamp() https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:102: uint32_t Packet::Ssrc() const { ssrc() https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:141: size_t Packet::HeadersSize() const { header_size() https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:145: size_t Packet::PayloadSize() const { payload_size() https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:149: size_t Packet::PaddingSize() const { padding_size() https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:254: << " left in buffer."; " bytes left in buffer."; https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:266: WriteAt(offset, random->Rand<uint8_t>()); remove {} https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_packet.h (right): https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.h:98: // Find and extension, based on the type field of the parameter. Find an extension https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.h:114: uint8_t* WriteAt(size_t offset) { return buffer_.data() + offset; } Move implementation to .cc file. https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.h:115: void WriteAt(size_t offset, uint8_t byte) { buffer_.data()[offset] = byte; } Ditto. https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.h:163: class RtpPacket : public rtp::Packet { Rename to SendRtpPacket or something that indicates that this is the class to use for sending an rtp packet. https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.h:167: : Packet(extensions, capacity) {} Why is the implementation for RtpPacket and ReceiveRtpPacket in the .h file?
https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_packet.cc (right): https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:86: bool Packet::Marker() const { Functions with very low cost (if it is reasonable to call it in a loop) should be named_liked_this_whatever_this_is_called(). Rename this to Packet::marker() https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:90: uint8_t Packet::PayloadType() const { payload_type() https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:102: uint32_t Packet::Ssrc() const { ssrc() https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:141: size_t Packet::HeadersSize() const { header_size() https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:145: size_t Packet::PayloadSize() const { payload_size() https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:266: WriteAt(offset, random->Rand<uint8_t>()); remove {} https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_packet.h (right): https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.h:98: // Find and extension, based on the type field of the parameter. Find an extension https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.h:114: uint8_t* WriteAt(size_t offset) { return buffer_.data() + offset; } Move implementation to .cc file. https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.h:115: void WriteAt(size_t offset, uint8_t byte) { buffer_.data()[offset] = byte; } Ditto. https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.h:163: class RtpPacket : public rtp::Packet { Rename to SendRtpPacket or something that indicates that this is the class to use for sending an rtp packet. https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.h:167: : Packet(extensions, capacity) {} Why is the implementation for RtpPacket and ReceiveRtpPacket in the .h file?
I think a packet class will be very useful! Some questions/comments below. In particular, could you think about how to initially parse a packet when you don't know the SSRC? https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_packet.cc (right): https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:263: *reinterpret_cast<uint32_t*>(WriteAt(offset)) = random->Rand<uint32_t>(); Do you know that the destination is aligned to a 32-bit boundary, or could there be issues on architectures with alignment requirements? https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:266: WriteAt(offset, random->Rand<uint8_t>()); On 2016/04/13 12:01:19, philipel wrote: > remove {} I believe different parts of the code use different conventions, and both are allowed by the style guide. As long as it is consistent within the file/module I think it is fine. https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:405: // Can't add new extension after payload/paddin was set. nit: padding https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_packet.h (right): https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.h:85: explicit Packet(const ExtensionManager* extensions); So you have to know what header extensions are used before you create the packet object? I wonder if this could be an issue. If you don't know the SSRC of the packet, do you create a temporary packet object with no registered extensions and copy the buffer there just to get the SSRC? https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.h:96: // Does not touch packet own buffer, leaving packet in desync state on return. I don't understand what this comment means. Updates all header fields of the Packet object, but does not copy the packet data to the buffer? Which functions am I allowed to call after ParseBuffer()? https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.h:110: // If not found and allocations fails, false is returned and paramter remains nit: parameter
Patchset #2 (id:20001) has been deleted
Thank you for review, code and comments adjusted, PTAL https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc (right): https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:72: case MediaType::ANY: On 2016/04/13 12:01:18, philipel wrote: > Why does MediaType::ANY support this extension, shouldn't it be for audio only? MediaType ANY seems used when caller doesn't not sure what media it is. E.g. Call forwards MediaType::ANY both to audio and video streams. so IsSupportedFor(MediaType::ANY) this code treat as question 'Are there ANY media type that support this extension'. https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_packet.cc (right): https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:86: bool Packet::Marker() const { On 2016/04/13 12:01:19, philipel wrote: > Functions with very low cost (if it is reasonable to call it in a loop) should > be named_liked_this_whatever_this_is_called(). Rename this to Packet::marker() I'm not sure current implementation would stay. May be a bit later it would change to bool Packet::Marker() { return buffer_[1] & 0x80 != 0; } to make Packet class smaller. Do not want to guarantee this function always be trivial. https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:141: size_t Packet::HeadersSize() const { On 2016/04/13 12:01:19, philipel wrote: > header_size() Done. https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:145: size_t Packet::PayloadSize() const { On 2016/04/13 12:01:19, philipel wrote: > payload_size() Done. https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:149: size_t Packet::PaddingSize() const { On 2016/04/13 12:01:18, philipel wrote: > padding_size() Done. https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:254: << " left in buffer."; On 2016/04/13 12:01:18, philipel wrote: > " bytes left in buffer."; Done. https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:263: *reinterpret_cast<uint32_t*>(WriteAt(offset)) = random->Rand<uint32_t>(); On 2016/04/13 13:17:00, terelius wrote: > Do you know that the destination is aligned to a 32-bit boundary, or could there > be issues on architectures with alignment requirements? True, though usually it is, this code can't assume that. Changed to single loop both for compatibility and clarity. Unlikely performance could be an issue here. https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:405: // Can't add new extension after payload/paddin was set. On 2016/04/13 13:17:00, terelius wrote: > nit: padding Done. https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_packet.h (right): https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.h:85: explicit Packet(const ExtensionManager* extensions); On 2016/04/13 13:17:00, terelius wrote: > So you have to know what header extensions are used before you create the packet > object? I wonder if this could be an issue. If you don't know the SSRC of the > packet, do you create a temporary packet object with no registered extensions > and copy the buffer there just to get the SSRC? Good point. Adjusted. Added function SetExtensionManager that can be called after packet is parsed. https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.h:96: // Does not touch packet own buffer, leaving packet in desync state on return. On 2016/04/13 13:17:00, terelius wrote: > I don't understand what this comment means. Updates all header fields of the > Packet object, but does not copy the packet data to the buffer? > > Which functions am I allowed to call after ParseBuffer()? this function is private, you shouldn't call it all, but instead use one of the Parse functions above. both of them call this function and then ensure buffer match header fields. I try to make more clear this helper function leave Packet in invalid state on return. https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.h:98: // Find and extension, based on the type field of the parameter. On 2016/04/13 12:01:19, philipel wrote: > Find an extension Done. https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.h:110: // If not found and allocations fails, false is returned and paramter remains On 2016/04/13 13:17:00, terelius wrote: > nit: parameter Done. https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.h:114: uint8_t* WriteAt(size_t offset) { return buffer_.data() + offset; } On 2016/04/13 12:01:19, philipel wrote: > Move implementation to .cc file. Done. https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.h:115: void WriteAt(size_t offset, uint8_t byte) { buffer_.data()[offset] = byte; } On 2016/04/13 12:01:19, philipel wrote: > Ditto. Done. https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.h:163: class RtpPacket : public rtp::Packet { On 2016/04/13 12:01:19, philipel wrote: > Rename to SendRtpPacket or something that indicates that this is the class to > use for sending an rtp packet. Done. https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.h:167: : Packet(extensions, capacity) {} On 2016/04/13 12:01:19, philipel wrote: > Why is the implementation for RtpPacket and ReceiveRtpPacket in the .h file? Definition is here for no good reason, moved to own files. Implementation is in .h because there is practically no implementation: all own functions are trivial accessors, constructor forwards directly to rtp::Packet constructor. There is no plan to add any non-trivial function.
lg, but I just think the style in rtp_packet.cc is a bit inconsistent. https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc (right): https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_header_extensions.cc:72: case MediaType::ANY: On 2016/04/13 16:18:33, danilchap wrote: > On 2016/04/13 12:01:18, philipel wrote: > > Why does MediaType::ANY support this extension, shouldn't it be for audio > only? > > MediaType ANY seems used when caller doesn't not sure what media it is. E.g. > Call forwards MediaType::ANY both to audio and video streams. > so IsSupportedFor(MediaType::ANY) this code treat as question 'Are there ANY > media type that support this extension'. Acknowledged. https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_packet.cc (right): https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:86: bool Packet::Marker() const { On 2016/04/13 16:18:33, danilchap wrote: > On 2016/04/13 12:01:19, philipel wrote: > > Functions with very low cost (if it is reasonable to call it in a loop) should > > be named_liked_this_whatever_this_is_called(). Rename this to Packet::marker() > > I'm not sure current implementation would stay. > May be a bit later it would change to > bool Packet::Marker() { return buffer_[1] & 0x80 != 0; } > to make Packet class smaller. > > Do not want to guarantee this function always be trivial. Acknowledged. https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:266: WriteAt(offset, random->Rand<uint8_t>()); On 2016/04/13 13:17:00, terelius wrote: > On 2016/04/13 12:01:19, philipel wrote: > > remove {} > > I believe different parts of the code use different conventions, and both are > allowed by the style guide. As long as it is consistent within the file/module I > think it is fine. True, but then other parts of the code has to be updated. For example line 293, 297, 308 and more. https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_packet.h (right): https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.h:167: : Packet(extensions, capacity) {} On 2016/04/13 16:18:34, danilchap wrote: > On 2016/04/13 12:01:19, philipel wrote: > > Why is the implementation for RtpPacket and ReceiveRtpPacket in the .h file? > > Definition is here for no good reason, moved to own files. > Implementation is in .h because there is practically no implementation: all own > functions are trivial accessors, > constructor forwards directly to rtp::Packet constructor. There is no plan to > add any non-trivial function. Acknowledged.
https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtp_packet.cc (right): https://codereview.webrtc.org/1841453004/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:266: WriteAt(offset, random->Rand<uint8_t>()); On 2016/04/14 10:32:02, philipel wrote: > On 2016/04/13 13:17:00, terelius wrote: > > On 2016/04/13 12:01:19, philipel wrote: > > > remove {} > > > > I believe different parts of the code use different conventions, and both are > > allowed by the style guide. As long as it is consistent within the file/module > I > > think it is fine. > > True, but then other parts of the code has to be updated. For example line 293, > 297, 308 and more. Good point! Updated.
lgtm, just one final nit though. https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_packet.cc (right): https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:390: if (num_extensions_ >= kMaxExtensionHeaders) {} :)
https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_packet.h (right): https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet.h:33: void SetExtensionManager(const ExtensionManager* extensions); Please document what this functions does or how it will be used. If I have called Packet packet(extension_manager); packet.Parse(buffer) packet.SetExtensionManager(new_extension_manager); can I start using the new extensions immediately, or do I have to call some other function to re-parse them from the buffer first? https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet.h:84: explicit Packet(const ExtensionManager* extensions); Am I allowed to call the constructors with a nullptr as argument? Is there a default constructor?
stefan@webrtc.org changed reviewers: + sprang@webrtc.org, stefan@webrtc.org
I think it would be good if Erik took a look at this too since he did initial work on the rtp packet class.
https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_packet.cc (right): https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:221: WriteAt(1, data()[1] | 0x80); Would it make sense to instead have a Serialize function which can be called once before we pass the packet to the network? I think the code might be cleaner since it would isolate the serializing code into one function, instead of having it split between all functions.
Nice! I'd welcome basic unit tests in this cl. https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_packet.cc (right): https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:221: WriteAt(1, data()[1] | 0x80); On 2016/04/14 13:00:29, stefan-webrtc (holmer) wrote: > Would it make sense to instead have a Serialize function which can be called > once before we pass the packet to the network? I think the code might be cleaner > since it would isolate the serializing code into one function, instead of having > it split between all functions. I tried that once it quickly became difficult, especially with the way our code currently works. We'd have to do a big-bang replacement to use this class everywhere at once. By always writing through to the underlying buffer it's easy to continue passing just the buffer around in places that haven't been updated yet. https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:327: // eXtension remove comment https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:390: if (num_extensions_ >= kMaxExtensionHeaders) Should we really silently ignore this? maybe log warning and/or return false? https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:401: } Log warning if profile isn't one byte extension header? https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_packet.h (right): https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet.h:33: void SetExtensionManager(const ExtensionManager* extensions); When do we want to dynamically change extension manager? Feels like it would make things easier if we could only set it at construction? https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet.h:84: explicit Packet(const ExtensionManager* extensions); On 2016/04/14 11:36:41, terelius wrote: > Am I allowed to call the constructors with a nullptr > as argument? Is there a default constructor? Looks like no (there's a dcheck) and no. You'll probably not use this directly but rely on the send/receive subclasses, right?
https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_packet.h (right): https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet.h:33: void SetExtensionManager(const ExtensionManager* extensions); On 2016/04/14 13:41:50, språng wrote: > When do we want to dynamically change extension manager? Feels like it would > make things easier if we could only set it at construction? Though afaik you need the SSRC of the incoming packet to know what extensions are used, and to get the SSRC you have to parse the header. https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet.h:84: explicit Packet(const ExtensionManager* extensions); On 2016/04/14 13:41:50, språng wrote: > On 2016/04/14 11:36:41, terelius wrote: > > Am I allowed to call the constructors with a nullptr > > as argument? Is there a default constructor? > > Looks like no (there's a dcheck) and no. > You'll probably not use this directly but rely on the send/receive subclasses, > right? I can't see any DCHECK in the constructor, so I assume it is allowed. Also, it appears to me that we *need* to be able to parse just the standard header without any extensions, so I think there should be a way to construct a packet without an ExtensionManager.
https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_packet.cc (right): https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:221: WriteAt(1, data()[1] | 0x80); On 2016/04/14 13:00:29, stefan-webrtc (holmer) wrote: > Would it make sense to instead have a Serialize function which can be called > once before we pass the packet to the network? I think the code might be cleaner > since it would isolate the serializing code into one function, instead of having > it split between all functions. Packet class is all about serializing/deserializing, it has no other features. Introducing Serialize function would require to keep extension values in some deserialized form, which is not trivial to make nice. Neither is trivial to store payload and serialize it later without extra memcopies. https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:327: // eXtension On 2016/04/14 13:41:50, språng wrote: > remove comment Done. https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:390: if (num_extensions_ >= kMaxExtensionHeaders) On 2016/04/14 13:41:50, språng wrote: > Should we really silently ignore this? > maybe log warning and/or return false? Done: so many extensions means packet is invalid, returning false now. https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:401: } On 2016/04/14 13:41:50, språng wrote: > Log warning if profile isn't one byte extension header? Done. https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_packet.h (right): https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet.h:33: void SetExtensionManager(const ExtensionManager* extensions); On 2016/04/14 13:41:50, språng wrote: > When do we want to dynamically change extension manager? Feels like it would > make things easier if we could only set it at construction? After parsing in case of bundling: first need to parse rtp packet to find out ssrc, then decide which extension mapping should be used: audio or video. https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet.h:84: explicit Packet(const ExtensionManager* extensions); On 2016/04/14 11:36:41, terelius wrote: > Am I allowed to call the constructors with a nullptr > as argument? Is there a default constructor? There is no default constructor, but it is allowed to create packet without extensions (either if code do not plan to use any in create mode (or during testing), or do not know which to use in case of parsing. RtpPacketReceived has a default constructor that create RtpPacket without extensions. And yes, currently rtp::Packet is not expected to be used directly, but through derived classes.
https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_packet.h (right): https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet.h:33: void SetExtensionManager(const ExtensionManager* extensions); On 2016/04/14 14:13:19, danilchap wrote: > On 2016/04/14 13:41:50, språng wrote: > > When do we want to dynamically change extension manager? Feels like it would > > make things easier if we could only set it at construction? > > After parsing in case of bundling: first need to parse rtp packet to find out > ssrc, > then decide which extension mapping should be used: audio or video. Could you add a comment that clarifies that changing the ExtensionManager does not modify what's written to the buffer, but allows you to access the new extensions through GetExtension(). Perhaps also document why this function was deemed useful. https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet.h:84: explicit Packet(const ExtensionManager* extensions); On 2016/04/14 14:13:19, danilchap wrote: > On 2016/04/14 11:36:41, terelius wrote: > > Am I allowed to call the constructors with a nullptr > > as argument? Is there a default constructor? > > There is no default constructor, but it is allowed to create packet without > extensions (either if code do not plan to use any in create mode (or during > testing), or do not know which to use in case of parsing. > > RtpPacketReceived has a default constructor that create RtpPacket without > extensions. > > And yes, currently rtp::Packet is not expected to be used directly, but through > derived classes. Acknowledged. Could you add a short comment about this, e.g. "If the packet is constructed with |extensions|==nullptr, then it will only read/write the fixed header."?
https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... File webrtc/modules/rtp_rtcp/source/rtp_packet.h (right): https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet.h:33: void SetExtensionManager(const ExtensionManager* extensions); On 2016/04/14 14:36:03, terelius wrote: > On 2016/04/14 14:13:19, danilchap wrote: > > On 2016/04/14 13:41:50, språng wrote: > > > When do we want to dynamically change extension manager? Feels like it would > > > make things easier if we could only set it at construction? > > > > After parsing in case of bundling: first need to parse rtp packet to find out > > ssrc, > > then decide which extension mapping should be used: audio or video. > > Could you add a comment that clarifies that changing the ExtensionManager does > not modify what's written to the buffer, but allows you to access the new > extensions through GetExtension(). Perhaps also document why this function was > deemed useful. Added, both in text and as a test. https://codereview.webrtc.org/1841453004/diff/60001/webrtc/modules/rtp_rtcp/s... webrtc/modules/rtp_rtcp/source/rtp_packet.h:84: explicit Packet(const ExtensionManager* extensions); On 2016/04/14 14:36:03, terelius wrote: > On 2016/04/14 14:13:19, danilchap wrote: > > On 2016/04/14 11:36:41, terelius wrote: > > > Am I allowed to call the constructors with a nullptr > > > as argument? Is there a default constructor? > > > > There is no default constructor, but it is allowed to create packet without > > extensions (either if code do not plan to use any in create mode (or during > > testing), or do not know which to use in case of parsing. > > > > RtpPacketReceived has a default constructor that create RtpPacket without > > extensions. > > > > And yes, currently rtp::Packet is not expected to be used directly, but > through > > derived classes. > > Acknowledged. Could you add a short comment about this, e.g. "If the packet is > constructed with |extensions|==nullptr, then it will only read/write the fixed > header."? Done.
Description was changed from ========== RtpPacket class introdcued. BUG=webrtc:1994, webrtc:5261 ========== to ========== RtpPacket class introdcued. BUG=webrtc:1994, webrtc:5261 ==========
danilchap@webrtc.org changed reviewers: + pbos@webrtc.org
+pbos for test/fuzzers
Thanks! A couple of question though: https://codereview.webrtc.org/1841453004/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_packet.cc (right): https://codereview.webrtc.org/1841453004/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:177: return buffer_.size(); Whats the reason for changing to .size() instead of capacity() here? https://codereview.webrtc.org/1841453004/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_packet.cc:486: RTC_DCHECK(num_extensions_ < kMaxExtensionHeaders); Why not use DCHECK_LT here?
On 2016/04/19 14:18:21, terelius wrote: > Thanks! A couple of question though: > > https://codereview.webrtc.org/1841453004/diff/100001/webrtc/modules/rtp_rtcp/... > File webrtc/modules/rtp_rtcp/source/rtp_packet.cc (right): > > https://codereview.webrtc.org/1841453004/diff/100001/webrtc/modules/rtp_rtcp/... > webrtc/modules/rtp_rtcp/source/rtp_packet.cc:177: return buffer_.size(); > Whats the reason for changing to .size() instead of capacity() here? short answer with an excuse: because it make no difference but make it consistent with Parse(rtc::Buffer). honest answer: While sending packet it is encoded with srtp. srtp need extra few bytes in the buffer to do the encoding. Currently buffer is copied into bigger buffer. By making difference between size available for packet creation and buffer size it would be possible to avoid that memcpy. > https://codereview.webrtc.org/1841453004/diff/100001/webrtc/modules/rtp_rtcp/... > webrtc/modules/rtp_rtcp/source/rtp_packet.cc:486: RTC_DCHECK(num_extensions_ < > kMaxExtensionHeaders); > Why not use DCHECK_LT here? DCHECK_LT take const& and so require parameter to have an address. To do this kMaxExtensionHeaders defintion should be moved from .h to .cc files making it less visible and unavailable at compile time. I find making DCHECK error message less descriptive a smaller problem.
Acknowledged. LGTM
Looks good! Have you looked into doing any fuzz testing of this? https://codereview.webrtc.org/1841453004/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_packet_unittest.cc (right): https://codereview.webrtc.org/1841453004/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_packet_unittest.cc:33: // const size_t kMaxPaddingSize = 224u; Unused?
On 2016/04/20 07:49:36, språng wrote: > Looks good! > Have you looked into doing any fuzz testing of this? Yes: 'webrtc/test/fuzzers/rtp_packet_fuzzer.cc' do just that. Run it locally for a night - no issues found.
https://codereview.webrtc.org/1841453004/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_packet_unittest.cc (right): https://codereview.webrtc.org/1841453004/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_packet_unittest.cc:33: // const size_t kMaxPaddingSize = 224u; On 2016/04/20 07:49:35, språng wrote: > Unused? Oops, made used.
webrtc/test/fuzzers lgtm https://codereview.webrtc.org/1841453004/diff/120001/webrtc/test/fuzzers/rtp_... File webrtc/test/fuzzers/rtp_packet_fuzzer.cc (right): https://codereview.webrtc.org/1841453004/diff/120001/webrtc/test/fuzzers/rtp_... webrtc/test/fuzzers/rtp_packet_fuzzer.cc:10: #include "webrtc/base/checks.h" No checks used in this file, shouldn't be needed
lgtm
https://codereview.webrtc.org/1841453004/diff/120001/webrtc/test/fuzzers/rtp_... File webrtc/test/fuzzers/rtp_packet_fuzzer.cc (right): https://codereview.webrtc.org/1841453004/diff/120001/webrtc/test/fuzzers/rtp_... webrtc/test/fuzzers/rtp_packet_fuzzer.cc:10: #include "webrtc/base/checks.h" On 2016/04/20 09:41:24, pbos-webrtc wrote: > No checks used in this file, shouldn't be needed Done.
Description was changed from ========== RtpPacket class introdcued. BUG=webrtc:1994, webrtc:5261 ========== to ========== RtpPacket class introduced. BUG=webrtc:1994, webrtc:5261 ==========
lgtm, but please consider adding some empty lines in rtp_packet.h https://codereview.webrtc.org/1841453004/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_packet.h (right): https://codereview.webrtc.org/1841453004/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_packet.h:33: // Parse and move given buffer into Packet. I prefer to have a new line before each comment so that it's easier to see where each new method begins. Not needed for methods without comments though.
https://codereview.webrtc.org/1841453004/diff/100001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtp_packet.h (right): https://codereview.webrtc.org/1841453004/diff/100001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtp_packet.h:33: // Parse and move given buffer into Packet. On 2016/04/20 10:53:40, stefan-webrtc (holmer) wrote: > I prefer to have a new line before each comment so that it's easier to see where > each new method begins. Not needed for methods without comments though. Done, it look nicer with extra empty lines.
The CQ bit was checked by danilchap@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org, terelius@webrtc.org, sprang@webrtc.org, pbos@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1841453004/#ps180001 (title: "added empty lines")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1841453004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1841453004/180001
Message was sent while issue was closed.
Description was changed from ========== RtpPacket class introduced. BUG=webrtc:1994, webrtc:5261 ========== to ========== RtpPacket class introduced. BUG=webrtc:1994, webrtc:5261 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== RtpPacket class introduced. BUG=webrtc:1994, webrtc:5261 ========== to ========== RtpPacket class introduced. BUG=webrtc:1994, webrtc:5261 Committed: https://crrev.com/1edb7ab7bd38318e039e99e0b1577de728e9a386 Cr-Commit-Position: refs/heads/master@{#12444} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/1edb7ab7bd38318e039e99e0b1577de728e9a386 Cr-Commit-Position: refs/heads/master@{#12444} |
