Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2399373002:
Only advance |first_seq_num_| if packets are explicitly cleared from the PacketBuffer. (Closed)
DescriptionOnly advance |first_seq_num_| if packets are explicitly cleared from the PacketBuffer.
In this CL:
- Don't insert a packet if we have explicitly cleared past it.
- Added some logging to ExpandBufferSize.
- Renamed IsContinuous to PotentialNewFrame.
- Unittests updated/added for this new behavior.
- Refactored TestPacketBuffer unittests.
BUG=webrtc:5514
R=danilchap@webrtc.org, stefan@webrtc.org
Committed: https://chromium.googlesource.com/external/webrtc/+/aee3e0eb321c57b6fdbe5d763670a33a58acda2e
Patch Set 1 #
Total comments: 25
Patch Set 2 : Feedback fixes. #
Total comments: 7
Patch Set 3 : Feedback fixes. #
Total comments: 4
Patch Set 4 : Compile fix #Patch Set 5 : Compile fix 2 #
Total comments: 2
Messages
Total messages: 29 (14 generated)
philipel@webrtc.org changed reviewers: + danilchap@webrtc.org, stefan@webrtc.org
https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/p... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.cc:55: PacketBuffer::~PacketBuffer() {} call Clear? https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.cc:78: return true; what does return false mean for this function? if you insert duplicate packet, it would return true if packet wasn't handled and cleaned and 'false' otherwise? https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.cc:105: if (packet.sizeBytes) { if packet.sizeBytes == 0, you will not allocate buffer, but because used = true might try to delete it later. since in Clear() you do not reset dataPtr to nullptr, it might create an error https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.cc:115: void PacketBuffer::ClearTo(uint16_t seq_num) { when this function is called? juts tests for now? https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.cc:123: size_t index = first_seq_num_ % size_; may be move this statement inside while loop. no need for 2 increment statements then. https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.cc:185: sequence_buffer_[index].seq_num - 1) { add some tests to check seq_num = 0 wraps properly. https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/p... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.h:82: bool used = false; seq_buffer[index].used = true <=> data_buffer[index].dataPtr != nullptr? if that the case, may be remove used and check dataPtr directly. https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.h:131: // If the buffer is cleared to a specific packet. can you write what packet it cleared to, to first_seq_num_? https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/v... File webrtc/modules/video_coding/video_packet_buffer_unittest.cc (right): https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/v... webrtc/modules/video_coding/video_packet_buffer_unittest.cc:106: uint16_t seq_num = Rand(); add const: const uint16_t seq_num = Rand(); seq_num is used a lot in the test and will help to know it can't change. https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/v... webrtc/modules/video_coding/video_packet_buffer_unittest.cc:113: frames_from_callback_.erase(frames_from_callback_.find(seq_num + 2)); that is same as frames_from_callback_.erase(seq_num + 2)?
https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/p... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.cc:55: PacketBuffer::~PacketBuffer() {} On 2016/10/07 18:25:37, danilchap wrote: > call Clear? Done. https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.cc:78: return true; On 2016/10/07 18:25:37, danilchap wrote: > what does return false mean for this function? > if you insert duplicate packet, it would return true if packet wasn't handled > and cleaned and 'false' otherwise? If |packet| is in the PacketBuffer when this function returns, true, otherwise false. https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.cc:105: if (packet.sizeBytes) { On 2016/10/07 18:25:37, danilchap wrote: > if packet.sizeBytes == 0, you will not allocate buffer, but because used = true > might try to delete it later. > since in Clear() you do not reset dataPtr to nullptr, it might create an error I now set |packet.dataPtr| to nullptr in Clear(). https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.cc:115: void PacketBuffer::ClearTo(uint16_t seq_num) { On 2016/10/07 18:25:37, danilchap wrote: > when this function is called? juts tests for now? For now, just in tests, but it will be used for the new jitter buffer. https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.cc:123: size_t index = first_seq_num_ % size_; On 2016/10/07 18:25:37, danilchap wrote: > may be move this statement inside while loop. > no need for 2 increment statements then. Done. https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.cc:185: sequence_buffer_[index].seq_num - 1) { On 2016/10/07 18:25:37, danilchap wrote: > add some tests to check seq_num = 0 wraps properly. Done. https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/p... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.h:131: // If the buffer is cleared to a specific packet. On 2016/10/07 18:25:37, danilchap wrote: > can you write what packet it cleared to, to first_seq_num_? Done. https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/v... File webrtc/modules/video_coding/video_packet_buffer_unittest.cc (right): https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/v... webrtc/modules/video_coding/video_packet_buffer_unittest.cc:106: uint16_t seq_num = Rand(); On 2016/10/07 18:25:37, danilchap wrote: > add const: > const uint16_t seq_num = Rand(); > seq_num is used a lot in the test and will help to know it can't change. Done. https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/v... webrtc/modules/video_coding/video_packet_buffer_unittest.cc:113: frames_from_callback_.erase(frames_from_callback_.find(seq_num + 2)); On 2016/10/07 18:25:37, danilchap wrote: > that is same as > frames_from_callback_.erase(seq_num + 2)? Done.
https://codereview.webrtc.org/2399373002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/2399373002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:132: bool has_cleared_to_ GUARDED_BY(crit_); I have a hard time understanding this member. What specific packet has it cleared to?
https://codereview.webrtc.org/2399373002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/2399373002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:132: bool has_cleared_to_ GUARDED_BY(crit_); On 2016/10/18 17:57:23, stefan-webrtc (holmer) wrote: > I have a hard time understanding this member. What specific packet has it > cleared to? This variable is set to true when ClearTo() has been called. The reason I have this variable is to be able to know if I should drop a packet because it's old (we have cleared past it), or if I should accept it (due to reordering that might occur at the start of the stream). See line 68 in packet_buffer.cc.
https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/p... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.cc:78: return true; On 2016/10/18 12:22:19, philipel wrote: > On 2016/10/07 18:25:37, danilchap wrote: > > what does return false mean for this function? > > if you insert duplicate packet, it would return true if packet wasn't handled > > and cleaned and 'false' otherwise? > > If |packet| is in the PacketBuffer when this function returns, true, otherwise > false. Probably would help to put this comment near InsertPacket declaration in the .h https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/p... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.h:82: bool used = false; On 2016/10/07 18:25:37, danilchap wrote: > seq_buffer[index].used = true <=> data_buffer[index].dataPtr != nullptr? > if that the case, may be remove used and check dataPtr directly. please ack if you disagree https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.h:131: // If the buffer is cleared to a specific packet. On 2016/10/18 12:22:19, philipel wrote: > On 2016/10/07 18:25:37, danilchap wrote: > > can you write what packet it cleared to, to first_seq_num_? > > Done. Can't see the difference. https://codereview.webrtc.org/2399373002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/2399373002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:137: if (sequence_buffer_[i].used) needed? if you prefer to keep it, may be have all 3 clear statements under this if or replace it with if (!used) continue; https://codereview.webrtc.org/2399373002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:182: return true; may be add a comment why true is returned, specially here, since condition is non-trivial. https://codereview.webrtc.org/2399373002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/2399373002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:132: bool has_cleared_to_ GUARDED_BY(crit_); may be name it something like is_cleared_before_first_seq_num_ : instead of describing past action, describe current state. (is instead of has/was/had) incorporate seq_num it is cleared to into name. 'before' as a notion of '<' since 'to' might mean '<='
https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/p... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.cc:78: return true; On 2016/10/19 09:25:10, danilchap wrote: > On 2016/10/18 12:22:19, philipel wrote: > > On 2016/10/07 18:25:37, danilchap wrote: > > > what does return false mean for this function? > > > if you insert duplicate packet, it would return true if packet wasn't > handled > > > and cleaned and 'false' otherwise? > > > > If |packet| is in the PacketBuffer when this function returns, true, otherwise > > false. > > Probably would help to put this comment near InsertPacket declaration in the .h Done. https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/p... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.h:82: bool used = false; On 2016/10/19 09:25:10, danilchap wrote: > On 2016/10/07 18:25:37, danilchap wrote: > > seq_buffer[index].used = true <=> data_buffer[index].dataPtr != nullptr? > > if that the case, may be remove used and check dataPtr directly. > > please ack if you disagree seq_buffer[index].used = true <=> data_buffer[index].dataPtr != nullptr is true, but I'm not sure if I want to remove |used| from ContinuityInfo. If we remove it then I think that should be done in another CL. https://codereview.webrtc.org/2399373002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.h:131: // If the buffer is cleared to a specific packet. On 2016/10/19 09:25:10, danilchap wrote: > On 2016/10/18 12:22:19, philipel wrote: > > On 2016/10/07 18:25:37, danilchap wrote: > > > can you write what packet it cleared to, to first_seq_num_? > > > > Done. > > Can't see the difference. Double Done. https://codereview.webrtc.org/2399373002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/2399373002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:137: if (sequence_buffer_[i].used) On 2016/10/19 09:25:10, danilchap wrote: > needed? > if you prefer to keep it, may be have all 3 clear statements under this if or > replace it with > if (!used) continue; Removed if case. https://codereview.webrtc.org/2399373002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:182: return true; On 2016/10/19 09:25:10, danilchap wrote: > may be add a comment why true is returned, specially here, since condition is > non-trivial. Done.
lgtm https://codereview.webrtc.org/2399373002/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/2399373002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:183: // index is because we want to avoid an inifite loop in the case where may be rephrase it a bit: ... to avoid an infinite loop in case a single frame contains more packets than the current size of the packet buffer. https://codereview.webrtc.org/2399373002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:186: return true; do you actually mean if (frame_begin) { // The comment. return !prev_used || AheadOf(seq_num, prev_seq_num); } ?
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/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_drmemory_light on master.tryserver.webrtc (JOB_FAILED, no build URL)
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/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ping
https://codereview.webrtc.org/2399373002/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/2399373002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:183: // index is because we want to avoid an inifite loop in the case where On 2016/10/20 10:37:55, danilchap wrote: > may be rephrase it a bit: > ... to avoid an infinite loop in case a single frame contains more packets than > the current size of the packet buffer. Acknowledged. https://codereview.webrtc.org/2399373002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:186: return true; On 2016/10/20 10:37:55, danilchap wrote: > do you actually mean > if (frame_begin) { > // The comment. > return !prev_used || AheadOf(seq_num, prev_seq_num); > } > ? I guess, I'm not sure if it's cleaner though...
lgtm assuming the test exists. https://codereview.webrtc.org/2399373002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/2399373002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:186: return true; Do we have a test case for this?
https://codereview.webrtc.org/2399373002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/2399373002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:186: return true; On 2016/10/31 15:13:56, stefan-webrtc (holmer) wrote: > Do we have a test case for this? Yes, SingleFrameExpandsBuffer
The CQ bit was checked by philipel@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/2399373002/#ps80001 (title: "Compile fix 2")
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: android_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)
Description was changed from ========== Only advance |first_seq_num_| if packets are explicitly cleared from the PacketBuffer. In this CL: - Don't insert a packet if we have explicitly cleared past it. - Added some logging to ExpandBufferSize. - Renamed IsContinuous to PotentialNewFrame. - Unittests updated/added for this new behavior. - Refactored TestPacketBuffer unittests. BUG=webrtc:5514 ========== to ========== Only advance |first_seq_num_| if packets are explicitly cleared from the PacketBuffer. In this CL: - Don't insert a packet if we have explicitly cleared past it. - Added some logging to ExpandBufferSize. - Renamed IsContinuous to PotentialNewFrame. - Unittests updated/added for this new behavior. - Refactored TestPacketBuffer unittests. BUG=webrtc:5514 R=danilchap@webrtc.org, stefan@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/aee3e0eb321c57b6fdbe5d763... ==========
Message was sent while issue was closed.
Description was changed from ========== Only advance |first_seq_num_| if packets are explicitly cleared from the PacketBuffer. In this CL: - Don't insert a packet if we have explicitly cleared past it. - Added some logging to ExpandBufferSize. - Renamed IsContinuous to PotentialNewFrame. - Unittests updated/added for this new behavior. - Refactored TestPacketBuffer unittests. BUG=webrtc:5514 R=danilchap@webrtc.org, stefan@webrtc.org Committed: https://chromium.googlesource.com/external/webrtc/+/aee3e0eb321c57b6fdbe5d763... ========== to ========== Only advance |first_seq_num_| if packets are explicitly cleared from the PacketBuffer. In this CL: - Don't insert a packet if we have explicitly cleared past it. - Added some logging to ExpandBufferSize. - Renamed IsContinuous to PotentialNewFrame. - Unittests updated/added for this new behavior. - Refactored TestPacketBuffer unittests. BUG=webrtc:5514 R=danilchap@webrtc.org, stefan@webrtc.org Committed: https://crrev.com/aee3e0eb321c57b6fdbe5d763670a33a58acda2e Cr-Commit-Position: refs/heads/master@{#14871} ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as aee3e0eb321c57b6fdbe5d763670a33a58acda2e (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
