Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2304723004:
Added ClearTo(seq_num) to RtpFrameReferenceFinder. (Closed)
DescriptionAdded ClearTo(seq_num) to RtpFrameReferenceFinder.
In order to be able to clear out any potentially stashed old frames from
the RtpFrameReferenceFinder we can now clear frames that contain packets
older than |seq_num|.
BUG=webrtc:5514
Committed: https://crrev.com/463d3011ceeb6d361411a42fa974b92d9dbc53e1
Cr-Commit-Position: refs/heads/master@{#14156}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Added comments. #
Total comments: 6
Patch Set 3 : Updated comment. #Patch Set 4 : std::queue --> std::deque for stashed_frames_" #
Total comments: 1
Patch Set 5 : Nit fix #
Messages
Total messages: 24 (10 generated)
philipel@webrtc.org changed reviewers: + brandtr@webrtc.org, stefan@webrtc.org
lgtm, with some nits. https://codereview.webrtc.org/2304723004/diff/1/webrtc/modules/video_coding/r... File webrtc/modules/video_coding/rtp_frame_reference_finder.h (right): https://codereview.webrtc.org/2304723004/diff/1/webrtc/modules/video_coding/r... webrtc/modules/video_coding/rtp_frame_reference_finder.h:42: explicit RtpFrameReferenceFinder(OnCompleteFrameCallback* frame_callback); Perhaps it would be good to comment the public interface of this class as well. (The private interface is nicely commented.) https://codereview.webrtc.org/2304723004/diff/1/webrtc/modules/video_coding/r... File webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc (right): https://codereview.webrtc.org/2304723004/diff/1/webrtc/modules/video_coding/r... webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc:286: InsertGeneric(sn + 8, sn + 9, false); Maybe add a comment here, explaining why the frame is sent through the callback, even though it's not a keyframe. https://codereview.webrtc.org/2304723004/diff/1/webrtc/modules/video_coding/r... webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc:289: InsertGeneric(sn + 2, sn + 3, false); // late, cleard past this frame. "cleared"
https://codereview.webrtc.org/2304723004/diff/1/webrtc/modules/video_coding/r... File webrtc/modules/video_coding/rtp_frame_reference_finder.h (right): https://codereview.webrtc.org/2304723004/diff/1/webrtc/modules/video_coding/r... webrtc/modules/video_coding/rtp_frame_reference_finder.h:42: explicit RtpFrameReferenceFinder(OnCompleteFrameCallback* frame_callback); On 2016/09/05 11:20:32, brandtr wrote: > Perhaps it would be good to comment the public interface of this class as well. > (The private interface is nicely commented.) Done. https://codereview.webrtc.org/2304723004/diff/1/webrtc/modules/video_coding/r... File webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc (right): https://codereview.webrtc.org/2304723004/diff/1/webrtc/modules/video_coding/r... webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc:286: InsertGeneric(sn + 8, sn + 9, false); On 2016/09/05 11:20:32, brandtr wrote: > Maybe add a comment here, explaining why the frame is sent through the callback, > even though it's not a keyframe. Done. https://codereview.webrtc.org/2304723004/diff/1/webrtc/modules/video_coding/r... webrtc/modules/video_coding/rtp_frame_reference_finder_unittest.cc:289: InsertGeneric(sn + 2, sn + 3, false); // late, cleard past this frame. On 2016/09/05 11:20:32, brandtr wrote: > "cleared" Done.
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/2304723004/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/rtp_frame_reference_finder.cc (right): https://codereview.webrtc.org/2304723004/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:77: for (size_t i = 0; i < num_stashed_frames; ++i) { Isn't it simpler to write this: while (AheadOf<uint16_t>(cleared_to_seq_num_, stashed_frames_.front()->first_seq_num()) { stashed_frames_.pop(); } https://codereview.webrtc.org/2304723004/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/rtp_frame_reference_finder.h (right): https://codereview.webrtc.org/2304723004/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.h:44: // Find all references for this frame. Maybe the method should be called that in stead of ManageFrame?
https://codereview.webrtc.org/2304723004/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/rtp_frame_reference_finder.cc (right): https://codereview.webrtc.org/2304723004/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:77: for (size_t i = 0; i < num_stashed_frames; ++i) { On 2016/09/07 13:29:57, stefan-webrtc (holmer) wrote: > Isn't it simpler to write this: > > while (AheadOf<uint16_t>(cleared_to_seq_num_, > stashed_frames_.front()->first_seq_num()) { > stashed_frames_.pop(); > } Frames might be inserted out of order, so we don't know if the oldest frames are first in the queue, and even if they were inserted in order I'm not 100% sure that order would be maintained. https://codereview.webrtc.org/2304723004/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/rtp_frame_reference_finder.h (right): https://codereview.webrtc.org/2304723004/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.h:44: // Find all references for this frame. On 2016/09/07 13:29:57, stefan-webrtc (holmer) wrote: > Maybe the method should be called that in stead of ManageFrame? Updated the comment the better reflect what ManageFrame actually does.
https://codereview.webrtc.org/2304723004/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/rtp_frame_reference_finder.cc (right): https://codereview.webrtc.org/2304723004/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:77: for (size_t i = 0; i < num_stashed_frames; ++i) { On 2016/09/08 11:27:19, philipel wrote: > On 2016/09/07 13:29:57, stefan-webrtc (holmer) wrote: > > Isn't it simpler to write this: > > > > while (AheadOf<uint16_t>(cleared_to_seq_num_, > > stashed_frames_.front()->first_seq_num()) { > > stashed_frames_.pop(); > > } > > Frames might be inserted out of order, so we don't know if the oldest frames are > first in the queue, and even if they were inserted in order I'm not 100% sure > that order would be maintained. Ok, but what about: for (it = stashed_frames_.begin(); ... ; ++it) { if (AheadOf<uint16_t>(cleared_to_seq_num_, it->first_seq_num())) stashed_frames_.erase(it++); } My main point is that it seems unnecessarily complicated to first pop the frame and then insert it back if it wasn't too old. I guess that'd mean you have to a list instead, but maybe that makes sense?
Last patchset was suppose to say: "std::queue --> std::deque for stashed_frames_" https://codereview.webrtc.org/2304723004/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/rtp_frame_reference_finder.cc (right): https://codereview.webrtc.org/2304723004/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:77: for (size_t i = 0; i < num_stashed_frames; ++i) { On 2016/09/08 11:51:30, stefan-webrtc (holmer) wrote: > On 2016/09/08 11:27:19, philipel wrote: > > On 2016/09/07 13:29:57, stefan-webrtc (holmer) wrote: > > > Isn't it simpler to write this: > > > > > > while (AheadOf<uint16_t>(cleared_to_seq_num_, > > > stashed_frames_.front()->first_seq_num()) { > > > stashed_frames_.pop(); > > > } > > > > Frames might be inserted out of order, so we don't know if the oldest frames > are > > first in the queue, and even if they were inserted in order I'm not 100% sure > > that order would be maintained. > > Ok, but what about: > > for (it = stashed_frames_.begin(); ... ; ++it) { > if (AheadOf<uint16_t>(cleared_to_seq_num_, it->first_seq_num())) > stashed_frames_.erase(it++); > > } > > My main point is that it seems unnecessarily complicated to first pop the frame > and then insert it back if it wasn't too old. I guess that'd mean you have to a > list instead, but maybe that makes sense? Changed to deque
lgtm % nit https://codereview.webrtc.org/2304723004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/rtp_frame_reference_finder.cc (right): https://codereview.webrtc.org/2304723004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:78: if (AheadOf<uint16_t>(cleared_to_seq_num_, (*it)->first_seq_num())) {} since it's a multiline if.
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from brandtr@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2304723004/#ps80001 (title: "Nit fix")
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_compile_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...)
The CQ bit was checked by philipel@webrtc.org
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.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Added ClearTo(seq_num) to RtpFrameReferenceFinder. In order to be able to clear out any potentially stashed old frames from the RtpFrameReferenceFinder we can now clear frames that contain packets older than |seq_num|. BUG=webrtc:5514 ========== to ========== Added ClearTo(seq_num) to RtpFrameReferenceFinder. In order to be able to clear out any potentially stashed old frames from the RtpFrameReferenceFinder we can now clear frames that contain packets older than |seq_num|. BUG=webrtc:5514 Committed: https://crrev.com/463d3011ceeb6d361411a42fa974b92d9dbc53e1 Cr-Commit-Position: refs/heads/master@{#14156} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/463d3011ceeb6d361411a42fa974b92d9dbc53e1 Cr-Commit-Position: refs/heads/master@{#14156} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
