Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2616343003:
Refactor TransportFeedback ensuring it's consistency: (Closed)
|
Created:
3 years, 11 months ago by danilchap Modified:
3 years, 11 months ago Reviewers:
sprang_webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhuangzesen_agora.io, danilchap, stefan-webrtc, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRefactor TransportFeedback ensuring it's consistency:
Removed const_cast while creating rtcp packet.
This way manually created packet is as good as parsed packet and can be used in tests directly.
To archive this, changed the way class stores deltas and their sizes:
encoded chunks are stored directly for all but last chunk simplifying rtcp packet creation.
deltas stored together with sequence_number that would allow to simplify reading them from the parsed packet.
Fixed test for maximum received packets.
BUG=None
Review-Url: https://codereview.webrtc.org/2616343003
Cr-Commit-Position: refs/heads/master@{#16091}
Committed: https://chromium.googlesource.com/external/webrtc/+/6deecb2a2f66312dfaf49e57e33c60c5e36cb7e4
Patch Set 1 : . #
Total comments: 1
Patch Set 2 : Add explicit modulo operation for time in us #Patch Set 3 : 32bit oops #Patch Set 4 : -unused headers #
Total comments: 22
Patch Set 5 : Comments #Patch Set 6 : Fix time calculations on 32bit systems #Patch Set 7 : nits #
Messages
Total messages: 45 (39 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Patchset #1 (id:80001) has been deleted
Description was changed from ========== Refactor TransportFeedback: Removed const_cast while creating rtcp packet by making state consistence Extended interface with GetReceivedPackets to better mirror set interface Fixed test for maximum packet size. BUG=None ========== to ========== Refactor TransportFeedback implementation: Removed const_cast while creating rtcp packet. Ensure consistent state. This way manually created packet is as good as parsed packet and can be used in tests directly. Fixed test for maximum packet size. BUG=None ==========
Patchset #1 (id:100001) has been deleted
danilchap@webrtc.org changed reviewers: + sprang@webrtc.org
is it reviewable, or should I try splitting refactoring of the implementation into several steps? https://codereview.webrtc.org/2616343003/diff/120001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback_unittest.cc (left): https://codereview.webrtc.org/2616343003/diff/120001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback_unittest.cc:374: EXPECT_TRUE(packet->AddReceivedPacket(0xFFFF, 2000)); ([0, 0xffff] has size 0x10000 which exceed maximum size.)
The CQ bit was checked by danilchap@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/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...) win_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/22032)
The CQ bit was checked by danilchap@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/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_arm on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_arm/builds/4624)
The CQ bit was checked by danilchap@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/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/21634)
Neat! Maybe update the cl description to indicate that we now store the encoded representation of all encoded chunks directly, rather than having an intermediate representation. https://codereview.webrtc.org/2616343003/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc (right): https://codereview.webrtc.org/2616343003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc:179: for (size_t i = 0; i < size_ - kMaxTwoBitCapacity; ++i) { Comment on this loop and its upper bound https://codereview.webrtc.org/2616343003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc:204: deltas->insert(deltas->end(), delta_sizes_, delta_sizes_ + size_); nit: {} with if/else https://codereview.webrtc.org/2616343003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc:213: DecodeTwoBit(chunk, max_size); nit: dito https://codereview.webrtc.org/2616343003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc:232: chunk |= delta_sizes_[i] << (13 - i); Use kMaxOneBitCapacity - 1 instead of 13? https://codereview.webrtc.org/2616343003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc:300: delta_sizes_[i] = delta_size; Maybe comment that we fill |delta_size| in the same way, up to kMaxVetorCapacity, just as if this had been on the send side. Ie we want IsConsistent() to pass. Technically I guess we could do without it otherwise? https://codereview.webrtc.org/2616343003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc:395: symbols.push_back(StatusSymbol::kReceivedLargeDelta); nit: {} with if/else https://codereview.webrtc.org/2616343003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc:590: bool TransportFeedback::IsConsistent() const { This method looks expensive to call so often, even in debug mode. Is it enough to call it only in tests? https://codereview.webrtc.org/2616343003/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h (right): https://codereview.webrtc.org/2616343003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h:71: using DeltaSize = uint8_t; Add comment static that this represents the byte size of the encoded time delta, so it's not confused with the size of the delta itself. https://codereview.webrtc.org/2616343003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h:72: class LastChunk; I'm not sure about this name. Could you update it or add a comment which makes it clear that this is a buffer where the last symbols are stored until the suitable chunk type to encode to is determined. https://codereview.webrtc.org/2616343003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h:94: std::vector<uint16_t> chunks_; Can you comment or possibly typedef this so it's clear that this is the encoded 2 byte status chunkgs?
The CQ bit was checked by danilchap@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/...
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/20238)
The CQ bit was checked by danilchap@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/...
Description was changed from ========== Refactor TransportFeedback implementation: Removed const_cast while creating rtcp packet. Ensure consistent state. This way manually created packet is as good as parsed packet and can be used in tests directly. Fixed test for maximum packet size. BUG=None ========== to ========== Refactor TransportFeedback implementation: Removed const_cast while creating rtcp packet. Ensure consistent state. This way manually created packet is as good as parsed packet and can be used in tests directly. Changed the way class stores deltas and their sizes: encoded chunks are stored directly for all but last chunk simplifying rtcp packet creation. deltas stored together with sequence_number that would allow to create simpler accessor while reading them from the parsed packet. Fixed test for maximum packet size. BUG=None ==========
Description was changed from ========== Refactor TransportFeedback implementation: Removed const_cast while creating rtcp packet. Ensure consistent state. This way manually created packet is as good as parsed packet and can be used in tests directly. Changed the way class stores deltas and their sizes: encoded chunks are stored directly for all but last chunk simplifying rtcp packet creation. deltas stored together with sequence_number that would allow to create simpler accessor while reading them from the parsed packet. Fixed test for maximum packet size. BUG=None ========== to ========== Refactor TransportFeedback ensuring it's consistency: Removed const_cast while creating rtcp packet. This way manually created packet is as good as parsed packet and can be used in tests directly. To archive this, changed the way class stores deltas and their sizes: encoded chunks are stored directly for all but last chunk simplifying rtcp packet creation. deltas stored together with sequence_number that would allow to simplify reading them from the parsed packet. Fixed test for maximum packet size. BUG=None ==========
Patchset #6 (id:220001) has been deleted
Patchset #6 (id:240001) has been deleted
Description was changed from ========== Refactor TransportFeedback ensuring it's consistency: Removed const_cast while creating rtcp packet. This way manually created packet is as good as parsed packet and can be used in tests directly. To archive this, changed the way class stores deltas and their sizes: encoded chunks are stored directly for all but last chunk simplifying rtcp packet creation. deltas stored together with sequence_number that would allow to simplify reading them from the parsed packet. Fixed test for maximum packet size. BUG=None ========== to ========== Refactor TransportFeedback ensuring it's consistency: Removed const_cast while creating rtcp packet. This way manually created packet is as good as parsed packet and can be used in tests directly. To archive this, changed the way class stores deltas and their sizes: encoded chunks are stored directly for all but last chunk simplifying rtcp packet creation. deltas stored together with sequence_number that would allow to simplify reading them from the parsed packet. Fixed test for maximum received packets. BUG=None ==========
The CQ bit was checked by danilchap@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/...
Patchset #6 (id:260001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2616343003/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc (right): https://codereview.webrtc.org/2616343003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc:179: for (size_t i = 0; i < size_ - kMaxTwoBitCapacity; ++i) { On 2017/01/12 13:18:46, språng wrote: > Comment on this loop and its upper bound comment expanded, moved size_-= kMaxTwoBitCapacity; before loop making boundary more natural. https://codereview.webrtc.org/2616343003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc:204: deltas->insert(deltas->end(), delta_sizes_, delta_sizes_ + size_); On 2017/01/12 13:18:46, språng wrote: > nit: {} with if/else Done. https://codereview.webrtc.org/2616343003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc:213: DecodeTwoBit(chunk, max_size); On 2017/01/12 13:18:46, språng wrote: > nit: dito Done. https://codereview.webrtc.org/2616343003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc:232: chunk |= delta_sizes_[i] << (13 - i); On 2017/01/12 13:18:46, språng wrote: > Use kMaxOneBitCapacity - 1 instead of 13? Done. using this constant make it look similar to TwoBit encode/decode too. https://codereview.webrtc.org/2616343003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc:300: delta_sizes_[i] = delta_size; On 2017/01/12 13:18:46, språng wrote: > Maybe comment that we fill |delta_size| in the same way, up to > kMaxVetorCapacity, just as if this had been on the send side. Ie we want > IsConsistent() to pass. > Technically I guess we could do without it otherwise? Done. It would work without it as long as we do not extend TransportFeedback packet after Parse. Would rather waste few cycles than document this exception. https://codereview.webrtc.org/2616343003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc:395: symbols.push_back(StatusSymbol::kReceivedLargeDelta); On 2017/01/12 13:18:46, språng wrote: > nit: {} with if/else Done. https://codereview.webrtc.org/2616343003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.cc:590: bool TransportFeedback::IsConsistent() const { On 2017/01/12 13:18:46, språng wrote: > This method looks expensive to call so often, even in debug mode. Is it enough > to call it only in tests? Made public and moved all calls to tests https://codereview.webrtc.org/2616343003/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h (right): https://codereview.webrtc.org/2616343003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h:71: using DeltaSize = uint8_t; On 2017/01/12 13:18:46, språng wrote: > Add comment static that this represents the byte size of the encoded time delta, > so it's not confused with the size of the delta itself. Done. https://codereview.webrtc.org/2616343003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h:72: class LastChunk; On 2017/01/12 13:18:46, språng wrote: > I'm not sure about this name. Could you update it or add a comment which makes > it clear that this is a buffer where the last symbols are stored until the > suitable chunk type to encode to is determined. copied comment from definition of the class. Not sure about better name: it is used to keep DeltaSizes for the last chunk in case no more ReceivedPackets will be reported. https://codereview.webrtc.org/2616343003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h:94: std::vector<uint16_t> chunks_; On 2017/01/12 13:18:46, språng wrote: > Can you comment or possibly typedef this so it's clear that this is the encoded > 2 byte status chunkgs? is rename enough? Added also a comment, is helpful or redundant?
lgtm https://codereview.webrtc.org/2616343003/diff/180001/webrtc/modules/rtp_rtcp/... File webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h (right): https://codereview.webrtc.org/2616343003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h:72: class LastChunk; On 2017/01/16 09:45:15, danilchap wrote: > On 2017/01/12 13:18:46, språng wrote: > > I'm not sure about this name. Could you update it or add a comment which makes > > it clear that this is a buffer where the last symbols are stored until the > > suitable chunk type to encode to is determined. > > copied comment from definition of the class. > Not sure about better name: it is used to keep DeltaSizes for the last chunk in > case no more ReceivedPackets will be reported. Let's keep the name, the comment is good. https://codereview.webrtc.org/2616343003/diff/180001/webrtc/modules/rtp_rtcp/... webrtc/modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h:94: std::vector<uint16_t> chunks_; On 2017/01/16 09:45:15, danilchap wrote: > On 2017/01/12 13:18:46, språng wrote: > > Can you comment or possibly typedef this so it's clear that this is the > encoded > > 2 byte status chunkgs? > > is rename enough? > Added also a comment, is helpful or redundant? This is fine with me, thanks!
The CQ bit was checked by danilchap@webrtc.org
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": 300001, "attempt_start_ts": 1484565261765280,
"parent_rev": "1f8239ca6f887ecb8c2c8347c00b5ac681e83c60", "commit_rev":
"6deecb2a2f66312dfaf49e57e33c60c5e36cb7e4"}
Message was sent while issue was closed.
Description was changed from ========== Refactor TransportFeedback ensuring it's consistency: Removed const_cast while creating rtcp packet. This way manually created packet is as good as parsed packet and can be used in tests directly. To archive this, changed the way class stores deltas and their sizes: encoded chunks are stored directly for all but last chunk simplifying rtcp packet creation. deltas stored together with sequence_number that would allow to simplify reading them from the parsed packet. Fixed test for maximum received packets. BUG=None ========== to ========== Refactor TransportFeedback ensuring it's consistency: Removed const_cast while creating rtcp packet. This way manually created packet is as good as parsed packet and can be used in tests directly. To archive this, changed the way class stores deltas and their sizes: encoded chunks are stored directly for all but last chunk simplifying rtcp packet creation. deltas stored together with sequence_number that would allow to simplify reading them from the parsed packet. Fixed test for maximum received packets. BUG=None Review-Url: https://codereview.webrtc.org/2616343003 Cr-Commit-Position: refs/heads/master@{#16091} Committed: https://chromium.googlesource.com/external/webrtc/+/6deecb2a2f66312dfaf49e57e... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:300001) as https://chromium.googlesource.com/external/webrtc/+/6deecb2a2f66312dfaf49e57e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
