Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2480293002:
New jitter buffer experiment. (Closed)
DescriptionNew jitter buffer experiment.
BUG=webrtc:5514
Committed: https://crrev.com/fd5a20fd683c826bb26b3a03abb068c1ce084a7b
Cr-Commit-Position: refs/heads/master@{#15077}
Patch Set 1 #Patch Set 2 #Patch Set 3 #
Total comments: 48
Patch Set 4 : Feedback Fixes. #
Total comments: 2
Patch Set 5 : VP9 pid/tl0 jump fix + PacketBuffer CritScope fix. #Patch Set 6 : jitter_buffer_experiment_ made const. #
Total comments: 20
Patch Set 7 : Vp9PidTl0Fix unittests + clean up. #
Total comments: 1
Patch Set 8 : Nit fix. #
Total comments: 2
Messages
Total messages: 34 (13 generated)
Patchset #2 (id:20001) has been deleted
philipel@webrtc.org changed reviewers: + danilchap@webrtc.org, stefan@webrtc.org
https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2_unittest.cc (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2_unittest.cc:302: InsertFrame(pid + 1, 0, ts + kFps20, false, pid); Just an error I found as I was working on the new jitter buffer. https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/rtp_frame_reference_finder.h (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.h:62: static const int kMaxLayerInfo = 50; In general we threw away info to early causing loss on a medium/high rtt link to result in a keyframe request (since we couldn't continue the stream even if we got the lost packet). Fixed by simply storing more history. https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/rtp_stream_r... File webrtc/video/rtp_stream_receiver.cc (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/rtp_stream_r... webrtc/video/rtp_stream_receiver.cc:417: void RtpStreamReceiver::OnReceivedFrame( When the PacketBuffer finds a complete frame this function is called. https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/rtp_stream_r... webrtc/video/rtp_stream_receiver.cc:422: void RtpStreamReceiver::OnCompleteFrame( When the RtpFrameReferenceFinder has found all the references for a frame this function is called. The |complete_frame_callback_->OnCompleteFrame| is in this case VideoReceiveStream::OnCompleteFrame. https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/rtp_stream_r... webrtc/video/rtp_stream_receiver.cc:562: void RtpStreamReceiver::FrameContinuous(uint16_t picture_id) { This function is called from VideoReceiveStream::OnCompleteFrame after the frame has been inserted into the FrameBuffer. https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/rtp_stream_r... webrtc/video/rtp_stream_receiver.cc:576: void RtpStreamReceiver::FrameDecoded(uint16_t picture_id) { Called from VideoReceiveStream::Decode when a frame has been successfully decoded.
stefan@webrtc.org changed reviewers: + nisse@webrtc.org
+nisse who may have some useful comments on this. https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:261: tl0_pic_idx_ = static_cast<uint8_t>(rand()); // NOLINT Not clear to me why this change is done, and why it's in this CL. Please comment. https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:224: crit_.Enter(); I think we should rewrite the code so that we can properly use scoped crits. For instance you can return a list of frames to be sent out from this method and call the callback from InsertPacket() after you have left the crit. https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/rtp_frame_reference_finder.h (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/rtp_frame_reference_finder.h:62: static const int kMaxLayerInfo = 50; On 2016/11/08 09:27:13, philipel wrote: > In general we threw away info to early causing loss on a medium/high rtt link to > result in a keyframe request (since we couldn't continue the stream even if we > got the lost packet). Fixed by simply storing more history. Acknowledged. https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/video_receiver.cc (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/video_receiver.cc:293: // NOTE! Ugly hack to give the video receiver a frame to decode. Add TODO to remove. When can it be cleaned up? And why is it needed? It's not clear to me what the hack is about. https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/rtp_stream_r... File webrtc/video/rtp_stream_receiver.cc (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/rtp_stream_r... webrtc/video/rtp_stream_receiver.cc:417: void RtpStreamReceiver::OnReceivedFrame( On 2016/11/08 09:27:13, philipel wrote: > When the PacketBuffer finds a complete frame this function is called. Can't we have RtpFrameReferenceFinder implement OnReceivedFrame instead and bypass RtpStreamReceiver? https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/rtp_stream_r... File webrtc/video/rtp_stream_receiver.h (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/rtp_stream_r... webrtc/video/rtp_stream_receiver.h:30: #include "webrtc/modules/video_coding/h264_sps_pps_tracker.h" Sort https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/rtp_stream_r... webrtc/video/rtp_stream_receiver.h:52: class NackModule; Sort https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/rtp_stream_r... webrtc/video/rtp_stream_receiver.h:187: bool jitter_buffer_experiment_; This would be good to make const. https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/video_receiv... File webrtc/video/video_receive_stream.cc (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/video_receiv... webrtc/video/video_receive_stream.cc:35: #include "webrtc/modules/video_coding/frame_object.h" Sort https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/video_receiv... webrtc/video/video_receive_stream.cc:221: this, // OnCompleteFrameCallback Should we consolidate these interfaces so that we only have to register 'this' once? Or do they make sense to have separate? https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/video_receiv... webrtc/video/video_receive_stream.cc:296: frame_buffer_->SetProtectionMode(kProtectionNackFEC); No need to set kProtectionNack if fec is disabled? https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/video_receiv... File webrtc/video/video_receive_stream.h (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/video_receiv... webrtc/video/video_receive_stream.h:123: std::unique_ptr<VCMTiming> timing_; // Jitter buffer experiment End with . Perhaps move it down to line 132?
https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:117: uint32_t timestamp = frame->timestamp; Would be better with an accessor method also for timestamp. And unit is unclear, other places use a unit-suffix, like timestamp_us() or timestamp_ms(). https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/rtp_stream_r... File webrtc/video/rtp_stream_receiver.cc (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/rtp_stream_r... webrtc/video/rtp_stream_receiver.cc:417: void RtpStreamReceiver::OnReceivedFrame( On 2016/11/08 09:27:13, philipel wrote: > When the PacketBuffer finds a complete frame this function is called. I think explanations like this belong in comments in the code. https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/video_receiv... File webrtc/video/video_receive_stream.cc (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/video_receiv... webrtc/video/video_receive_stream.cc:406: if (last_continuous_pid != -1) When using magic values like this, you have to make sure that the magic value can't occur except for the exceptional cases it is intended for. What happens if -1 is encoded on the wire (using rtp or quartc transport)? When typing the picture idd as int, it's not obvious that negative values are invalid. Other code where 0 or -1 was magic, has been converted to use rtc::Optional.
https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:261: tl0_pic_idx_ = static_cast<uint8_t>(rand()); // NOLINT On 2016/11/08 10:41:34, stefan-webrtc (holmer) wrote: > Not clear to me why this change is done, and why it's in this CL. Please > comment. Done. https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:117: uint32_t timestamp = frame->timestamp; On 2016/11/08 11:56:30, nisse-webrtc wrote: > Would be better with an accessor method also for timestamp. And unit is unclear, > other places use a unit-suffix, like timestamp_us() or timestamp_ms(). I agree on the accessor method, but just "timestamp" is established (at least in the video team) as the timestamp of a frame. Also, the unit may vary between codecs. https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:224: crit_.Enter(); On 2016/11/08 10:41:34, stefan-webrtc (holmer) wrote: > I think we should rewrite the code so that we can properly use scoped crits. For > instance you can return a list of frames to be sent out from this method and > call the callback from InsertPacket() after you have left the crit. Done. https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/video_receiver.cc (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/video_receiver.cc:293: // NOTE! Ugly hack to give the video receiver a frame to decode. On 2016/11/08 10:41:34, stefan-webrtc (holmer) wrote: > Add TODO to remove. When can it be cleaned up? And why is it needed? It's not > clear to me what the hack is about. Done. https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/rtp_stream_r... File webrtc/video/rtp_stream_receiver.cc (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/rtp_stream_r... webrtc/video/rtp_stream_receiver.cc:417: void RtpStreamReceiver::OnReceivedFrame( On 2016/11/08 10:41:34, stefan-webrtc (holmer) wrote: > On 2016/11/08 09:27:13, philipel wrote: > > When the PacketBuffer finds a complete frame this function is called. > > Can't we have RtpFrameReferenceFinder implement OnReceivedFrame instead and > bypass RtpStreamReceiver? I don't think it makes sense to modify the RtpFrameReferenceFinder just to fit this case, and also, I think this is more modular. https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/rtp_stream_r... webrtc/video/rtp_stream_receiver.cc:417: void RtpStreamReceiver::OnReceivedFrame( On 2016/11/08 11:56:30, nisse-webrtc wrote: > On 2016/11/08 09:27:13, philipel wrote: > > When the PacketBuffer finds a complete frame this function is called. > > I think explanations like this belong in comments in the code. These comments were added to make reviewing easier, but I don't think they should be in the code since they are very hard to maintain (documenting where this function is called from is not good imo). https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/rtp_stream_r... File webrtc/video/rtp_stream_receiver.h (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/rtp_stream_r... webrtc/video/rtp_stream_receiver.h:30: #include "webrtc/modules/video_coding/h264_sps_pps_tracker.h" On 2016/11/08 10:41:34, stefan-webrtc (holmer) wrote: > Sort Done. https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/rtp_stream_r... webrtc/video/rtp_stream_receiver.h:52: class NackModule; On 2016/11/08 10:41:34, stefan-webrtc (holmer) wrote: > Sort Done. https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/video_receiv... File webrtc/video/video_receive_stream.cc (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/video_receiv... webrtc/video/video_receive_stream.cc:35: #include "webrtc/modules/video_coding/frame_object.h" On 2016/11/08 10:41:35, stefan-webrtc (holmer) wrote: > Sort Done. https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/video_receiv... webrtc/video/video_receive_stream.cc:221: this, // OnCompleteFrameCallback On 2016/11/08 10:41:35, stefan-webrtc (holmer) wrote: > Should we consolidate these interfaces so that we only have to register 'this' > once? Or do they make sense to have separate? To me it doesn't make much sense to have them in one interface as that interface would be extremely specific to just initialize the RtpStreamReceiver. I think this is more modular/clear. https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/video_receiv... webrtc/video/video_receive_stream.cc:296: frame_buffer_->SetProtectionMode(kProtectionNackFEC); On 2016/11/08 10:41:35, stefan-webrtc (holmer) wrote: > No need to set kProtectionNack if fec is disabled? The FrameBuffer only change its behavior if the protection mode is kProtectionNackFEC. https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/video_receiv... webrtc/video/video_receive_stream.cc:406: if (last_continuous_pid != -1) On 2016/11/08 11:56:30, nisse-webrtc wrote: > When using magic values like this, you have to make sure that the magic value > can't occur except for the exceptional cases it is intended for. What happens if > -1 is encoded on the wire (using rtp or quartc transport)? When typing the > picture idd as int, it's not obvious that negative values are invalid. > > Other code where 0 or -1 was magic, has been converted to use rtc::Optional. Good point, but I have a few other changes I would like to do as well and I don't want to do that in this CL. https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/video_receiv... File webrtc/video/video_receive_stream.h (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/video_receiv... webrtc/video/video_receive_stream.h:123: std::unique_ptr<VCMTiming> timing_; // Jitter buffer experiment On 2016/11/08 10:41:35, stefan-webrtc (holmer) wrote: > End with . > > Perhaps move it down to line 132? Done, |timing_| has to be initialized before |rtp_stream_receiver_|.
https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:117: uint32_t timestamp = frame->timestamp; On 2016/11/08 12:28:39, philipel wrote: > On 2016/11/08 11:56:30, nisse-webrtc wrote: > > Would be better with an accessor method also for timestamp. And unit is > unclear, > > other places use a unit-suffix, like timestamp_us() or timestamp_ms(). > > I agree on the accessor method, but just "timestamp" is established (at least in > the video team) as the timestamp of a frame. Also, the unit may vary between > codecs. may be rename it to rtp_timestamp
https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:117: uint32_t timestamp = frame->timestamp; On 2016/11/08 12:28:39, philipel wrote: > On 2016/11/08 11:56:30, nisse-webrtc wrote: > > Would be better with an accessor method also for timestamp. And unit is > unclear, > > other places use a unit-suffix, like timestamp_us() or timestamp_ms(). > > I agree on the accessor method, but just "timestamp" is established (at least in > the video team) as the timestamp of a frame. Also, the unit may vary between > codecs. Ouch. Does it really have to be a codec-specific unit? https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/rtp_stream_r... File webrtc/video/rtp_stream_receiver.cc (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/rtp_stream_r... webrtc/video/rtp_stream_receiver.cc:417: void RtpStreamReceiver::OnReceivedFrame( On 2016/11/08 12:28:39, philipel wrote: > On 2016/11/08 11:56:30, nisse-webrtc wrote: > > On 2016/11/08 09:27:13, philipel wrote: > > > When the PacketBuffer finds a complete frame this function is called. > > > > I think explanations like this belong in comments in the code. > > These comments were added to make reviewing easier, but I don't think they > should be in the code since they are very hard to maintain (documenting where > this function is called from is not good imo). The code comment should explain the purpose of the class. Hopefully, that can be done without brittle references to the call sites. I mean, the distinction between "OnReceivedFrame" and "OnCompleteFrame" is not that obvious, in particular since "complete" is part of the explanation of the former... Maybe rename OnCompleteFrame to OnReadyFrame or OnFrameWithSatisfiedDependencies? (Note that there are two difficult unsolved computer science problems: Cache invalidation, and how to name things).
https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:117: uint32_t timestamp = frame->timestamp; On 2016/11/08 12:37:21, danilchap wrote: > On 2016/11/08 12:28:39, philipel wrote: > > On 2016/11/08 11:56:30, nisse-webrtc wrote: > > > Would be better with an accessor method also for timestamp. And unit is > > unclear, > > > other places use a unit-suffix, like timestamp_us() or timestamp_ms(). > > > > I agree on the accessor method, but just "timestamp" is established (at least > in > > the video team) as the timestamp of a frame. Also, the unit may vary between > > codecs. > > may be rename it to rtp_timestamp Sounds good, WDYT stefan@? https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:117: uint32_t timestamp = frame->timestamp; On 2016/11/08 12:58:12, nisse-webrtc wrote: > On 2016/11/08 12:28:39, philipel wrote: > > On 2016/11/08 11:56:30, nisse-webrtc wrote: > > > Would be better with an accessor method also for timestamp. And unit is > > unclear, > > > other places use a unit-suffix, like timestamp_us() or timestamp_ms(). > > > > I agree on the accessor method, but just "timestamp" is established (at least > in > > the video team) as the timestamp of a frame. Also, the unit may vary between > > codecs. > > Ouch. Does it really have to be a codec-specific unit? Well, for VP8/VP9/H264 the timestamp is a 90 kHz clock, but that is not true for all codecs. https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/rtp_stream_r... File webrtc/video/rtp_stream_receiver.cc (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/rtp_stream_r... webrtc/video/rtp_stream_receiver.cc:417: void RtpStreamReceiver::OnReceivedFrame( On 2016/11/08 12:58:12, nisse-webrtc wrote: > On 2016/11/08 12:28:39, philipel wrote: > > On 2016/11/08 11:56:30, nisse-webrtc wrote: > > > On 2016/11/08 09:27:13, philipel wrote: > > > > When the PacketBuffer finds a complete frame this function is called. > > > > > > I think explanations like this belong in comments in the code. > > > > These comments were added to make reviewing easier, but I don't think they > > should be in the code since they are very hard to maintain (documenting where > > this function is called from is not good imo). > > The code comment should explain the purpose of the class. Hopefully, that can be > done without brittle references to the call sites. > > I mean, the distinction between "OnReceivedFrame" and "OnCompleteFrame" is not > that obvious, in particular since "complete" is part of the explanation of the > former... > > Maybe rename OnCompleteFrame to OnReadyFrame or > OnFrameWithSatisfiedDependencies? > > (Note that there are two difficult unsolved computer science problems: Cache > invalidation, and how to name things). There are comments for OnReceivedFrameCallback (packet_buffer.h:34) and OnCompleteFrameCallback (rtp_frame_reference_finder.h:32). And yes, naming things are hard (FrameObject) :)
https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:261: tl0_pic_idx_ = static_cast<uint8_t>(rand()); // NOLINT On 2016/11/08 12:28:39, philipel wrote: > On 2016/11/08 10:41:34, stefan-webrtc (holmer) wrote: > > Not clear to me why this change is done, and why it's in this CL. Please > > comment. > > Done. The problem is that if we're relying on this change for the new JB to work, we'll not be compatible with old Chrome versions. So either we'll have to filter out VP9 from the experiment for a couple of versions, or we find a way to handle it on the receiver... https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/rtp_stream_r... File webrtc/video/rtp_stream_receiver.h (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/rtp_stream_r... webrtc/video/rtp_stream_receiver.h:187: bool jitter_buffer_experiment_; On 2016/11/08 10:41:34, stefan-webrtc (holmer) wrote: > This would be good to make const. I still think we should make this const. :) https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/video_receiv... File webrtc/video/video_receive_stream.cc (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/video_receiv... webrtc/video/video_receive_stream.cc:221: this, // OnCompleteFrameCallback On 2016/11/08 12:28:39, philipel wrote: > On 2016/11/08 10:41:35, stefan-webrtc (holmer) wrote: > > Should we consolidate these interfaces so that we only have to register 'this' > > once? Or do they make sense to have separate? > > To me it doesn't make much sense to have them in one interface as that interface > would be extremely specific to just initialize the RtpStreamReceiver. I think > this is more modular/clear. Acknowledged. https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/video_receiv... webrtc/video/video_receive_stream.cc:296: frame_buffer_->SetProtectionMode(kProtectionNackFEC); On 2016/11/08 12:28:39, philipel wrote: > On 2016/11/08 10:41:35, stefan-webrtc (holmer) wrote: > > No need to set kProtectionNack if fec is disabled? > > The FrameBuffer only change its behavior if the protection mode is > kProtectionNackFEC. Acknowledged. https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/video_receiv... File webrtc/video/video_receive_stream.h (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/video_receiv... webrtc/video/video_receive_stream.h:123: std::unique_ptr<VCMTiming> timing_; // Jitter buffer experiment On 2016/11/08 12:28:39, philipel wrote: > On 2016/11/08 10:41:35, stefan-webrtc (holmer) wrote: > > End with . > > > > Perhaps move it down to line 132? > > Done, |timing_| has to be initialized before |rtp_stream_receiver_|. Acknowledged. https://codereview.webrtc.org/2480293002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/2480293002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:103: FindFrames(seq_num); This shouldn't be called from within crit_ now as we'll take crit_ twice. I think it would have been better to return a list of frames from FindFrames. That's also what it sounds like the method does. :)
Description was changed from ========== New jitter buffer experiment. BUG=webrtc:5514 ========== to ========== New jitter buffer experiment. BUG=webrtc:5514 ==========
danilchap@webrtc.org changed reviewers: - danilchap@webrtc.org
Since I'm not familiar enough with receive stream code and this CL is about wiring jitter buffer into it, it would be hard for me to provide timely review, removing myself from reviewers.
https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/codecs/vp9/vp9_impl.cc:261: tl0_pic_idx_ = static_cast<uint8_t>(rand()); // NOLINT On 2016/11/08 14:49:27, stefan-webrtc (holmer) wrote: > On 2016/11/08 12:28:39, philipel wrote: > > On 2016/11/08 10:41:34, stefan-webrtc (holmer) wrote: > > > Not clear to me why this change is done, and why it's in this CL. Please > > > comment. > > > > Done. > > The problem is that if we're relying on this change for the new JB to work, > we'll not be compatible with old Chrome versions. So either we'll have to filter > out VP9 from the experiment for a couple of versions, or we find a way to handle > it on the receiver... Fixed in RtpFrameReferenceFinder https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/rtp_stream_r... File webrtc/video/rtp_stream_receiver.h (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/video/rtp_stream_r... webrtc/video/rtp_stream_receiver.h:187: bool jitter_buffer_experiment_; On 2016/11/08 14:49:27, stefan-webrtc (holmer) wrote: > On 2016/11/08 10:41:34, stefan-webrtc (holmer) wrote: > > This would be good to make const. > > I still think we should make this const. :) Done :) https://codereview.webrtc.org/2480293002/diff/80001/webrtc/modules/video_codi... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/2480293002/diff/80001/webrtc/modules/video_codi... webrtc/modules/video_coding/packet_buffer.cc:103: FindFrames(seq_num); On 2016/11/08 14:49:27, stefan-webrtc (holmer) wrote: > This shouldn't be called from within crit_ now as we'll take crit_ twice. > > I think it would have been better to return a list of frames from FindFrames. > That's also what it sounds like the method does. :) FindFrames now return a vector of found frames.
https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/2480293002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:117: uint32_t timestamp = frame->timestamp; On 2016/11/08 13:35:51, philipel wrote: > On 2016/11/08 12:37:21, danilchap wrote: > > On 2016/11/08 12:28:39, philipel wrote: > > > On 2016/11/08 11:56:30, nisse-webrtc wrote: > > > > Would be better with an accessor method also for timestamp. And unit is > > > unclear, > > > > other places use a unit-suffix, like timestamp_us() or timestamp_ms(). > > > > > > I agree on the accessor method, but just "timestamp" is established (at > least > > in > > > the video team) as the timestamp of a frame. Also, the unit may vary between > > > codecs. > > > > may be rename it to rtp_timestamp > > Sounds good, WDYT stefan@? It makes some sense to me, if that's what it is used for. And then it's unlikely to ever be anything but units of the 90 kHz clock? But I'm still trying to understand the codec dependency. The RTP profile for any particular codec specifies the unit of rtp timestamps on the wire, which is 90 kHz for all codecs we care about. Right? But is there anything outside of the RTP code that really needs 90kHz? E.g., are these timestamps somehow used in combination with pts/dts timestamps in the bitstream? For quic, I think it's unlikely that we'll have any 90 kHz timestamps on the wire, so that's one motivation to try to move away from rtp-specific units here. I have been trying to push for us units everywhere in the video pipeline; that's sufficient accuracy that it can be converted to 90kHz and back when needed, without worrying about the small rounding errors.
This jump detection / correction is pretty complex and non-trivial to review. Whatever we can do to simplify it would be good. https://codereview.webrtc.org/2480293002/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/2480293002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/packet_buffer.h:102: // Return a vector received frames. Returns a vector of received frames https://codereview.webrtc.org/2480293002/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/rtp_frame_reference_finder.cc (right): https://codereview.webrtc.org/2480293002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:593: bool RtpFrameReferenceFinder::Vp9PidTl0Fix(const RtpFrameObject& frame, I think we'll have to add tests to make sure these cases work as intended... :) https://codereview.webrtc.org/2480293002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:613: vp9_fix_jump_timestamp_ = -1; Should vp9_fix_last_picture_id_ be reset here too? https://codereview.webrtc.org/2480293002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:629: if (AheadOrAt<uint32_t>(frame.timestamp, vp9_fix_last_timestamp_) && I think it could have been good to split this method into three different: DetectPicIdJump() DetectTl0PicIdxJump() CorrectForJump() WDYT? https://codereview.webrtc.org/2480293002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:636: // Test if we have jump forward too much. The reason we have to do this jumped forward https://codereview.webrtc.org/2480293002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:660: fixed_tl0 = Add<256>(*tl0_pic_idx, vp9_fix_tl0_pic_idx_offset_); Should we have a named constant 256? https://codereview.webrtc.org/2480293002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:666: vp9_fix_last_tl0_pic_idx_ = fixed_tl0; Should we break out these 4 lines into a function that can also be used at lines 618-622? Not sure if it's feasible... :) https://codereview.webrtc.org/2480293002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:668: // Test if there has been a jump backwards in tl0 pic index. Can there be a jump in tl0 pic index but not in picture id, or is it enough to track picture id jumps? https://codereview.webrtc.org/2480293002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:690: vp9_fix_pid_offset_ += 128; Can we name this constant in a good way? Maybe it's already named somewhere? https://codereview.webrtc.org/2480293002/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/rtp_frame_reference_finder.h (right): https://codereview.webrtc.org/2480293002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/rtp_frame_reference_finder.h:133: // TODO(philipel): Remove when VP9 PID does not jump mid-stream. M59 timeframe?
https://codereview.webrtc.org/2480293002/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/packet_buffer.h (right): https://codereview.webrtc.org/2480293002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/packet_buffer.h:102: // Return a vector received frames. On 2016/11/14 12:00:26, stefan-webrtc (holmer) wrote: > Returns a vector of received frames Done. https://codereview.webrtc.org/2480293002/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/rtp_frame_reference_finder.cc (right): https://codereview.webrtc.org/2480293002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:593: bool RtpFrameReferenceFinder::Vp9PidTl0Fix(const RtpFrameObject& frame, On 2016/11/14 12:00:26, stefan-webrtc (holmer) wrote: > I think we'll have to add tests to make sure these cases work as intended... :) Done. https://codereview.webrtc.org/2480293002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:613: vp9_fix_jump_timestamp_ = -1; On 2016/11/14 12:00:26, stefan-webrtc (holmer) wrote: > Should vp9_fix_last_picture_id_ be reset here too? Not needed since |vp9_fix_last_picture_id_| is continuously updated so it will never become old. https://codereview.webrtc.org/2480293002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:629: if (AheadOrAt<uint32_t>(frame.timestamp, vp9_fix_last_timestamp_) && On 2016/11/14 12:00:26, stefan-webrtc (holmer) wrote: > I think it could have been good to split this method into three different: > DetectPicIdJump() > DetectTl0PicIdxJump() > CorrectForJump() > > WDYT? Done, but not CorrectForJump (annoying to return multiple recalculated values). https://codereview.webrtc.org/2480293002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:636: // Test if we have jump forward too much. The reason we have to do this On 2016/11/14 12:00:26, stefan-webrtc (holmer) wrote: > jumped forward Done. https://codereview.webrtc.org/2480293002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:660: fixed_tl0 = Add<256>(*tl0_pic_idx, vp9_fix_tl0_pic_idx_offset_); On 2016/11/14 12:00:26, stefan-webrtc (holmer) wrote: > Should we have a named constant 256? Done. https://codereview.webrtc.org/2480293002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:666: vp9_fix_last_tl0_pic_idx_ = fixed_tl0; On 2016/11/14 12:00:26, stefan-webrtc (holmer) wrote: > Should we break out these 4 lines into a function that can also be used at lines > 618-622? Not sure if it's feasible... :) Not really feasible... https://codereview.webrtc.org/2480293002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:668: // Test if there has been a jump backwards in tl0 pic index. On 2016/11/14 12:00:26, stefan-webrtc (holmer) wrote: > Can there be a jump in tl0 pic index but not in picture id, or is it enough to > track picture id jumps? I guess picture id might be enough but I think it's better if we track both. https://codereview.webrtc.org/2480293002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:690: vp9_fix_pid_offset_ += 128; On 2016/11/14 12:00:26, stefan-webrtc (holmer) wrote: > Can we name this constant in a good way? Maybe it's already named somewhere? Done. https://codereview.webrtc.org/2480293002/diff/120001/webrtc/modules/video_cod... File webrtc/modules/video_coding/rtp_frame_reference_finder.h (right): https://codereview.webrtc.org/2480293002/diff/120001/webrtc/modules/video_cod... webrtc/modules/video_coding/rtp_frame_reference_finder.h:133: // TODO(philipel): Remove when VP9 PID does not jump mid-stream. On 2016/11/14 12:00:26, stefan-webrtc (holmer) wrote: > M59 timeframe? Added comment.
lgtm % nit https://codereview.webrtc.org/2480293002/diff/140001/webrtc/modules/video_cod... File webrtc/modules/video_coding/rtp_frame_reference_finder.cc (right): https://codereview.webrtc.org/2480293002/diff/140001/webrtc/modules/video_cod... webrtc/modules/video_coding/rtp_frame_reference_finder.cc:641: has_jumped |= DetectVp9Tl0PicIdxJump(fixed_tl0, frame.timestamp); Call this only if has_jumped isn't already true.
You should probably get an lg from nisse too before landing.
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/...
Nisse, PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_x64_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...)
lgtm https://codereview.webrtc.org/2480293002/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/2480293002/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/packet_buffer.cc:79: if (sequence_buffer_[index].used) { I suspect the sequence_buffer_ logic could be simplified a bit (or maybe it's just using different conventions than what I'm familiar with), but that might not fit with this cl.
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/2480293002/#ps160001 (title: "Nit fix.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2480293002/diff/160001/webrtc/modules/video_cod... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/2480293002/diff/160001/webrtc/modules/video_cod... webrtc/modules/video_coding/packet_buffer.cc:79: if (sequence_buffer_[index].used) { On 2016/11/15 08:36:29, nisse-webrtc wrote: > I suspect the sequence_buffer_ logic could be simplified a bit (or maybe it's > just using different conventions than what I'm familiar with), but that might > not fit with this cl. Could you follow-up on this offline? It would be nice to simplify if possible!
Message was sent while issue was closed.
Description was changed from ========== New jitter buffer experiment. BUG=webrtc:5514 ========== to ========== New jitter buffer experiment. BUG=webrtc:5514 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== New jitter buffer experiment. BUG=webrtc:5514 ========== to ========== New jitter buffer experiment. BUG=webrtc:5514 Committed: https://crrev.com/fd5a20fd683c826bb26b3a03abb068c1ce084a7b Cr-Commit-Position: refs/heads/master@{#15077} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/fd5a20fd683c826bb26b3a03abb068c1ce084a7b Cr-Commit-Position: refs/heads/master@{#15077} |
