Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1439553003:
[rtp_rtcp] rtcp::Sdes cleaned and got Parse function (Closed)
|
Created:
5 years, 1 month ago by danilchap Modified:
4 years, 10 months ago Reviewers:
åsapersson CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, stefan-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdded accessor and Parse function.
Create function merged into one.
BUG=webrtc:5260
Committed: https://crrev.com/9f35d55c580eb0f50392a57d6ec9d14493661f13
Cr-Commit-Position: refs/heads/master@{#11581}
Patch Set 1 #
Total comments: 11
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : Style fixes, more tests. #Patch Set 5 : rebase & clean #Patch Set 6 : #Patch Set 7 : rebase #Patch Set 8 : rebase & reclean #Patch Set 9 : rebase #Patch Set 10 : rebase #
Total comments: 18
Patch Set 11 : #Patch Set 12 : skip chunk without cname instead of failing parse #Patch Set 13 : #
Messages
Total messages: 20 (8 generated)
danilchap@webrtc.org changed reviewers: + asapersson@webrtc.org, sprang@webrtc.org
https://codereview.webrtc.org/1439553003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc (right): https://codereview.webrtc.org/1439553003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc:54: // unchanged. This will only be a problem if we try to reuse an instance, right? Otherwise it might actually be interesting to get what data we actually managed to parse. https://codereview.webrtc.org/1439553003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc:58: const uint8_t* next_chunk = payload; Don't like the name next_chunk, since it doesn't necessarily point to the next chunk. current_data ? Dunno, something that makes it clearer it's pointing into the buffer where we're currently parsing. https://codereview.webrtc.org/1439553003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc:164: return length; Should we perhaps store this as a member instead, and update it in Parse() and WithCName() ? https://codereview.webrtc.org/1439553003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.h (right): https://codereview.webrtc.org/1439553003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.h:45: const Chunk& chunk(size_t index) const { return chunks_[index]; } Could we just return a const std::vector<Chunk>& instead?
https://codereview.webrtc.org/1439553003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc (right): https://codereview.webrtc.org/1439553003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc:54: // unchanged. On 2015/11/12 15:27:15, språng wrote: > This will only be a problem if we try to reuse an instance, right? > Otherwise it might actually be interesting to get what data we actually managed > to parse. If packet is partially parsed, it means it is damaged. I am not sure data that can't be trusted is interesting. Keeping object clean doesn't add any noticeable overhead. https://codereview.webrtc.org/1439553003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc:58: const uint8_t* next_chunk = payload; On 2015/11/12 15:27:15, språng wrote: > Don't like the name next_chunk, since it doesn't necessarily point to the next > chunk. > current_data ? Dunno, something that makes it clearer it's pointing into the > buffer where we're currently parsing. Agree, can't keep it stay at a chunk since it also run through chunk items. Renamed to 'looking_at' https://codereview.webrtc.org/1439553003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc:164: return length; On 2015/11/12 15:27:15, språng wrote: > Should we perhaps store this as a member instead, and update it in Parse() and > WithCName() ? Agree, calculation of the length is not trivial but is used several times during Create function (including implicitly in the HeaderLength) Can't see simpler way to speed this up a bit. https://codereview.webrtc.org/1439553003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.h (right): https://codereview.webrtc.org/1439553003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.h:45: const Chunk& chunk(size_t index) const { return chunks_[index]; } On 2015/11/12 15:27:15, språng wrote: > Could we just return a const std::vector<Chunk>& instead? I do not want to disclose to user that vector is used as internal storage (may be later, for perfomance reasons, it will be replaced with a plain array) In fact it may be better to go further: hide struct Chunk and instead of function 'chunk' have two functions: ssrc(size_t index) and cname(size_t index) If we want to have other fields than cname (name, email, etc.) passed and extracted from this packet, then function per field would give more flexability how to organize it internally.
https://codereview.webrtc.org/1439553003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc (right): https://codereview.webrtc.org/1439553003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc:54: // unchanged. On 2015/11/12 16:59:09, danilchap wrote: > On 2015/11/12 15:27:15, språng wrote: > > This will only be a problem if we try to reuse an instance, right? > > Otherwise it might actually be interesting to get what data we actually > managed > > to parse. > > If packet is partially parsed, it means it is damaged. I am not sure data that > can't be trusted is interesting. > Keeping object clean doesn't add any noticeable overhead. Fair enough. https://codereview.webrtc.org/1439553003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.h (right): https://codereview.webrtc.org/1439553003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.h:45: const Chunk& chunk(size_t index) const { return chunks_[index]; } On 2015/11/12 16:59:09, danilchap wrote: > On 2015/11/12 15:27:15, språng wrote: > > Could we just return a const std::vector<Chunk>& instead? > > I do not want to disclose to user that vector is used as internal storage > (may be later, for perfomance reasons, it will be replaced with a plain array) > > In fact it may be better to go further: hide struct Chunk and instead of > function 'chunk' have two functions: > ssrc(size_t index) and cname(size_t index) > > If we want to have other fields than cname (name, email, etc.) passed and > extracted from this packet, then function per field would give more flexability > how to organize it internally. I get the sentiment, but I think this is premature optimization. I'd rather have more easily read code, say being able to use a for-each loop or pass the reference around. Should any of these places prove to actually be noticeable performance wise, let's refactor at that point instead.
https://codereview.webrtc.org/1439553003/diff/1/webrtc/modules/rtp_rtcp/sourc... File webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.h (right): https://codereview.webrtc.org/1439553003/diff/1/webrtc/modules/rtp_rtcp/sourc... webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.h:45: const Chunk& chunk(size_t index) const { return chunks_[index]; } On 2015/11/16 09:50:34, språng wrote: > On 2015/11/12 16:59:09, danilchap wrote: > > On 2015/11/12 15:27:15, språng wrote: > > > Could we just return a const std::vector<Chunk>& instead? > > > > I do not want to disclose to user that vector is used as internal storage > > (may be later, for perfomance reasons, it will be replaced with a plain array) > > > > In fact it may be better to go further: hide struct Chunk and instead of > > function 'chunk' have two functions: > > ssrc(size_t index) and cname(size_t index) > > > > If we want to have other fields than cname (name, email, etc.) passed and > > extracted from this packet, then function per field would give more > flexability > > how to organize it internally. > > I get the sentiment, but I think this is premature optimization. > I'd rather have more easily read code, say being able to use a for-each loop > or pass the reference around. Should any of these places prove to actually be > noticeable performance wise, let's refactor at that point instead. Done.
danilchap@webrtc.org changed reviewers: - sprang@webrtc.org
Description was changed from ========== rtcp::Sdes moved into own file and got Parse function ========== to ========== rtcp::Sdes moved into own file and got Parse function BUG=webrtc:5260 ==========
Description was changed from ========== rtcp::Sdes moved into own file and got Parse function BUG=webrtc:5260 ========== to ========== Added accessor and Parse function. Create function merged into one. BUG=webrtc:5260 ==========
https://codereview.webrtc.org/1439553003/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc (right): https://codereview.webrtc.org/1439553003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc:88: const uint8_t kCnameItemType = 1; use same name here and in Create (kCnameId) https://codereview.webrtc.org/1439553003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc:96: uint8_t size = *(looking_at++); maybe call length as in figure https://codereview.webrtc.org/1439553003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc:97: if (looking_at + size + 1 > payload_end) { maybe looking_at + size >= payload_end https://codereview.webrtc.org/1439553003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc:113: } is a chuck with zero items valid? https://codereview.webrtc.org/1439553003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc:118: block_length += ChunkSize(chunks[i]); is block_length supposed to not include non-cname items? https://codereview.webrtc.org/1439553003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc:146: RTC_DCHECK(!chunks_.empty()); zero is valid right? maybe remove https://codereview.webrtc.org/1439553003/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes_unittest.cc (right): https://codereview.webrtc.org/1439553003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes_unittest.cc:47: const Sdes& parsed = sdes_parsed; // Ensure acessors are const. acessors->accessors https://codereview.webrtc.org/1439553003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes_unittest.cc:96: maybe add a test for zero chunks
https://codereview.webrtc.org/1439553003/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc (right): https://codereview.webrtc.org/1439553003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc:88: const uint8_t kCnameItemType = 1; On 2016/02/11 10:20:32, åsapersson wrote: > use same name here and in Create (kCnameId) Using now same name here, in Create and in unittests (kCnameTag) https://codereview.webrtc.org/1439553003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc:96: uint8_t size = *(looking_at++); On 2016/02/11 10:20:32, åsapersson wrote: > maybe call length as in figure Done. https://codereview.webrtc.org/1439553003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc:97: if (looking_at + size + 1 > payload_end) { On 2016/02/11 10:20:32, åsapersson wrote: > maybe looking_at + size >= payload_end +1 has a different meaning here. Explained with a constant. https://codereview.webrtc.org/1439553003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc:113: } On 2016/02/11 10:20:32, åsapersson wrote: > is a chuck with zero items valid? Chunk need to have a cname. chunk with zero items can't. https://codereview.webrtc.org/1439553003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc:118: block_length += ChunkSize(chunks[i]); On 2016/02/11 10:20:33, åsapersson wrote: > is block_length supposed to not include non-cname items? block_length is for packing Sdes back to byte array. Because non-cname items are not stored and will not be included in rebuild packet, their size implicitly ignored. If chunk would include another field that we would send, ChunkSize would count its size. https://codereview.webrtc.org/1439553003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc:146: RTC_DCHECK(!chunks_.empty()); On 2016/02/11 10:20:32, åsapersson wrote: > zero is valid right? maybe remove Done. https://codereview.webrtc.org/1439553003/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes_unittest.cc (right): https://codereview.webrtc.org/1439553003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes_unittest.cc:47: const Sdes& parsed = sdes_parsed; // Ensure acessors are const. On 2016/02/11 10:20:33, åsapersson wrote: > acessors->accessors Done. https://codereview.webrtc.org/1439553003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes_unittest.cc:96: On 2016/02/11 10:20:33, åsapersson wrote: > maybe add a test for zero chunks Done.
lgtm https://codereview.webrtc.org/1439553003/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc (right): https://codereview.webrtc.org/1439553003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc:118: block_length += ChunkSize(chunks[i]); On 2016/02/11 11:13:37, danilchap wrote: > On 2016/02/11 10:20:33, åsapersson wrote: > > is block_length supposed to not include non-cname items? > > block_length is for packing Sdes back to byte array. Because non-cname items are > not stored and will not be included in rebuild packet, their size implicitly > ignored. If chunk would include another field that we would send, ChunkSize > would count its size. Maybe add a comment.
https://codereview.webrtc.org/1439553003/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc (right): https://codereview.webrtc.org/1439553003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/sdes.cc:118: block_length += ChunkSize(chunks[i]); On 2016/02/11 12:28:45, åsapersson wrote: > On 2016/02/11 11:13:37, danilchap wrote: > > On 2016/02/11 10:20:33, åsapersson wrote: > > > is block_length supposed to not include non-cname items? > > > > block_length is for packing Sdes back to byte array. Because non-cname items > are > > not stored and will not be included in rebuild packet, their size implicitly > > ignored. If chunk would include another field that we would send, ChunkSize > > would count its size. > > Maybe add a comment. Done.
The CQ bit was checked by danilchap@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from asapersson@webrtc.org Link to the patchset: https://codereview.webrtc.org/1439553003/#ps240001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1439553003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1439553003/240001
Message was sent while issue was closed.
Description was changed from ========== Added accessor and Parse function. Create function merged into one. BUG=webrtc:5260 ========== to ========== Added accessor and Parse function. Create function merged into one. BUG=webrtc:5260 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Added accessor and Parse function. Create function merged into one. BUG=webrtc:5260 ========== to ========== Added accessor and Parse function. Create function merged into one. BUG=webrtc:5260 Committed: https://crrev.com/9f35d55c580eb0f50392a57d6ec9d14493661f13 Cr-Commit-Position: refs/heads/master@{#11581} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/9f35d55c580eb0f50392a57d6ec9d14493661f13 Cr-Commit-Position: refs/heads/master@{#11581} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
