|
|
Created:
4 years, 9 months ago by philipel Modified:
4 years, 8 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. |
DescriptionPatch Set 1 #Patch Set 2 : #Patch Set 3 : Generalized the FrameObject class. #
Total comments: 26
Patch Set 4 : Feedback fixes. #
Total comments: 4
Patch Set 5 : Feedback fixes #
Total comments: 24
Patch Set 6 : Feedback fixes #
Total comments: 2
Patch Set 7 : Moved comment and ClearUpTo() now clears packets immediately. #
Total comments: 4
Patch Set 8 : Update cleared_to_seq_num_ when FrameObjects are returned #
Total comments: 6
Patch Set 9 : Last comment fixes #Patch Set 10 : Rebase #Patch Set 11 : Added frame_object.h/cc to BUILD.gn. #Patch Set 12 : More parenthesis for the buildbots! #
Messages
Total messages: 42 (15 generated)
The FrameObject class is currently rather minimal and will be extended as needed as I develop the FrameBuffer.
Put the FrameObject class in frame_object.h/cc instead of packet_buffer.h/cc. Again, the FrameObject class is rather minimal and will be expanded as I work on the FrameBuffer.
ping
A first set of comments. https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.cc (right): https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.cc:27: int index = first_packet_ % packet_buffer_->size_; I think this code would be better to have in PacketBuffer, and maybe instead call packet_buffer_->FreePackets(first_packet_, last_packet_) or something like that? Alternatively: packet_buffer_->ReturnFrame(this); https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.cc:49: return 1; Is this correct? https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.cc:53: rtc::CritScope lock(&packet_buffer_->crit_); Seems like most of this code also should live in packet_buffer_? packet_buffer_->GetBitstream(destination, first_packet_, last_packet_); https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.h (right): https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.h:31: uint16_t first_packet, git cl format https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.h:34: uint16_t FirstPacket() const; For simple getters like this, name it first_packet() https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:33: RTC_DCHECK_LE(start_buffer_size, max_buffer_size); Indentation, will probably be fixed with git cl format. https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:54: // The Packet Buffer is full, try to expand the buffer. PacketBuffer or "packet buffer" https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:118: while (IsContinous(seq_num)) { Do we always have to deliver continuous frames (based on sequence numbers) to the frame buffer? Doesn't that mean we can't skip incomplete frames in, e.g., TL1 as long as we have a complete TL0 sequence? https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:35: PacketBuffer(int start_buffer_size, size_t here and below https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:56: bool IsContinous(uint16_t seq_num) const EXCLUSIVE_LOCKS_REQUIRED(crit_); IsContinuous https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:64: uint16_t clear_up_to_ GUARDED_BY(crit_); Group all members guarded by the crit
https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.cc (right): https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.cc:27: int index = first_packet_ % packet_buffer_->size_; On 2016/03/17 12:06:31, stefan-webrtc (holmer) wrote: > I think this code would be better to have in PacketBuffer, and maybe instead > call packet_buffer_->FreePackets(first_packet_, last_packet_) or something like > that? > > Alternatively: > packet_buffer_->ReturnFrame(this); I would like to keep this function in the frame object since I want the packet buffer to only assemble frames. https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.cc:49: return 1; On 2016/03/17 12:06:31, stefan-webrtc (holmer) wrote: > Is this correct? Added picture id to the frame object, but the corresponding logic in the packet buffer is not yet implemented. https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.cc:53: rtc::CritScope lock(&packet_buffer_->crit_); On 2016/03/17 12:06:31, stefan-webrtc (holmer) wrote: > Seems like most of this code also should live in packet_buffer_? > > packet_buffer_->GetBitstream(destination, first_packet_, last_packet_); Again, I would like the functionality of the packet buffer to be as close as possible to only assemble frames. https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.h (right): https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.h:31: uint16_t first_packet, On 2016/03/17 12:06:31, stefan-webrtc (holmer) wrote: > git cl format Done. https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.h:34: uint16_t FirstPacket() const; On 2016/03/17 12:06:31, stefan-webrtc (holmer) wrote: > For simple getters like this, name it first_packet() Done. https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:33: RTC_DCHECK_LE(start_buffer_size, max_buffer_size); On 2016/03/17 12:06:31, stefan-webrtc (holmer) wrote: > Indentation, will probably be fixed with git cl format. Formated, lets hope for the best :) https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:54: // The Packet Buffer is full, try to expand the buffer. On 2016/03/17 12:06:31, stefan-webrtc (holmer) wrote: > PacketBuffer or "packet buffer" Done. https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:118: while (IsContinous(seq_num)) { On 2016/03/17 12:06:31, stefan-webrtc (holmer) wrote: > Do we always have to deliver continuous frames (based on sequence numbers) to > the frame buffer? Doesn't that mean we can't skip incomplete frames in, e.g., > TL1 as long as we have a complete TL0 sequence? This function only test the continuity between packets within a frame, so frames are still assembled out of order. In short, as soon as a frame is complete a frame object is created and the callback is called. This logic will have to be changed slightly in the case of not having a picture id available. https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:35: PacketBuffer(int start_buffer_size, On 2016/03/17 12:06:31, stefan-webrtc (holmer) wrote: > size_t here and below Done. https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:56: bool IsContinous(uint16_t seq_num) const EXCLUSIVE_LOCKS_REQUIRED(crit_); On 2016/03/17 12:06:31, stefan-webrtc (holmer) wrote: > IsContinuous Done. https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:64: uint16_t clear_up_to_ GUARDED_BY(crit_); On 2016/03/17 12:06:31, stefan-webrtc (holmer) wrote: > Group all members guarded by the crit Done.
ping...
https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.cc (right): https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.cc:27: int index = first_packet_ % packet_buffer_->size_; On 2016/03/17 15:00:35, philipel wrote: > On 2016/03/17 12:06:31, stefan-webrtc (holmer) wrote: > > I think this code would be better to have in PacketBuffer, and maybe instead > > call packet_buffer_->FreePackets(first_packet_, last_packet_) or something > like > > that? > > > > Alternatively: > > packet_buffer_->ReturnFrame(this); > > I would like to keep this function in the frame object since I want the packet > buffer to only assemble frames. I understand, but I think it's pretty ugly to expose a lot of protected members of the PacketBuffer, especially the lock since it can be difficult to determine if a deadlock can occur if the lock can be taken from outside the class itself... RtpFrameObject has none of the members needed for this and GetBitstream, while PacketBuffer has all that is needed (if you pass things in on ReturnFrame/GetBitstream, as suggested). I also think it would be a lot easier to write a test for RtpFrameObject if it doesn't rely on members of PacketBuffer as in that case you can pass in a MockPacketBuffer instead. https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.cc:49: return 1; On 2016/03/17 15:00:35, philipel wrote: > On 2016/03/17 12:06:31, stefan-webrtc (holmer) wrote: > > Is this correct? > > Added picture id to the frame object, but the corresponding logic in the packet > buffer is not yet implemented. Acknowledged. https://codereview.webrtc.org/1772383002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.h (right): https://codereview.webrtc.org/1772383002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.h:40: uint16_t picture_id() const override; I don't think you have to override this method if the base class owns the picture_id_? https://codereview.webrtc.org/1772383002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer_unittest.cc (right): https://codereview.webrtc.org/1772383002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer_unittest.cc:49: Do we have any tests that verify that we handle cases like: We have size_= 256 and insert both seq_num = 65535 and seq_num = 255? I don't see that as unreasonable, and they may use the same slot if we're not handling this correctly.
https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.cc (right): https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.cc:27: int index = first_packet_ % packet_buffer_->size_; On 2016/03/24 09:41:21, stefan-webrtc (holmer) wrote: > On 2016/03/17 15:00:35, philipel wrote: > > On 2016/03/17 12:06:31, stefan-webrtc (holmer) wrote: > > > I think this code would be better to have in PacketBuffer, and maybe instead > > > call packet_buffer_->FreePackets(first_packet_, last_packet_) or something > > like > > > that? > > > > > > Alternatively: > > > packet_buffer_->ReturnFrame(this); > > > > I would like to keep this function in the frame object since I want the packet > > buffer to only assemble frames. > > I understand, but I think it's pretty ugly to expose a lot of protected members > of the PacketBuffer, especially the lock since it can be difficult to determine > if a deadlock can occur if the lock can be taken from outside the class > itself... > > RtpFrameObject has none of the members needed for this and GetBitstream, while > PacketBuffer has all that is needed (if you pass things in on > ReturnFrame/GetBitstream, as suggested). > > I also think it would be a lot easier to write a test for RtpFrameObject if it > doesn't rely on members of PacketBuffer as in that case you can pass in a > MockPacketBuffer instead. Done. https://codereview.webrtc.org/1772383002/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.cc:53: rtc::CritScope lock(&packet_buffer_->crit_); On 2016/03/17 15:00:35, philipel wrote: > On 2016/03/17 12:06:31, stefan-webrtc (holmer) wrote: > > Seems like most of this code also should live in packet_buffer_? > > > > packet_buffer_->GetBitstream(destination, first_packet_, last_packet_); > > Again, I would like the functionality of the packet buffer to be as close as > possible to only assemble frames. Done. https://codereview.webrtc.org/1772383002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.h (right): https://codereview.webrtc.org/1772383002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.h:40: uint16_t picture_id() const override; On 2016/03/24 09:41:21, stefan-webrtc (holmer) wrote: > I don't think you have to override this method if the base class owns the > picture_id_? Moved picture_id to RtpFrameObject. https://codereview.webrtc.org/1772383002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer_unittest.cc (right): https://codereview.webrtc.org/1772383002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer_unittest.cc:49: On 2016/03/24 09:41:21, stefan-webrtc (holmer) wrote: > Do we have any tests that verify that we handle cases like: > We have size_= 256 and insert both seq_num = 65535 and seq_num = 255? I don't > see that as unreasonable, and they may use the same slot if we're not handling > this correctly. The ExpandBuffer and ExpandBufferOverflow test this.
LG, just a few more comments to address. https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.h (right): https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.h:41: PacketBuffer* packet_buffer_; I think this can be const? https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:77: clear_up_to_ = seq_num; Why doesn't this simply clear the packet_buffer_ instead of storing it? https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:104: if (!sequence_buffer_[index].used) We should be able to DCHECK on this, right? We should never be calling IsContinuous on a seq num we haven't added. https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:113: return false; Can we rewrite this method so that we have a single "return true", I think it would be easier to read. For instance by changing line 110 and line 106 to check the opposite. https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:121: // If the frame is complete, find the first packet of the frame and Should this say "If the frame has the last packet, determine if it is complete and if so create a FrameObject."? https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:129: } Can we DCHECK on that we actually found the first packet of the same frame? https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:157: uint8_t* destination) { Is it up to the caller to ensure this buffer is large enough? How would that be done without a way of asking the PacketBuffer how large the frame is? https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:165: sequence_buffer_[index].seq_num != seq_num) {} since this if statement is > 2 lines. https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:35: PacketBuffer(size_t start_buffer_size, Comment on power of 2 requirements.
https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_object.h (right): https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_object.h:41: PacketBuffer* packet_buffer_; On 2016/03/30 11:59:31, stefan-webrtc (holmer) wrote: > I think this can be const? Can't be const. The ReturnFrame function is non const and is called when the FrameObject is destroyed. https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:77: clear_up_to_ = seq_num; On 2016/03/30 11:59:32, stefan-webrtc (holmer) wrote: > Why doesn't this simply clear the packet_buffer_ instead of storing it? Since both the |data_buffer_| and the |sequence_buffer_| is a vector (and not a map) it is not possible to remove (mark them as not used) the correct elements without having to loop over all the elements in both vectors. Instead we save how far packets has been cleared and then later on when a packet is inserted we check if the current packet in the buffer has indeed been removed and if so we overwrite it. https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:104: if (!sequence_buffer_[index].used) On 2016/03/30 11:59:31, stefan-webrtc (holmer) wrote: > We should be able to DCHECK on this, right? We should never be calling > IsContinuous on a seq num we haven't added. We are guaranteed to check sequence numbers that has not yet been inserted into the packet buffer. This is done in the loop in FindCompleteFrames(). https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:113: return false; On 2016/03/30 11:59:31, stefan-webrtc (holmer) wrote: > Can we rewrite this method so that we have a single "return true", I think it > would be easier to read. For instance by changing line 110 and line 106 to check > the opposite. Hmm... Negating the condition on line 106 and 110 does not mean we should return the opposite, it means we should perform more checks... https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:121: // If the frame is complete, find the first packet of the frame and On 2016/03/30 11:59:31, stefan-webrtc (holmer) wrote: > Should this say "If the frame has the last packet, determine if it is complete > and if so create a FrameObject."? No, if we have reached a packet that is marked with frame_end then it means that we have all packets required for this frame, so we only have to find where it begins and then create a frame object using those sequence numbers. https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:129: } On 2016/03/30 11:59:31, stefan-webrtc (holmer) wrote: > Can we DCHECK on that we actually found the first packet of the same frame? I guess, but then we have to save the capture timestamp of the frame as well, and only in order to perform that check at this point. https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:157: uint8_t* destination) { On 2016/03/30 11:59:31, stefan-webrtc (holmer) wrote: > Is it up to the caller to ensure this buffer is large enough? How would that be > done without a way of asking the PacketBuffer how large the frame is? It's up to the caller, I guess we could (shoud?) add a GetSize() method to the FrameObject class. https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:165: sequence_buffer_[index].seq_num != seq_num) On 2016/03/30 11:59:31, stefan-webrtc (holmer) wrote: > {} since this if statement is > 2 lines. Done. https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.h:35: PacketBuffer(size_t start_buffer_size, On 2016/03/30 11:59:32, stefan-webrtc (holmer) wrote: > Comment on power of 2 requirements. Done.
https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:77: clear_up_to_ = seq_num; On 2016/03/30 12:40:11, philipel wrote: > On 2016/03/30 11:59:32, stefan-webrtc (holmer) wrote: > > Why doesn't this simply clear the packet_buffer_ instead of storing it? > > Since both the |data_buffer_| and the |sequence_buffer_| is a vector (and not a > map) it is not possible to remove (mark them as not used) the correct elements > without having to loop over all the elements in both vectors. > > Instead we save how far packets has been cleared and then later on when a packet > is inserted we check if the current packet in the buffer has indeed been removed > and if so we overwrite it. Change as discussed offline https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:104: if (!sequence_buffer_[index].used) On 2016/03/30 12:40:11, philipel wrote: > On 2016/03/30 11:59:31, stefan-webrtc (holmer) wrote: > > We should be able to DCHECK on this, right? We should never be calling > > IsContinuous on a seq num we haven't added. > > We are guaranteed to check sequence numbers that has not yet been inserted into > the packet buffer. This is done in the loop in FindCompleteFrames(). Ah, yes, my mistake. https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:113: return false; On 2016/03/30 12:40:11, philipel wrote: > On 2016/03/30 11:59:31, stefan-webrtc (holmer) wrote: > > Can we rewrite this method so that we have a single "return true", I think it > > would be easier to read. For instance by changing line 110 and line 106 to > check > > the opposite. > > Hmm... Negating the condition on line 106 and 110 does not mean we should return > the opposite, it means we should perform more checks... That's right, maybe it gets more complicated. https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:121: // If the frame is complete, find the first packet of the frame and On 2016/03/30 12:40:11, philipel wrote: > On 2016/03/30 11:59:31, stefan-webrtc (holmer) wrote: > > Should this say "If the frame has the last packet, determine if it is complete > > and if so create a FrameObject."? > > No, if we have reached a packet that is marked with frame_end then it means that > we have all packets required for this frame, so we only have to find where it > begins and then create a frame object using those sequence numbers. Maybe change the comment to what you just wrote instead. :) https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:129: } On 2016/03/30 12:40:11, philipel wrote: > On 2016/03/30 11:59:31, stefan-webrtc (holmer) wrote: > > Can we DCHECK on that we actually found the first packet of the same frame? > > I guess, but then we have to save the capture timestamp of the frame as well, > and only in order to perform that check at this point. Can we add a check on picture id instead then? https://codereview.webrtc.org/1772383002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:157: uint8_t* destination) { On 2016/03/30 12:40:11, philipel wrote: > On 2016/03/30 11:59:31, stefan-webrtc (holmer) wrote: > > Is it up to the caller to ensure this buffer is large enough? How would that > be > > done without a way of asking the PacketBuffer how large the frame is? > > It's up to the caller, I guess we could (shoud?) add a GetSize() method to the > FrameObject class. I think that'd be good, otherwise I suspect it'd be difficult to use. https://codereview.webrtc.org/1772383002/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/1772383002/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/packet_buffer.cc:23: // Both |start_buffer_size| and |max_buffer_size| must be a power of 2. I think this is better suited in the header file so that you don't have to read the impl to use the class.
https://codereview.webrtc.org/1772383002/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/1772383002/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/packet_buffer.cc:23: // Both |start_buffer_size| and |max_buffer_size| must be a power of 2. On 2016/03/30 12:56:20, stefan-webrtc (holmer) wrote: > I think this is better suited in the header file so that you don't have to read > the impl to use the class. Done.
https://codereview.webrtc.org/1772383002/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/1772383002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/packet_buffer.cc:52: return false; Maybe we should treat this as a success and just suppress it? Duplicate packets aren't unexpected. https://codereview.webrtc.org/1772383002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/packet_buffer.cc:76: int index = clear_up_to_ % size_; Would lowest_seq_num_ or something like that be a better name? Something that indicates the "start" of the buffer rather than how far we have "cleared". The way this works now, if we call ClearUpTo() very rarely, we'll end up with a clear_up_to_ which is out of sync if the sequence numbers have wrapped and packets have been removed from the packet buffer via ReturnFrame. So I think we have to update the min seq num (or whatever we call it) whenever we remove packets.
https://codereview.webrtc.org/1772383002/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/1772383002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/packet_buffer.cc:52: return false; On 2016/03/30 13:47:35, stefan-webrtc (holmer) wrote: > Maybe we should treat this as a success and just suppress it? Duplicate packets > aren't unexpected. Done. https://codereview.webrtc.org/1772383002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/packet_buffer.cc:76: int index = clear_up_to_ % size_; On 2016/03/30 13:47:35, stefan-webrtc (holmer) wrote: > Would lowest_seq_num_ or something like that be a better name? Something that > indicates the "start" of the buffer rather than how far we have "cleared". The > way this works now, if we call ClearUpTo() very rarely, we'll end up with a > clear_up_to_ which is out of sync if the sequence numbers have wrapped and > packets have been removed from the packet buffer via ReturnFrame. So I think we > have to update the min seq num (or whatever we call it) whenever we remove > packets. Renamed |clear_up_to_| to |cleared_to_seq_num_|. Also updates it when frames are returned to the packet buffer.
lgtm, but consider my last comments before committing. https://codereview.webrtc.org/1772383002/diff/140001/webrtc/modules/video_cod... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/1772383002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/packet_buffer.cc:46: cleared_to_seq_num_ = seq_num - 1; I would still prefer to call this first_seq_num_, especially since it would match last_seq_num_, but I won't block the cl on it. https://codereview.webrtc.org/1772383002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/packet_buffer.cc:165: !sequence_buffer_[index].used) { git cl format https://codereview.webrtc.org/1772383002/diff/140001/webrtc/modules/video_cod... File webrtc/modules/video_coding/packet_buffer_unittest.cc (right): https://codereview.webrtc.org/1772383002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/packet_buffer_unittest.cc:303: Test the new behavior where we clear sequence numbers when we return a frame. I.e., verify that the problem you fixed can't happen.
https://codereview.webrtc.org/1772383002/diff/140001/webrtc/modules/video_cod... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/1772383002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/packet_buffer.cc:46: cleared_to_seq_num_ = seq_num - 1; On 2016/03/31 13:03:00, stefan-webrtc (holmer) wrote: > I would still prefer to call this first_seq_num_, especially since it would > match last_seq_num_, but I won't block the cl on it. Agree, changed. https://codereview.webrtc.org/1772383002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/packet_buffer.cc:165: !sequence_buffer_[index].used) { On 2016/03/31 13:03:00, stefan-webrtc (holmer) wrote: > git cl format Formated, but nothing changed. https://codereview.webrtc.org/1772383002/diff/140001/webrtc/modules/video_cod... File webrtc/modules/video_coding/packet_buffer_unittest.cc (right): https://codereview.webrtc.org/1772383002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/packet_buffer_unittest.cc:303: On 2016/03/31 13:03:01, stefan-webrtc (holmer) wrote: > Test the new behavior where we clear sequence numbers when we return a frame. > I.e., verify that the problem you fixed can't happen. That is tested in DiscardOldPacket and DiscardMultipleOldPackets.
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1772383002/#ps160001 (title: "Last comment fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772383002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772383002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_x86_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...) android_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_dbg/builds/9906) android_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_rel/builds/1...) ios32_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/6351) ios64_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/6339) ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/8677) ios_arm64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/8598) ios_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/13880) ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/12512) linux_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_compile_dbg/build...) linux_tsan2 on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/11026) linux_ubsan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/910) linux_ubsan_vptr on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...) mac_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/13527) mac_baremetal on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/10244) mac_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/1995) mac_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/8224) mac_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/13928)
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1772383002/#ps180001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772383002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772383002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/1996)
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1772383002/#ps200001 (title: "Added frame_object.h/cc to BUILD.gn.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772383002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772383002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_x64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...) android_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_rel/builds/1...)
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/1772383002/#ps220001 (title: "More parenthesis for the buildbots!")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772383002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772383002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by philipel@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772383002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772383002/220001
Message was sent while issue was closed.
Description was changed from ========== Packet buffer for the new jitter buffer. BUG=webrtc:5514 R=stefan@webrtc.org, mflodman@webrtc.org ========== to ========== Packet buffer for the new jitter buffer. BUG=webrtc:5514 R=stefan@webrtc.org, mflodman@webrtc.org Committed: https://crrev.com/c707ab7cb0d087b7db33a1b2d65a5a9ff631f3ad Cr-Commit-Position: refs/heads/master@{#12194} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/c707ab7cb0d087b7db33a1b2d65a5a9ff631f3ad Cr-Commit-Position: refs/heads/master@{#12194}
Message was sent while issue was closed.
Description was changed from ========== Packet buffer for the new jitter buffer. BUG=webrtc:5514 R=stefan@webrtc.org, mflodman@webrtc.org Committed: https://crrev.com/c707ab7cb0d087b7db33a1b2d65a5a9ff631f3ad Cr-Commit-Position: refs/heads/master@{#12194} ========== to ========== Packet buffer for the new jitter buffer. BUG=webrtc:5514 R=stefan@webrtc.org, mflodman@webrtc.org ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) |