Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2051453002:
Padding is now used to check for continuity in the packet sequence. (Closed)
|
Created:
4 years, 6 months ago by philipel Modified:
4 years, 5 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhengzhonghou_agora.io, video-team_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. |
DescriptionPadding is now used to check for continuity in the packet sequence.
BUG=webrtc:5514
Committed: https://crrev.com/9b2ce6be093c24a4b8bca76841e757151701ddad
Cr-Commit-Position: refs/heads/master@{#13383}
Patch Set 1 #Patch Set 2 : Added comments and formating. #
Total comments: 6
Patch Set 3 : Use unique_ptr for dummy data. #
Total comments: 2
Patch Set 4 : Comment clarification #Patch Set 5 : Clariy clarification comment. #
Total comments: 19
Patch Set 6 : Feedback fixes. #
Total comments: 1
Patch Set 7 : Nit fix. #
Messages
Total messages: 25 (9 generated)
Description was changed from ========== Padding is now be used to check for continuity in the packet sequence. BUG=webrtc:5514 ========== to ========== Padding is now used to check for continuity in the packet sequence. BUG=webrtc:5514 ==========
philipel@webrtc.org changed reviewers: + holmer@chromium.org, terelius@webrtc.org
The CQ bit was checked by philipel@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2051453002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/15351)
https://codereview.webrtc.org/2051453002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/rtp_frame_reference_finder.h (right): https://codereview.webrtc.org/2051453002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.h:106: // first = last sequence number last sequence number of non-padding packets? https://codereview.webrtc.org/2051453002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.h:107: // second = last sequence number with padding included last sequence number including padding packets? https://codereview.webrtc.org/2051453002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.h:117: // Padding packets that have been received but that is not yet continuous with contiguous?
https://codereview.webrtc.org/2051453002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/rtp_frame_reference_finder.h (right): https://codereview.webrtc.org/2051453002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.h:106: // first = last sequence number On 2016/06/08 18:52:51, terelius wrote: > last sequence number of non-padding packets? Kind of, it is not the last sequence number of the last managed frame, it is the sequence number of the last packet in the last continuous frame. https://codereview.webrtc.org/2051453002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.h:107: // second = last sequence number with padding included On 2016/06/08 18:52:51, terelius wrote: > last sequence number including padding packets? And this is the last packet sequence number but advanced by continuous packets of padding. https://codereview.webrtc.org/2051453002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.h:117: // Padding packets that have been received but that is not yet continuous with On 2016/06/08 18:52:51, terelius wrote: > contiguous? I think continuous is the right word to use here.
philipel@webrtc.org changed reviewers: + stefan@webrtc.org - holmer@chromium.org
ping
https://codereview.webrtc.org/2051453002/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/rtp_frame_reference_finder.h (right): https://codereview.webrtc.org/2051453002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.h:105: // last sequence number with padding included. I'd still like to have this comment clarified.
https://codereview.webrtc.org/2051453002/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/rtp_frame_reference_finder.h (right): https://codereview.webrtc.org/2051453002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.h:105: // last sequence number with padding included. On 2016/07/04 12:03:35, terelius wrote: > I'd still like to have this comment clarified. "For every group of pictures, holds the packet sequence number and the the packet sequence number advanced by continuous packets of padding for the last completed frame withing that group." WDYT @terelius?
lgtm
https://codereview.webrtc.org/2051453002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/2051453002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:69: // If this is a padding packet, don't insert it. In practice this could also be an FEC packet which we just want to notify the packet buffer about. Maybe mention that too? https://codereview.webrtc.org/2051453002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer_unittest.cc (right): https://codereview.webrtc.org/2051453002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer_unittest.cc:33: dummy_data(new uint8_t[dummy_data_size]()) {} dummy_data_ and no () after new uint8_t[dummy_data_size], or is that useful for something? https://codereview.webrtc.org/2051453002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer_unittest.cc:248: const int dummy_data_size = 4; kDummydataSize https://codereview.webrtc.org/2051453002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/rtp_frame_reference_finder.cc (right): https://codereview.webrtc.org/2051453002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:71: gop_seq_num_it--; --gop_seq_num_it; https://codereview.webrtc.org/2051453002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:74: // padding" sequence number. I think you should describe what the "last picture id with padding" is a bit better, I find it hard to understand what it's supposed to track/represent. Is it simply the last picture id in a gop which has a padding packet? If so, maybe also mention why we need to keep track of that. https://codereview.webrtc.org/2051453002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:78: while (padding_seq_num_it != received_padding_.end() && What does this loop do? Add a comment. https://codereview.webrtc.org/2051453002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:119: if (frame->frame_type() == kVideoFrameKey) {} https://codereview.webrtc.org/2051453002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/rtp_frame_reference_finder.h (right): https://codereview.webrtc.org/2051453002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.h:117: // Padding packets that have been received but that is not yet continuous with "...but that are not..." https://codereview.webrtc.org/2051453002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.h:119: std::set<uint16_t, DescendingSeqNumComp<uint16_t>> received_padding_ Maybe call these stashed_padding_ to make it more clear that they are stored here until they can be inserted in their right position? Btw, is it necessary to keep stashed padding separate from stashed regular packets?
https://codereview.webrtc.org/2051453002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/2051453002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:69: // If this is a padding packet, don't insert it. On 2016/07/05 08:00:40, stefan-webrtc (holmer) wrote: > In practice this could also be an FEC packet which we just want to notify the > packet buffer about. Maybe mention that too? Done. https://codereview.webrtc.org/2051453002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer_unittest.cc (right): https://codereview.webrtc.org/2051453002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer_unittest.cc:33: dummy_data(new uint8_t[dummy_data_size]()) {} On 2016/07/05 08:00:40, stefan-webrtc (holmer) wrote: > dummy_data_ Done. > and no () after new uint8_t[dummy_data_size], or is that useful for something? new uint8_t[dummy_data_size] => allocated but not initialized. new uint8_t[dummy_data_size]() => allocated and initialized. I need to initialize the memory so that the trybots wont complain about uninitialized memory. https://codereview.webrtc.org/2051453002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer_unittest.cc:248: const int dummy_data_size = 4; On 2016/07/05 08:00:40, stefan-webrtc (holmer) wrote: > kDummydataSize Done. https://codereview.webrtc.org/2051453002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/rtp_frame_reference_finder.cc (right): https://codereview.webrtc.org/2051453002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:71: gop_seq_num_it--; On 2016/07/05 08:00:40, stefan-webrtc (holmer) wrote: > --gop_seq_num_it; Done. https://codereview.webrtc.org/2051453002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:74: // padding" sequence number. On 2016/07/05 08:00:40, stefan-webrtc (holmer) wrote: > I think you should describe what the "last picture id with padding" is a bit > better, I find it hard to understand what it's supposed to track/represent. > > Is it simply the last picture id in a gop which has a padding packet? If so, > maybe also mention why we need to keep track of that. Added more comments to clarify. https://codereview.webrtc.org/2051453002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:78: while (padding_seq_num_it != received_padding_.end() && On 2016/07/05 08:00:40, stefan-webrtc (holmer) wrote: > What does this loop do? Add a comment. Done. https://codereview.webrtc.org/2051453002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:119: if (frame->frame_type() == kVideoFrameKey) On 2016/07/05 08:00:40, stefan-webrtc (holmer) wrote: > {} Done. https://codereview.webrtc.org/2051453002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/rtp_frame_reference_finder.h (right): https://codereview.webrtc.org/2051453002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.h:117: // Padding packets that have been received but that is not yet continuous with On 2016/07/05 08:00:40, stefan-webrtc (holmer) wrote: > "...but that are not..." Done. https://codereview.webrtc.org/2051453002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.h:119: std::set<uint16_t, DescendingSeqNumComp<uint16_t>> received_padding_ On 2016/07/05 08:00:40, stefan-webrtc (holmer) wrote: > Maybe call these stashed_padding_ to make it more clear that they are stored > here until they can be inserted in their right position? Done. > Btw, is it necessary to > keep stashed padding separate from stashed regular packets? This is the only place we stash packets in the RtpFrameReferenceFinder class.
lgtm with nit fixed https://codereview.webrtc.org/2051453002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/rtp_frame_reference_finder.cc (right): https://codereview.webrtc.org/2051453002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:74: // padding" sequence number. On 2016/07/05 08:41:00, philipel wrote: > On 2016/07/05 08:00:40, stefan-webrtc (holmer) wrote: > > I think you should describe what the "last picture id with padding" is a bit > > better, I find it hard to understand what it's supposed to track/represent. > > > > Is it simply the last picture id in a gop which has a padding packet? If so, > > maybe also mention why we need to keep track of that. > > Added more comments to clarify. Thanks, a lot better! https://codereview.webrtc.org/2051453002/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/rtp_frame_reference_finder.cc (right): https://codereview.webrtc.org/2051453002/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:73: // Calculate the next contiuous sequence number and seach for it in search
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from terelius@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2051453002/#ps120001 (title: "Nit fix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Padding is now used to check for continuity in the packet sequence. BUG=webrtc:5514 ========== to ========== Padding is now used to check for continuity in the packet sequence. BUG=webrtc:5514 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Padding is now used to check for continuity in the packet sequence. BUG=webrtc:5514 ========== to ========== Padding is now used to check for continuity in the packet sequence. BUG=webrtc:5514 Committed: https://crrev.com/9b2ce6be093c24a4b8bca76841e757151701ddad Cr-Commit-Position: refs/heads/master@{#13383} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/9b2ce6be093c24a4b8bca76841e757151701ddad Cr-Commit-Position: refs/heads/master@{#13383} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
