Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1211873004:
Request keyframe if too many packets are missing and NACK is disabled. (Closed)
|
Created:
5 years, 5 months ago by joachim Modified:
5 years ago CC:
webrtc-reviews_webrtc.org, yujie_mao (webrtc), tterriberry_mozilla.com, mflodman, perkj_webrtc, andresp Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRequest keyframe if too many packets are missing and NACK is disabled.
This allows enabling "EndToEndTest.ReceivesPliAndRecoversWithoutNack".
BUG=webrtc:2250
Committed: https://crrev.com/db81ffd6f50d487441555947ec63ccc766e75043
Cr-Commit-Position: refs/heads/master@{#10747}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Renamed method + updated comment #
Total comments: 3
Patch Set 3 : Updated how to request keyframes #
Total comments: 12
Patch Set 4 : Updated based on feedback #
Total comments: 6
Patch Set 5 : No more custom checking for too many discontinuous frames. #Patch Set 6 : Brought back code removed in prev. change, fixes failing EndToEndTests. #
Total comments: 18
Patch Set 7 : Updated condition #Patch Set 8 : More updates after discussion with stefan #Patch Set 9 : Rebased #Patch Set 10 : Updated condition for fetching from incomplete_frames_. #
Messages
Total messages: 57 (11 generated)
jbauch@webrtc.org changed reviewers: + stefan@webrtc.org
PTAL
pbos@webrtc.org changed reviewers: + pbos@webrtc.org
Thanks a lot for taking over this. :) https://codereview.webrtc.org/1211873004/diff/1/webrtc/modules/video_coding/m... File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right): https://codereview.webrtc.org/1211873004/diff/1/webrtc/modules/video_coding/m... webrtc/modules/video_coding/main/source/jitter_buffer.cc:952: // Packets missing but NACK disabled, request key frame. Should we request a keyframe even though the sequence is decodable? That might make sense, since quality will start to deteriorate. Also should this be called UpdateNackList if it's not related to NACK? Should this check be plugged in elsewhere, or method renamed?
https://codereview.webrtc.org/1211873004/diff/1/webrtc/modules/video_coding/m... File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right): https://codereview.webrtc.org/1211873004/diff/1/webrtc/modules/video_coding/m... webrtc/modules/video_coding/main/source/jitter_buffer.cc:952: // Packets missing but NACK disabled, request key frame. On 2015/06/26 05:01:02, pbos-webrtc wrote: > Should we request a keyframe even though the sequence is decodable? That might > make sense, since quality will start to deteriorate. I'm not sure how to check if the sequence is decodable at that point. Isn't that something only the codec knows? > Also should this be called UpdateNackList if it's not related to NACK? Should > this check be plugged in elsewhere, or method renamed? I renamed the method to "CoverSequenceUpTo" and updated the comment in the header file.
On 2015/06/26 07:46:01, joachim wrote: > https://codereview.webrtc.org/1211873004/diff/1/webrtc/modules/video_coding/m... > File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right): > > https://codereview.webrtc.org/1211873004/diff/1/webrtc/modules/video_coding/m... > webrtc/modules/video_coding/main/source/jitter_buffer.cc:952: // Packets missing > but NACK disabled, request key frame. > On 2015/06/26 05:01:02, pbos-webrtc wrote: > > Should we request a keyframe even though the sequence is decodable? That might > > make sense, since quality will start to deteriorate. > > I'm not sure how to check if the sequence is decodable at that point. Isn't that > something only the codec knows? > > > Also should this be called UpdateNackList if it's not related to NACK? Should > > this check be plugged in elsewhere, or method renamed? > > I renamed the method to "CoverSequenceUpTo" and updated the comment in the > header file. I think it makes most sense for stefan@ to review the rest, just some thoughts I had. I'm not very comfortable in the jitter-buffer space. :)
On 2015/06/26 12:32:43, pbos-webrtc wrote: > On 2015/06/26 07:46:01, joachim wrote: > > > https://codereview.webrtc.org/1211873004/diff/1/webrtc/modules/video_coding/m... > > File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right): > > > > > https://codereview.webrtc.org/1211873004/diff/1/webrtc/modules/video_coding/m... > > webrtc/modules/video_coding/main/source/jitter_buffer.cc:952: // Packets > missing > > but NACK disabled, request key frame. > > On 2015/06/26 05:01:02, pbos-webrtc wrote: > > > Should we request a keyframe even though the sequence is decodable? That > might > > > make sense, since quality will start to deteriorate. > > > > I'm not sure how to check if the sequence is decodable at that point. Isn't > that > > something only the codec knows? > > > > > Also should this be called UpdateNackList if it's not related to NACK? > Should > > > this check be plugged in elsewhere, or method renamed? > > > > I renamed the method to "CoverSequenceUpTo" and updated the comment in the > > header file. > > I think it makes most sense for stefan@ to review the rest, just some thoughts I > had. I'm not very comfortable in the jitter-buffer space. :) Stefan, ping?
https://codereview.chromium.org/1211873004/diff/20001/webrtc/modules/video_co... File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right): https://codereview.chromium.org/1211873004/diff/20001/webrtc/modules/video_co... webrtc/modules/video_coding/main/source/jitter_buffer.cc:719: if (decode_error_mode_ == kWithErrors) I think you have to take a look at how this stuff works to make a proper fix to this bug. https://codereview.chromium.org/1211873004/diff/20001/webrtc/modules/video_co... webrtc/modules/video_coding/main/source/jitter_buffer.cc:768: // 2. The end of the list was reached. Note how this method takes temporal layers into account. We don't want to lose that ability just because we have disabled NACK. https://codereview.chromium.org/1211873004/diff/20001/webrtc/modules/video_co... webrtc/modules/video_coding/main/source/jitter_buffer.cc:952: // Packets missing but NACK disabled, request key frame. This isn't a good way of handling lost packets since a lost packet doesn't necessarily imply a decodable frame sequence. Some decoders (vp8 and h.264) can decode frames with missing packets and damaged references in a (sometimes) satisfactory way. Therefore we have traditionally left the decision to request a key frame or not to the decoder (in non-NACK mode, that is). Other ways than a key frame can also be used for the recovery of a loss. Some frame sequences may use layering to achieve robustness, so that for instance base layer frames can be decoded even if higher layers are lost. This solution will break that functionality when NACK is disabled. See the other places in the code I commented on.
On 2015/07/01 13:41:03, stefan-webrtc (holmer) wrote: > https://codereview.chromium.org/1211873004/diff/20001/webrtc/modules/video_co... > File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right): > > https://codereview.chromium.org/1211873004/diff/20001/webrtc/modules/video_co... > webrtc/modules/video_coding/main/source/jitter_buffer.cc:719: if > (decode_error_mode_ == kWithErrors) > I think you have to take a look at how this stuff works to make a proper fix to > this bug. > > https://codereview.chromium.org/1211873004/diff/20001/webrtc/modules/video_co... > webrtc/modules/video_coding/main/source/jitter_buffer.cc:768: // 2. The end of > the list was reached. > Note how this method takes temporal layers into account. We don't want to lose > that ability just because we have disabled NACK. > > https://codereview.chromium.org/1211873004/diff/20001/webrtc/modules/video_co... > webrtc/modules/video_coding/main/source/jitter_buffer.cc:952: // Packets missing > but NACK disabled, request key frame. > This isn't a good way of handling lost packets since a lost packet doesn't > necessarily imply a decodable frame sequence. > > Some decoders (vp8 and h.264) can decode frames with missing packets and damaged > references in a (sometimes) satisfactory way. Therefore we have traditionally > left the decision to request a key frame or not to the decoder (in non-NACK > mode, that is). Other ways than a key frame can also be used for the recovery of > a loss. > > Some frame sequences may use layering to achieve robustness, so that for > instance base layer frames can be decoded even if higher layers are lost. This > solution will break that functionality when NACK is disabled. > > See the other places in the code I commented on. Is this CL actively being worked on, or should it be closed?
Sorry, I lost track of this... Just updated the code based on your feedback and validated using the "*Jitter*" tests of "modules_unittests" and "*EndToEnd*" of "video_engine_tests" (which all are passing). Please let me know what you think. https://codereview.webrtc.org/1211873004/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right): https://codereview.webrtc.org/1211873004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/jitter_buffer.cc:38: static const int64_t kMaxDiscontinuousFramesTime = 10000; I based the timeout on the behaviour of "EndToEndTest.ReceivesPliAndRecoversWithNack" which requests a keyframe after about 10 seconds.
On 2015/09/03 00:05:01, joachim wrote: > Sorry, I lost track of this... > > Just updated the code based on your feedback and validated using the "*Jitter*" > tests of "modules_unittests" and "*EndToEnd*" of "video_engine_tests" (which all > are passing). > > Please let me know what you think. > > https://codereview.webrtc.org/1211873004/diff/40001/webrtc/modules/video_codi... > File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right): > > https://codereview.webrtc.org/1211873004/diff/40001/webrtc/modules/video_codi... > webrtc/modules/video_coding/main/source/jitter_buffer.cc:38: static const > int64_t kMaxDiscontinuousFramesTime = 10000; > I based the timeout on the behaviour of > "EndToEndTest.ReceivesPliAndRecoversWithNack" which requests a keyframe after > about 10 seconds. Ping, does this look better now, or do you have other suggestions what should be changed?
Sorry for the delay. Could you explain how you want this to work in a bit more detail, it would be easier to review it that way. Thanks! https://codereview.webrtc.org/1211873004/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right): https://codereview.webrtc.org/1211873004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/jitter_buffer.cc:38: static const int64_t kMaxDiscontinuousFramesTime = 10000; On 2015/09/03 00:05:01, joachim wrote: > I based the timeout on the behaviour of > "EndToEndTest.ReceivesPliAndRecoversWithNack" which requests a keyframe after > about 10 seconds. Acknowledged. https://codereview.webrtc.org/1211873004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/jitter_buffer.cc:705: (decode_error_mode_ != kWithErrors || nack_mode_ != kNoNack)) { Can you comment on this if-statement? Especially I don't understand why we are checking decode_error_mode_ or nack_mode_. https://codereview.webrtc.org/1211873004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/jitter_buffer.cc:741: if (nack_mode_ == kNoNack && continuous && !continuous_for_decoding) { Why do you want to do this only for kDecodableSession? Is it not worth doing for kIncomplete too? https://codereview.webrtc.org/1211873004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/jitter_buffer.cc:783: bool VCMJitterBuffer::IsContinuousInState(const VCMFrameBuffer& frame, Should this method have a different name? I'd say it rather checks if a frame is considered decodable or not? Doesn't have to be Continuous https://codereview.webrtc.org/1211873004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/jitter_buffer.cc:788: return IsContinuousInStateForDecoding(frame, decoding_state); return decode_error_mode_ == kWithErrors || IsContinuous...(); https://codereview.webrtc.org/1211873004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/jitter_buffer.cc:822: bool VCMJitterBuffer::IsContinuousForDecoding( I don't like how this duplicates the code of IsContinuous()
Thanks for your feedback, I updated the CL based on your comments. https://codereview.webrtc.org/1211873004/diff/40001/webrtc/modules/video_codi... File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right): https://codereview.webrtc.org/1211873004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/jitter_buffer.cc:705: (decode_error_mode_ != kWithErrors || nack_mode_ != kNoNack)) { On 2015/09/18 14:22:41, stefan-webrtc (holmer) wrote: > Can you comment on this if-statement? Especially I don't understand why we are > checking decode_error_mode_ or nack_mode_. If "IsContinuous" returns true and decode_error_mode_ is not kWithErrors, the frame is always continuous for decoding (already checked in "IsContinuous"). Also if nack_mode_ is enabled, NACKs will be used to report missing data to the sender and the value of "continuous_for_decoding" is not used below. Added comment about that. https://codereview.webrtc.org/1211873004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/jitter_buffer.cc:741: if (nack_mode_ == kNoNack && continuous && !continuous_for_decoding) { On 2015/09/18 14:22:42, stefan-webrtc (holmer) wrote: > Why do you want to do this only for kDecodableSession? Is it not worth doing for > kIncomplete too? Right, added there too. https://codereview.webrtc.org/1211873004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/jitter_buffer.cc:783: bool VCMJitterBuffer::IsContinuousInState(const VCMFrameBuffer& frame, On 2015/09/18 14:22:42, stefan-webrtc (holmer) wrote: > Should this method have a different name? I'd say it rather checks if a frame is > considered decodable or not? Doesn't have to be Continuous As the functionality of "IsContinuousInState" is the same as before, I didn't want to change the name. https://codereview.webrtc.org/1211873004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/jitter_buffer.cc:788: return IsContinuousInStateForDecoding(frame, decoding_state); On 2015/09/18 14:22:41, stefan-webrtc (holmer) wrote: > return decode_error_mode_ == kWithErrors || IsContinuous...(); Refactored, so that doesn't apply any more. https://codereview.webrtc.org/1211873004/diff/40001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/jitter_buffer.cc:822: bool VCMJitterBuffer::IsContinuousForDecoding( On 2015/09/18 14:22:41, stefan-webrtc (holmer) wrote: > I don't like how this duplicates the code of IsContinuous() Updated to take a parameter for the two modes.
Thanks for redoing it a bit, a lot easier to understand now. https://codereview.webrtc.org/1211873004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right): https://codereview.webrtc.org/1211873004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/jitter_buffer.cc:704: if (continuous && I think I'm starting to see what you want to do here. You want to see if there are too many non-decodable frames, and in that case flush and request a key frame? If you look at line 983, I think we have code that can do that for you. I think it's equally valid to request a key frame if the number of non-decodable frames has grown too large even if NACK is on, or? https://codereview.webrtc.org/1211873004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/jitter_buffer.cc:983: if (max_incomplete_time_ms_ > 0) { I wonder if what you really want to do is something like what we are doing here?
Thanks for your feedback. https://codereview.webrtc.org/1211873004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right): https://codereview.webrtc.org/1211873004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/jitter_buffer.cc:704: if (continuous && On 2015/09/24 07:05:48, stefan-webrtc (holmer) wrote: > I think I'm starting to see what you want to do here. You want to see if there > are too many non-decodable frames, and in that case flush and request a key > frame? > > If you look at line 983, I think we have code that can do that for you. Thanks, however with NACKs disabled and "decode_error_mode_ = kWithErrors", no frames will be added to "incomplete_frames_", so "NonContinuousOrIncompleteDuration" as used in line 983 always returns "0". > I think it's equally valid to request a key frame if the number of non-decodable > frames has grown too large even if NACK is on, or? Isn't this already handled in "GetNackList" and "UpdateNackList"? https://codereview.webrtc.org/1211873004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/jitter_buffer.cc:983: if (max_incomplete_time_ms_ > 0) { On 2015/09/24 07:05:47, stefan-webrtc (holmer) wrote: > I wonder if what you really want to do is something like what we are doing here? Yes, that looks similar, see my other comment regarding "NonContinuousOrIncompleteDuration" in my scenario.
https://codereview.webrtc.org/1211873004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right): https://codereview.webrtc.org/1211873004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/jitter_buffer.cc:704: if (continuous && On 2015/09/24 08:52:46, joachim (ooo until oct 17) wrote: > On 2015/09/24 07:05:48, stefan-webrtc (holmer) wrote: > > I think I'm starting to see what you want to do here. You want to see if there > > are too many non-decodable frames, and in that case flush and request a key > > frame? > > > > If you look at line 983, I think we have code that can do that for you. > > Thanks, however with NACKs disabled and "decode_error_mode_ = kWithErrors", no > frames will be added to "incomplete_frames_", so > "NonContinuousOrIncompleteDuration" as used in line 983 always returns "0". I see. In that case, would it make sense to only put complete and continuous frames in decodable_frames_ also for the decode_error_mode_ and allow picking frames from incomplete_frames_ for decoding when in that mode? I'd prefer if we could reuse the NonContinuousOrIncompleteDuration code in some way since we already have that functionality. > > > I think it's equally valid to request a key frame if the number of > non-decodable > > frames has grown too large even if NACK is on, or? > > Isn't this already handled in "GetNackList" and "UpdateNackList"?
On 2015/09/28 13:49:24, stefan-webrtc (holmer) wrote: > https://codereview.webrtc.org/1211873004/diff/60001/webrtc/modules/video_codi... > File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right): > > https://codereview.webrtc.org/1211873004/diff/60001/webrtc/modules/video_codi... > webrtc/modules/video_coding/main/source/jitter_buffer.cc:704: if (continuous && > On 2015/09/24 08:52:46, joachim (ooo until oct 17) wrote: > > On 2015/09/24 07:05:48, stefan-webrtc (holmer) wrote: > > > I think I'm starting to see what you want to do here. You want to see if > there > > > are too many non-decodable frames, and in that case flush and request a key > > > frame? > > > > > > If you look at line 983, I think we have code that can do that for you. > > > > Thanks, however with NACKs disabled and "decode_error_mode_ = kWithErrors", no > > frames will be added to "incomplete_frames_", so > > "NonContinuousOrIncompleteDuration" as used in line 983 always returns "0". > > > I see. In that case, would it make sense to only put complete and continuous > frames in decodable_frames_ also for the decode_error_mode_ and allow picking > frames from incomplete_frames_ for decoding when in that mode? I'd prefer if we > could reuse the NonContinuousOrIncompleteDuration code in some way since we > already have that functionality. I'm back from vacation and will try to take a look at your suggestions regarding "NonContinuousOrIncompleteDuration" over the next days. > > > > > I think it's equally valid to request a key frame if the number of > > non-decodable > > > frames has grown too large even if NACK is on, or? > > > > Isn't this already handled in "GetNackList" and "UpdateNackList"?
@Stefan: I finally had some time to add the changes you suggested. All *EndToEnd* in video_engine_tests and the *Jitter* in modules_unittests are running fine. ptal https://codereview.webrtc.org/1211873004/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right): https://codereview.webrtc.org/1211873004/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/main/source/jitter_buffer.cc:704: if (continuous && On 2015/09/28 13:49:24, stefan-webrtc (holmer) wrote: > On 2015/09/24 08:52:46, joachim (ooo until oct 17) wrote: > > On 2015/09/24 07:05:48, stefan-webrtc (holmer) wrote: > > > I think I'm starting to see what you want to do here. You want to see if > there > > > are too many non-decodable frames, and in that case flush and request a key > > > frame? > > > > > > If you look at line 983, I think we have code that can do that for you. > > > > Thanks, however with NACKs disabled and "decode_error_mode_ = kWithErrors", no > > frames will be added to "incomplete_frames_", so > > "NonContinuousOrIncompleteDuration" as used in line 983 always returns "0". > > > I see. In that case, would it make sense to only put complete and continuous > frames in decodable_frames_ also for the decode_error_mode_ and allow picking > frames from incomplete_frames_ for decoding when in that mode? I'd prefer if we > could reuse the NonContinuousOrIncompleteDuration code in some way since we > already have that functionality. I updated the code as you suggested, looks a lot cleaner now imho. > > > > > > > I think it's equally valid to request a key frame if the number of > > non-decodable > > > frames has grown too large even if NACK is on, or? > > > > Isn't this already handled in "GetNackList" and "UpdateNackList"?
On 2015/11/05 23:48:01, joachim wrote: > @Stefan: I finally had some time to add the changes you suggested. > > All *EndToEnd* in video_engine_tests and the *Jitter* in modules_unittests are > running fine. > > ptal > > https://codereview.webrtc.org/1211873004/diff/60001/webrtc/modules/video_codi... > File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right): > > https://codereview.webrtc.org/1211873004/diff/60001/webrtc/modules/video_codi... > webrtc/modules/video_coding/main/source/jitter_buffer.cc:704: if (continuous && > On 2015/09/28 13:49:24, stefan-webrtc (holmer) wrote: > > On 2015/09/24 08:52:46, joachim (ooo until oct 17) wrote: > > > On 2015/09/24 07:05:48, stefan-webrtc (holmer) wrote: > > > > I think I'm starting to see what you want to do here. You want to see if > > there > > > > are too many non-decodable frames, and in that case flush and request a > key > > > > frame? > > > > > > > > If you look at line 983, I think we have code that can do that for you. > > > > > > Thanks, however with NACKs disabled and "decode_error_mode_ = kWithErrors", > no > > > frames will be added to "incomplete_frames_", so > > > "NonContinuousOrIncompleteDuration" as used in line 983 always returns "0". > > > > > > I see. In that case, would it make sense to only put complete and continuous > > frames in decodable_frames_ also for the decode_error_mode_ and allow picking > > frames from incomplete_frames_ for decoding when in that mode? I'd prefer if > we > > could reuse the NonContinuousOrIncompleteDuration code in some way since we > > already have that functionality. > > I updated the code as you suggested, looks a lot cleaner now imho. > > > > > > > > > > > > I think it's equally valid to request a key frame if the number of > > > non-decodable > > > > frames has grown too large even if NACK is on, or? > > > > > > Isn't this already handled in "GetNackList" and "UpdateNackList"? ping?
Sorry for the delay, I've been traveling. A lot better now, I have a few questions still, but I think this is getting close now. https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right): https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/jitter_buffer.cc:552: && oldest_frame->GetState() != kStateComplete) { Can we end up here without oldest_frame being complete? https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/jitter_buffer.cc:781: continuous_for_decoding = true; Does this make sure that we actually wait for the packets to be nacked/retransmitted before we decode the frame though? Or can this cause us to decode the frame before the retransmissions have arrived? https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/jitter_buffer.cc:783: continuous_for_decoding = IsContinuous(*frame, true); Does this mean we require frames to be complete also for decode_error_mode_ == kWithErrors? It's not entirely clear to me in what situations we get in to this "else", and why it should ignore the error mode. Do we even need an error mode check in IsContinuous?
Thanks for your feedback, I hope my answers clarify some things... https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right): https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/jitter_buffer.cc:552: && oldest_frame->GetState() != kStateComplete) { On 2015/11/16 17:52:15, stefan-webrtc (holmer) wrote: > Can we end up here without oldest_frame being complete? Well, the condition was like that before, I just moved it. Removing the state comparison causes "TestJitterBufferNack.NormalOperation" to fail. https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/jitter_buffer.cc:781: continuous_for_decoding = true; On 2015/11/16 17:52:15, stefan-webrtc (holmer) wrote: > Does this make sure that we actually wait for the packets to be > nacked/retransmitted before we decode the frame though? Or can this cause us to > decode the frame before the retransmissions have arrived? I don't think so as the behavior for enabled NACKs was not modified with this change. Frames with "IsContinuous = true" were also added to decodable_frames_ before. https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/jitter_buffer.cc:783: continuous_for_decoding = IsContinuous(*frame, true); On 2015/11/16 17:52:15, stefan-webrtc (holmer) wrote: > Does this mean we require frames to be complete also for decode_error_mode_ == > kWithErrors? It's not entirely clear to me in what situations we get in to this > "else", and why it should ignore the error mode. Do we even need an error mode > check in IsContinuous? For "decode_error_mode_ == kWithErrors", the actual state of the frame is not checked ("IsContinuous" immediately returns "true" unless the error mode is ignored). That's why I added the second mode to "IsContinuous" that always checks the state of the frame (stored in "continuous_for_decoding_"). The "else" path is taken when NACKs are disabled and/or the decode_error_mode_ is "kWithErrors" because in these cases either missing packets won't be reported (NACKs disabled) or "IsContinuous" didn't already check the state of the frame (decode_error_mode_ == kWithErrors).
https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right): https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/jitter_buffer.cc:552: && oldest_frame->GetState() != kStateComplete) { On 2015/11/16 21:58:11, joachim wrote: > On 2015/11/16 17:52:15, stefan-webrtc (holmer) wrote: > > Can we end up here without oldest_frame being complete? > > Well, the condition was like that before, I just moved it. Removing the state > comparison causes "TestJitterBufferNack.NormalOperation" to fail. Acknowledged. https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/jitter_buffer.cc:783: continuous_for_decoding = IsContinuous(*frame, true); On 2015/11/16 21:58:11, joachim wrote: > On 2015/11/16 17:52:15, stefan-webrtc (holmer) wrote: > > Does this mean we require frames to be complete also for decode_error_mode_ == > > kWithErrors? It's not entirely clear to me in what situations we get in to > this > > "else", and why it should ignore the error mode. Do we even need an error mode > > check in IsContinuous? > > For "decode_error_mode_ == kWithErrors", the actual state of the frame is not > checked ("IsContinuous" immediately returns "true" unless the error mode is > ignored). That's why I added the second mode to "IsContinuous" that always > checks the state of the frame (stored in "continuous_for_decoding_"). I'm trying to understand when and why it is necessary to call IsContinuous with ignore_error_mode = false. Do we need to go down that code path? > > The "else" path is taken when NACKs are disabled and/or the decode_error_mode_ > is "kWithErrors" because in these cases either missing packets won't be reported > (NACKs disabled) or "IsContinuous" didn't already check the state of the frame > (decode_error_mode_ == kWithErrors). So continuous_for_decoding means that the frame is complete (all packets received) and in sequence (i.e., continuous) if we have nack and not decoding with errors. And if we are decoding with errors it means the frame is in sequence (continuous) but not necessarily that it is complete? Do we need to check both continuous and continuous_for_decoding at lines 806 and 812, or is continuous_for_decoding enough?
https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right): https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/jitter_buffer.cc:783: continuous_for_decoding = IsContinuous(*frame, true); On 2015/11/18 00:02:17, stefan-webrtc (holmer) wrote: > On 2015/11/16 21:58:11, joachim wrote: > > On 2015/11/16 17:52:15, stefan-webrtc (holmer) wrote: > > > Does this mean we require frames to be complete also for decode_error_mode_ > == > > > kWithErrors? It's not entirely clear to me in what situations we get in to > > this > > > "else", and why it should ignore the error mode. Do we even need an error > mode > > > check in IsContinuous? > > > > For "decode_error_mode_ == kWithErrors", the actual state of the frame is not > > checked ("IsContinuous" immediately returns "true" unless the error mode is > > ignored). That's why I added the second mode to "IsContinuous" that always > > checks the state of the frame (stored in "continuous_for_decoding_"). > > I'm trying to understand when and why it is necessary to call IsContinuous with > ignore_error_mode = false. Do we need to go down that code path? > The problem is, that with ignore_error_mode = true the actual frame is never checked if decoding with errors is allowed (which often is the case). This however is solved by this CL, so that with disabled NACKs (and decoding with errors allowed), the frame contents are still checked and if no continuous frame for decoding are received for too long, a keyframe can be requested. > > > > The "else" path is taken when NACKs are disabled and/or the decode_error_mode_ > > is "kWithErrors" because in these cases either missing packets won't be > reported > > (NACKs disabled) or "IsContinuous" didn't already check the state of the frame > > (decode_error_mode_ == kWithErrors). > > So continuous_for_decoding means that the frame is complete (all packets > received) and in sequence (i.e., continuous) if we have nack and not decoding > with errors. And if we are decoding with errors it means the frame is in > sequence (continuous) but not necessarily that it is complete? > > Do we need to check both continuous and continuous_for_decoding at lines 806 and > 812, or is continuous_for_decoding enough? Line 806 can be changed (verified with the Jitter and EndToEnd tests). Thanks for spotting this. Changing line 812 causes this test to fail: [ RUN ] TestRunningJitterBuffer.Full ../../webrtc/modules/video_coding/main/source/jitter_buffer_unittest.cc:2139: Failure Expected: (InsertFrames(kMaxNumberOfFrames, kVideoFrameDelta)) >= (kNoError), actual: -3 vs 0 [ FAILED ] TestRunningJitterBuffer.Full (2 ms) This is because more than 10 seconds (i.e. "kMaxDiscontinuousFramesTime") of frames are added and so a keyframe is requested. All other tests (Jitter and EndToEnd) are running without error. What do you think, should this still be changed and the test adjusted (how?), or should I keep the condition as is right now?
https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right): https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/jitter_buffer.cc:783: continuous_for_decoding = IsContinuous(*frame, true); On 2015/11/18 00:35:14, joachim wrote: > On 2015/11/18 00:02:17, stefan-webrtc (holmer) wrote: > > On 2015/11/16 21:58:11, joachim wrote: > > > On 2015/11/16 17:52:15, stefan-webrtc (holmer) wrote: > > > > Does this mean we require frames to be complete also for > decode_error_mode_ > > == > > > > kWithErrors? It's not entirely clear to me in what situations we get in to > > > this > > > > "else", and why it should ignore the error mode. Do we even need an error > > mode > > > > check in IsContinuous? > > > > > > For "decode_error_mode_ == kWithErrors", the actual state of the frame is > not > > > checked ("IsContinuous" immediately returns "true" unless the error mode is > > > ignored). That's why I added the second mode to "IsContinuous" that always > > > checks the state of the frame (stored in "continuous_for_decoding_"). > > > > I'm trying to understand when and why it is necessary to call IsContinuous > with > > ignore_error_mode = false. Do we need to go down that code path? > > > > The problem is, that with ignore_error_mode = true the actual frame is never > checked if decoding with errors is allowed (which often is the case). This > however is solved by this CL, so that with disabled NACKs (and decoding with > errors allowed), the frame contents are still checked and if no continuous frame > for decoding are received for too long, a keyframe can be requested. Btw, I did some more testing and you are right: if I change both (always checking with ignore_error_mode = true and also updating the conditions below), all Jitter/EndToEnd tests but the "TestRunningJitterBuffer.Full" are running without errors. However in the (I think) common case (NACKs enabled and decoding with errors allowed), this will always check the frame contents. > > > > > > The "else" path is taken when NACKs are disabled and/or the > decode_error_mode_ > > > is "kWithErrors" because in these cases either missing packets won't be > > reported > > > (NACKs disabled) or "IsContinuous" didn't already check the state of the > frame > > > (decode_error_mode_ == kWithErrors). > > > > So continuous_for_decoding means that the frame is complete (all packets > > received) and in sequence (i.e., continuous) if we have nack and not decoding > > with errors. And if we are decoding with errors it means the frame is in > > sequence (continuous) but not necessarily that it is complete? > > > > Do we need to check both continuous and continuous_for_decoding at lines 806 > and > > 812, or is continuous_for_decoding enough? > > Line 806 can be changed (verified with the Jitter and EndToEnd tests). Thanks > for spotting this. > > Changing line 812 causes this test to fail: > [ RUN ] TestRunningJitterBuffer.Full > ../../webrtc/modules/video_coding/main/source/jitter_buffer_unittest.cc:2139: > Failure > Expected: (InsertFrames(kMaxNumberOfFrames, kVideoFrameDelta)) >= (kNoError), > actual: -3 vs 0 > [ FAILED ] TestRunningJitterBuffer.Full (2 ms) > > This is because more than 10 seconds (i.e. "kMaxDiscontinuousFramesTime") of > frames are added and so a keyframe is requested. All other tests (Jitter and > EndToEnd) are running without error. > > What do you think, should this still be changed and the test adjusted (how?), or > should I keep the condition as is right now?
https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right): https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/jitter_buffer.cc:783: continuous_for_decoding = IsContinuous(*frame, true); On 2015/11/18 00:50:26, joachim wrote: > On 2015/11/18 00:35:14, joachim wrote: > > On 2015/11/18 00:02:17, stefan-webrtc (holmer) wrote: > > > On 2015/11/16 21:58:11, joachim wrote: > > > > On 2015/11/16 17:52:15, stefan-webrtc (holmer) wrote: > > > > > Does this mean we require frames to be complete also for > > decode_error_mode_ > > > == > > > > > kWithErrors? It's not entirely clear to me in what situations we get in > to > > > > this > > > > > "else", and why it should ignore the error mode. Do we even need an > error > > > mode > > > > > check in IsContinuous? > > > > > > > > For "decode_error_mode_ == kWithErrors", the actual state of the frame is > > not > > > > checked ("IsContinuous" immediately returns "true" unless the error mode > is > > > > ignored). That's why I added the second mode to "IsContinuous" that always > > > > checks the state of the frame (stored in "continuous_for_decoding_"). > > > > > > I'm trying to understand when and why it is necessary to call IsContinuous > > with > > > ignore_error_mode = false. Do we need to go down that code path? > > > > > > > The problem is, that with ignore_error_mode = true the actual frame is never > > checked if decoding with errors is allowed (which often is the case). This > > however is solved by this CL, so that with disabled NACKs (and decoding with > > errors allowed), the frame contents are still checked and if no continuous > frame > > for decoding are received for too long, a keyframe can be requested. > > Btw, I did some more testing and you are right: if I change both (always > checking with ignore_error_mode = true and also updating the conditions below), > all Jitter/EndToEnd tests but the "TestRunningJitterBuffer.Full" are running > without errors. > > However in the (I think) common case (NACKs enabled and decoding with errors > allowed), this will always check the frame contents. I'd say the common case is NACKs enabled and decoding with errors not allowed. > > > > > > > > > The "else" path is taken when NACKs are disabled and/or the > > decode_error_mode_ > > > > is "kWithErrors" because in these cases either missing packets won't be > > > reported > > > > (NACKs disabled) or "IsContinuous" didn't already check the state of the > > frame > > > > (decode_error_mode_ == kWithErrors). > > > > > > So continuous_for_decoding means that the frame is complete (all packets > > > received) and in sequence (i.e., continuous) if we have nack and not > decoding > > > with errors. And if we are decoding with errors it means the frame is in > > > sequence (continuous) but not necessarily that it is complete? > > > > > > Do we need to check both continuous and continuous_for_decoding at lines 806 > > and > > > 812, or is continuous_for_decoding enough? > > > > Line 806 can be changed (verified with the Jitter and EndToEnd tests). Thanks > > for spotting this. > > > > Changing line 812 causes this test to fail: > > [ RUN ] TestRunningJitterBuffer.Full > > ../../webrtc/modules/video_coding/main/source/jitter_buffer_unittest.cc:2139: > > Failure > > Expected: (InsertFrames(kMaxNumberOfFrames, kVideoFrameDelta)) >= (kNoError), > > actual: -3 vs 0 > > [ FAILED ] TestRunningJitterBuffer.Full (2 ms) > > > > This is because more than 10 seconds (i.e. "kMaxDiscontinuousFramesTime") of > > frames are added and so a keyframe is requested. All other tests (Jitter and > > EndToEnd) are running without error. Right, that seems like correct behavior. You may want to change the test to do the DropFrame(1) at the end, so that the non-decodable frames time is shorter, and then simply verify that it's getting full and causes a kFlushIndicator. > > > > What do you think, should this still be changed and the test adjusted (how?), > or > > should I keep the condition as is right now? > https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/jitter_buffer.cc:783: continuous_for_decoding = IsContinuous(*frame, true); On 2015/11/18 00:35:14, joachim wrote: > On 2015/11/18 00:02:17, stefan-webrtc (holmer) wrote: > > On 2015/11/16 21:58:11, joachim wrote: > > > On 2015/11/16 17:52:15, stefan-webrtc (holmer) wrote: > > > > Does this mean we require frames to be complete also for > decode_error_mode_ > > == > > > > kWithErrors? It's not entirely clear to me in what situations we get in to > > > this > > > > "else", and why it should ignore the error mode. Do we even need an error > > mode > > > > check in IsContinuous? > > > > > > For "decode_error_mode_ == kWithErrors", the actual state of the frame is > not > > > checked ("IsContinuous" immediately returns "true" unless the error mode is > > > ignored). That's why I added the second mode to "IsContinuous" that always > > > checks the state of the frame (stored in "continuous_for_decoding_"). > > > > I'm trying to understand when and why it is necessary to call IsContinuous > with > > ignore_error_mode = false. Do we need to go down that code path? > > > > The problem is, that with ignore_error_mode = true the actual frame is never > checked if decoding with errors is allowed (which often is the case). This > however is solved by this CL, so that with disabled NACKs (and decoding with > errors allowed), the frame contents are still checked and if no continuous frame > for decoding are received for too long, a keyframe can be requested. Yes, but doesn't that mean that we don't decode with errors any longer? That may be the right decision, but then maybe the code also shouldn't have a decode_error_mode_ mode? I think it would be fine to only decode frames that are in sequence and complete, as long as we make sure we request a key frame if we have failed to decode for too long. That is what you are trying to do here, right? > > > > > > > The "else" path is taken when NACKs are disabled and/or the > decode_error_mode_ > > > is "kWithErrors" because in these cases either missing packets won't be > > reported > > > (NACKs disabled) or "IsContinuous" didn't already check the state of the > frame > > > (decode_error_mode_ == kWithErrors). > > > > So continuous_for_decoding means that the frame is complete (all packets > > received) and in sequence (i.e., continuous) if we have nack and not decoding > > with errors. And if we are decoding with errors it means the frame is in > > sequence (continuous) but not necessarily that it is complete? > > > > Do we need to check both continuous and continuous_for_decoding at lines 806 > and > > 812, or is continuous_for_decoding enough? > > Line 806 can be changed (verified with the Jitter and EndToEnd tests). Thanks > for spotting this. > > Changing line 812 causes this test to fail: > [ RUN ] TestRunningJitterBuffer.Full > ../../webrtc/modules/video_coding/main/source/jitter_buffer_unittest.cc:2139: > Failure > Expected: (InsertFrames(kMaxNumberOfFrames, kVideoFrameDelta)) >= (kNoError), > actual: -3 vs 0 > [ FAILED ] TestRunningJitterBuffer.Full (2 ms) > > This is because more than 10 seconds (i.e. "kMaxDiscontinuousFramesTime") of > frames are added and so a keyframe is requested. All other tests (Jitter and > EndToEnd) are running without error. > > What do you think, should this still be changed and the test adjusted (how?), or > should I keep the condition as is right now?
https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right): https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/jitter_buffer.cc:783: continuous_for_decoding = IsContinuous(*frame, true); On 2015/11/18 20:51:57, stefan-webrtc (holmer) wrote: > On 2015/11/18 00:50:26, joachim wrote: > > On 2015/11/18 00:35:14, joachim wrote: > > > On 2015/11/18 00:02:17, stefan-webrtc (holmer) wrote: > > > > On 2015/11/16 21:58:11, joachim wrote: > > > > > On 2015/11/16 17:52:15, stefan-webrtc (holmer) wrote: > > > > > > Does this mean we require frames to be complete also for > > > decode_error_mode_ > > > > == > > > > > > kWithErrors? It's not entirely clear to me in what situations we get > in > > to > > > > > this > > > > > > "else", and why it should ignore the error mode. Do we even need an > > error > > > > mode > > > > > > check in IsContinuous? > > > > > > > > > > For "decode_error_mode_ == kWithErrors", the actual state of the frame > is > > > not > > > > > checked ("IsContinuous" immediately returns "true" unless the error mode > > is > > > > > ignored). That's why I added the second mode to "IsContinuous" that > always > > > > > checks the state of the frame (stored in "continuous_for_decoding_"). > > > > > > > > I'm trying to understand when and why it is necessary to call IsContinuous > > > with > > > > ignore_error_mode = false. Do we need to go down that code path? > > > > > > > > > > The problem is, that with ignore_error_mode = true the actual frame is never > > > checked if decoding with errors is allowed (which often is the case). This > > > however is solved by this CL, so that with disabled NACKs (and decoding with > > > errors allowed), the frame contents are still checked and if no continuous > > frame > > > for decoding are received for too long, a keyframe can be requested. > > > > Btw, I did some more testing and you are right: if I change both (always > > checking with ignore_error_mode = true and also updating the conditions > below), > > all Jitter/EndToEnd tests but the "TestRunningJitterBuffer.Full" are running > > without errors. > > > > However in the (I think) common case (NACKs enabled and decoding with errors > > allowed), this will always check the frame contents. > > I'd say the common case is NACKs enabled and decoding with errors not allowed. Acknowledged. > > > > > > > > > > > > The "else" path is taken when NACKs are disabled and/or the > > > decode_error_mode_ > > > > > is "kWithErrors" because in these cases either missing packets won't be > > > > reported > > > > > (NACKs disabled) or "IsContinuous" didn't already check the state of the > > > frame > > > > > (decode_error_mode_ == kWithErrors). > > > > > > > > So continuous_for_decoding means that the frame is complete (all packets > > > > received) and in sequence (i.e., continuous) if we have nack and not > > decoding > > > > with errors. And if we are decoding with errors it means the frame is in > > > > sequence (continuous) but not necessarily that it is complete? > > > > > > > > Do we need to check both continuous and continuous_for_decoding at lines > 806 > > > and > > > > 812, or is continuous_for_decoding enough? > > > > > > Line 806 can be changed (verified with the Jitter and EndToEnd tests). > Thanks > > > for spotting this. > > > > > > Changing line 812 causes this test to fail: > > > [ RUN ] TestRunningJitterBuffer.Full > > > > ../../webrtc/modules/video_coding/main/source/jitter_buffer_unittest.cc:2139: > > > Failure > > > Expected: (InsertFrames(kMaxNumberOfFrames, kVideoFrameDelta)) >= > (kNoError), > > > actual: -3 vs 0 > > > [ FAILED ] TestRunningJitterBuffer.Full (2 ms) > > > > > > This is because more than 10 seconds (i.e. "kMaxDiscontinuousFramesTime") of > > > frames are added and so a keyframe is requested. All other tests (Jitter and > > > EndToEnd) are running without error. > > Right, that seems like correct behavior. You may want to change the test to do > the DropFrame(1) at the end, so that the non-decodable frames time is shorter, > and then simply verify that it's getting full and causes a kFlushIndicator. Sorry, I don't get that. If DropFrame(1) is moved after InsertFrames(kMax..), the frames are not added to "incomplete_frames_", the new code doesn't trigger and the frames are decodable afterwards (causing the rest of the test to fail). > > > > > > What do you think, should this still be changed and the test adjusted > (how?), > > or > > > should I keep the condition as is right now? > > https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/jitter_buffer.cc:783: continuous_for_decoding = IsContinuous(*frame, true); On 2015/11/18 20:51:57, stefan-webrtc (holmer) wrote: > On 2015/11/18 00:35:14, joachim wrote: > > On 2015/11/18 00:02:17, stefan-webrtc (holmer) wrote: > > > On 2015/11/16 21:58:11, joachim wrote: > > > > On 2015/11/16 17:52:15, stefan-webrtc (holmer) wrote: > > > > > Does this mean we require frames to be complete also for > > decode_error_mode_ > > > == > > > > > kWithErrors? It's not entirely clear to me in what situations we get in > to > > > > this > > > > > "else", and why it should ignore the error mode. Do we even need an > error > > > mode > > > > > check in IsContinuous? > > > > > > > > For "decode_error_mode_ == kWithErrors", the actual state of the frame is > > not > > > > checked ("IsContinuous" immediately returns "true" unless the error mode > is > > > > ignored). That's why I added the second mode to "IsContinuous" that always > > > > checks the state of the frame (stored in "continuous_for_decoding_"). > > > > > > I'm trying to understand when and why it is necessary to call IsContinuous > > with > > > ignore_error_mode = false. Do we need to go down that code path? > > > > > > > The problem is, that with ignore_error_mode = true the actual frame is never > > checked if decoding with errors is allowed (which often is the case). This > > however is solved by this CL, so that with disabled NACKs (and decoding with > > errors allowed), the frame contents are still checked and if no continuous > frame > > for decoding are received for too long, a keyframe can be requested. > > Yes, but doesn't that mean that we don't decode with errors any longer? That may > be the right decision, but then maybe the code also shouldn't have a > decode_error_mode_ mode? I think it would be fine to only decode frames that are > in sequence and complete, as long as we make sure we request a key frame if we > have failed to decode for too long. That is what you are trying to do here, > right? Hmm. With the latest changes, decoding with errors indeed will be no longer working. That functionality was removed with patch 5 and the change to reuse existing code. Before I was still adding these frames to "decodable_frames_" and manually measuring the time since the last continuous and decodable frame to detect when to request keyframes. Reusing "NonContinuousOrIncompleteDuration" required them to be stored in "incomplete_frames_" so they don't get decoded any more. I'm not sure if that's the way to go, now a keyframe will be requested either through the existing NACK code or through the new code if no frame was decoded for too long. > > > > > > > > > > The "else" path is taken when NACKs are disabled and/or the > > decode_error_mode_ > > > > is "kWithErrors" because in these cases either missing packets won't be > > > reported > > > > (NACKs disabled) or "IsContinuous" didn't already check the state of the > > frame > > > > (decode_error_mode_ == kWithErrors). > > > > > > So continuous_for_decoding means that the frame is complete (all packets > > > received) and in sequence (i.e., continuous) if we have nack and not > decoding > > > with errors. And if we are decoding with errors it means the frame is in > > > sequence (continuous) but not necessarily that it is complete? > > > > > > Do we need to check both continuous and continuous_for_decoding at lines 806 > > and > > > 812, or is continuous_for_decoding enough? > > > > Line 806 can be changed (verified with the Jitter and EndToEnd tests). Thanks > > for spotting this. > > > > Changing line 812 causes this test to fail: > > [ RUN ] TestRunningJitterBuffer.Full > > ../../webrtc/modules/video_coding/main/source/jitter_buffer_unittest.cc:2139: > > Failure > > Expected: (InsertFrames(kMaxNumberOfFrames, kVideoFrameDelta)) >= (kNoError), > > actual: -3 vs 0 > > [ FAILED ] TestRunningJitterBuffer.Full (2 ms) > > > > This is because more than 10 seconds (i.e. "kMaxDiscontinuousFramesTime") of > > frames are added and so a keyframe is requested. All other tests (Jitter and > > EndToEnd) are running without error. > > > > What do you think, should this still be changed and the test adjusted (how?), > or > > should I keep the condition as is right now? >
https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right): https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/jitter_buffer.cc:783: continuous_for_decoding = IsContinuous(*frame, true); On 2015/11/18 23:46:07, joachim wrote: > On 2015/11/18 20:51:57, stefan-webrtc (holmer) wrote: > > On 2015/11/18 00:35:14, joachim wrote: > > > On 2015/11/18 00:02:17, stefan-webrtc (holmer) wrote: > > > > On 2015/11/16 21:58:11, joachim wrote: > > > > > On 2015/11/16 17:52:15, stefan-webrtc (holmer) wrote: > > > > > > Does this mean we require frames to be complete also for > > > decode_error_mode_ > > > > == > > > > > > kWithErrors? It's not entirely clear to me in what situations we get > in > > to > > > > > this > > > > > > "else", and why it should ignore the error mode. Do we even need an > > error > > > > mode > > > > > > check in IsContinuous? > > > > > > > > > > For "decode_error_mode_ == kWithErrors", the actual state of the frame > is > > > not > > > > > checked ("IsContinuous" immediately returns "true" unless the error mode > > is > > > > > ignored). That's why I added the second mode to "IsContinuous" that > always > > > > > checks the state of the frame (stored in "continuous_for_decoding_"). > > > > > > > > I'm trying to understand when and why it is necessary to call IsContinuous > > > with > > > > ignore_error_mode = false. Do we need to go down that code path? > > > > > > > > > > The problem is, that with ignore_error_mode = true the actual frame is never > > > checked if decoding with errors is allowed (which often is the case). This > > > however is solved by this CL, so that with disabled NACKs (and decoding with > > > errors allowed), the frame contents are still checked and if no continuous > > frame > > > for decoding are received for too long, a keyframe can be requested. > > > > Yes, but doesn't that mean that we don't decode with errors any longer? That > may > > be the right decision, but then maybe the code also shouldn't have a > > decode_error_mode_ mode? I think it would be fine to only decode frames that > are > > in sequence and complete, as long as we make sure we request a key frame if we > > have failed to decode for too long. That is what you are trying to do here, > > right? > > Hmm. With the latest changes, decoding with errors indeed will be no longer > working. That functionality was removed with patch 5 and the change to reuse > existing code. Before I was still adding these frames to "decodable_frames_" and > manually measuring the time since the last continuous and decodable frame to > detect when to request keyframes. > Reusing "NonContinuousOrIncompleteDuration" required them to be stored in > "incomplete_frames_" so they don't get decoded any more. > > I'm not sure if that's the way to go, now a keyframe will be requested either > through the existing NACK code or through the new code if no frame was decoded > for too long. > > > > > > > > > > > > > > The "else" path is taken when NACKs are disabled and/or the > > > decode_error_mode_ > > > > > is "kWithErrors" because in these cases either missing packets won't be > > > > reported > > > > > (NACKs disabled) or "IsContinuous" didn't already check the state of the > > > frame > > > > > (decode_error_mode_ == kWithErrors). > > > > > > > > So continuous_for_decoding means that the frame is complete (all packets > > > > received) and in sequence (i.e., continuous) if we have nack and not > > decoding > > > > with errors. And if we are decoding with errors it means the frame is in > > > > sequence (continuous) but not necessarily that it is complete? > > > > > > > > Do we need to check both continuous and continuous_for_decoding at lines > 806 > > > and > > > > 812, or is continuous_for_decoding enough? > > > > > > Line 806 can be changed (verified with the Jitter and EndToEnd tests). > Thanks > > > for spotting this. > > > > > > Changing line 812 causes this test to fail: > > > [ RUN ] TestRunningJitterBuffer.Full > > > > ../../webrtc/modules/video_coding/main/source/jitter_buffer_unittest.cc:2139: > > > Failure > > > Expected: (InsertFrames(kMaxNumberOfFrames, kVideoFrameDelta)) >= > (kNoError), > > > actual: -3 vs 0 > > > [ FAILED ] TestRunningJitterBuffer.Full (2 ms) > > > > > > This is because more than 10 seconds (i.e. "kMaxDiscontinuousFramesTime") of > > > frames are added and so a keyframe is requested. All other tests (Jitter and > > > EndToEnd) are running without error. > > > > > > What do you think, should this still be changed and the test adjusted > (how?), > > or > > > should I keep the condition as is right now? > > > Right. What behavior is it that you want? We typically don't use this mode and would be OK with only decoding frames without error, but making sure we send PLI/key frame requests often when there is packet loss. If you however do want to decode with errors your previous version is probably more correct... Sorry in that case if I led you down the wrong path. If we decide not to decode with errors, I think we should clean that code up too.
On 2015/11/18 23:51:03, stefan-webrtc (holmer) wrote: > https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... > File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right): > > https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... > webrtc/modules/video_coding/main/source/jitter_buffer.cc:783: > continuous_for_decoding = IsContinuous(*frame, true); > On 2015/11/18 23:46:07, joachim wrote: > > On 2015/11/18 20:51:57, stefan-webrtc (holmer) wrote: > > > On 2015/11/18 00:35:14, joachim wrote: > > > > On 2015/11/18 00:02:17, stefan-webrtc (holmer) wrote: > > > > > On 2015/11/16 21:58:11, joachim wrote: > > > > > > On 2015/11/16 17:52:15, stefan-webrtc (holmer) wrote: > > > > > > > Does this mean we require frames to be complete also for > > > > decode_error_mode_ > > > > > == > > > > > > > kWithErrors? It's not entirely clear to me in what situations we get > > in > > > to > > > > > > this > > > > > > > "else", and why it should ignore the error mode. Do we even need an > > > error > > > > > mode > > > > > > > check in IsContinuous? > > > > > > > > > > > > For "decode_error_mode_ == kWithErrors", the actual state of the frame > > is > > > > not > > > > > > checked ("IsContinuous" immediately returns "true" unless the error > mode > > > is > > > > > > ignored). That's why I added the second mode to "IsContinuous" that > > always > > > > > > checks the state of the frame (stored in "continuous_for_decoding_"). > > > > > > > > > > I'm trying to understand when and why it is necessary to call > IsContinuous > > > > with > > > > > ignore_error_mode = false. Do we need to go down that code path? > > > > > > > > > > > > > The problem is, that with ignore_error_mode = true the actual frame is > never > > > > checked if decoding with errors is allowed (which often is the case). This > > > > however is solved by this CL, so that with disabled NACKs (and decoding > with > > > > errors allowed), the frame contents are still checked and if no continuous > > > frame > > > > for decoding are received for too long, a keyframe can be requested. > > > > > > Yes, but doesn't that mean that we don't decode with errors any longer? That > > may > > > be the right decision, but then maybe the code also shouldn't have a > > > decode_error_mode_ mode? I think it would be fine to only decode frames that > > are > > > in sequence and complete, as long as we make sure we request a key frame if > we > > > have failed to decode for too long. That is what you are trying to do here, > > > right? > > > > Hmm. With the latest changes, decoding with errors indeed will be no longer > > working. That functionality was removed with patch 5 and the change to reuse > > existing code. Before I was still adding these frames to "decodable_frames_" > and > > manually measuring the time since the last continuous and decodable frame to > > detect when to request keyframes. > > Reusing "NonContinuousOrIncompleteDuration" required them to be stored in > > "incomplete_frames_" so they don't get decoded any more. > > > > I'm not sure if that's the way to go, now a keyframe will be requested either > > through the existing NACK code or through the new code if no frame was decoded > > for too long. > > > > > > > > > > > > > > > > > > The "else" path is taken when NACKs are disabled and/or the > > > > decode_error_mode_ > > > > > > is "kWithErrors" because in these cases either missing packets won't > be > > > > > reported > > > > > > (NACKs disabled) or "IsContinuous" didn't already check the state of > the > > > > frame > > > > > > (decode_error_mode_ == kWithErrors). > > > > > > > > > > So continuous_for_decoding means that the frame is complete (all packets > > > > > received) and in sequence (i.e., continuous) if we have nack and not > > > decoding > > > > > with errors. And if we are decoding with errors it means the frame is in > > > > > sequence (continuous) but not necessarily that it is complete? > > > > > > > > > > Do we need to check both continuous and continuous_for_decoding at lines > > 806 > > > > and > > > > > 812, or is continuous_for_decoding enough? > > > > > > > > Line 806 can be changed (verified with the Jitter and EndToEnd tests). > > Thanks > > > > for spotting this. > > > > > > > > Changing line 812 causes this test to fail: > > > > [ RUN ] TestRunningJitterBuffer.Full > > > > > > ../../webrtc/modules/video_coding/main/source/jitter_buffer_unittest.cc:2139: > > > > Failure > > > > Expected: (InsertFrames(kMaxNumberOfFrames, kVideoFrameDelta)) >= > > (kNoError), > > > > actual: -3 vs 0 > > > > [ FAILED ] TestRunningJitterBuffer.Full (2 ms) > > > > > > > > This is because more than 10 seconds (i.e. "kMaxDiscontinuousFramesTime") > of > > > > frames are added and so a keyframe is requested. All other tests (Jitter > and > > > > EndToEnd) are running without error. > > > > > > > > What do you think, should this still be changed and the test adjusted > > (how?), > > > or > > > > should I keep the condition as is right now? > > > > > > > Right. What behavior is it that you want? We typically don't use this mode and > would be OK with only decoding frames without error, but making sure we send > PLI/key frame requests often when there is packet loss. > > If you however do want to decode with errors your previous version is probably > more correct... Sorry in that case if I led you down the wrong path. > > If we decide not to decode with errors, I think we should clean that code up > too. I don't have any preference about decoding with/without errors. Both use cases have their validity (somewhat movement with some errors vs. fewer but clear frames), so I leave it up to you as owner to decide - I'm just some external dev ;-) My initial goal was to change as little existing functionality as possible.
https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right): https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/jitter_buffer.cc:783: continuous_for_decoding = IsContinuous(*frame, true); On 2015/11/18 23:46:07, joachim wrote: > On 2015/11/18 20:51:57, stefan-webrtc (holmer) wrote: > > On 2015/11/18 00:50:26, joachim wrote: > > > On 2015/11/18 00:35:14, joachim wrote: > > > > On 2015/11/18 00:02:17, stefan-webrtc (holmer) wrote: > > > > > On 2015/11/16 21:58:11, joachim wrote: > > > > > > On 2015/11/16 17:52:15, stefan-webrtc (holmer) wrote: > > > > > > > Does this mean we require frames to be complete also for > > > > decode_error_mode_ > > > > > == > > > > > > > kWithErrors? It's not entirely clear to me in what situations we get > > in > > > to > > > > > > this > > > > > > > "else", and why it should ignore the error mode. Do we even need an > > > error > > > > > mode > > > > > > > check in IsContinuous? > > > > > > > > > > > > For "decode_error_mode_ == kWithErrors", the actual state of the frame > > is > > > > not > > > > > > checked ("IsContinuous" immediately returns "true" unless the error > mode > > > is > > > > > > ignored). That's why I added the second mode to "IsContinuous" that > > always > > > > > > checks the state of the frame (stored in "continuous_for_decoding_"). > > > > > > > > > > I'm trying to understand when and why it is necessary to call > IsContinuous > > > > with > > > > > ignore_error_mode = false. Do we need to go down that code path? > > > > > > > > > > > > > The problem is, that with ignore_error_mode = true the actual frame is > never > > > > checked if decoding with errors is allowed (which often is the case). This > > > > however is solved by this CL, so that with disabled NACKs (and decoding > with > > > > errors allowed), the frame contents are still checked and if no continuous > > > frame > > > > for decoding are received for too long, a keyframe can be requested. > > > > > > Btw, I did some more testing and you are right: if I change both (always > > > checking with ignore_error_mode = true and also updating the conditions > > below), > > > all Jitter/EndToEnd tests but the "TestRunningJitterBuffer.Full" are running > > > without errors. > > > > > > However in the (I think) common case (NACKs enabled and decoding with errors > > > allowed), this will always check the frame contents. > > > > I'd say the common case is NACKs enabled and decoding with errors not allowed. > > Acknowledged. > > > > > > > > > > > > > > > > The "else" path is taken when NACKs are disabled and/or the > > > > decode_error_mode_ > > > > > > is "kWithErrors" because in these cases either missing packets won't > be > > > > > reported > > > > > > (NACKs disabled) or "IsContinuous" didn't already check the state of > the > > > > frame > > > > > > (decode_error_mode_ == kWithErrors). > > > > > > > > > > So continuous_for_decoding means that the frame is complete (all packets > > > > > received) and in sequence (i.e., continuous) if we have nack and not > > > decoding > > > > > with errors. And if we are decoding with errors it means the frame is in > > > > > sequence (continuous) but not necessarily that it is complete? > > > > > > > > > > Do we need to check both continuous and continuous_for_decoding at lines > > 806 > > > > and > > > > > 812, or is continuous_for_decoding enough? > > > > > > > > Line 806 can be changed (verified with the Jitter and EndToEnd tests). > > Thanks > > > > for spotting this. > > > > > > > > Changing line 812 causes this test to fail: > > > > [ RUN ] TestRunningJitterBuffer.Full > > > > > > ../../webrtc/modules/video_coding/main/source/jitter_buffer_unittest.cc:2139: > > > > Failure > > > > Expected: (InsertFrames(kMaxNumberOfFrames, kVideoFrameDelta)) >= > > (kNoError), > > > > actual: -3 vs 0 > > > > [ FAILED ] TestRunningJitterBuffer.Full (2 ms) > > > > > > > > This is because more than 10 seconds (i.e. "kMaxDiscontinuousFramesTime") > of > > > > frames are added and so a keyframe is requested. All other tests (Jitter > and > > > > EndToEnd) are running without error. > > > > Right, that seems like correct behavior. You may want to change the test to do > > the DropFrame(1) at the end, so that the non-decodable frames time is shorter, > > and then simply verify that it's getting full and causes a kFlushIndicator. > > Sorry, I don't get that. If DropFrame(1) is moved after InsertFrames(kMax..), > the frames are not added to "incomplete_frames_", the new code doesn't trigger > and the frames are decodable afterwards (causing the rest of the test to fail). I see, maybe if you increase the framerate (decrease the time between frames) you can verify the behavior if the buffer gets full without triggering the new functionality. > > > > > > > > > What do you think, should this still be changed and the test adjusted > > (how?), > > > or > > > > should I keep the condition as is right now? > > >
https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right): https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/jitter_buffer.cc:783: continuous_for_decoding = IsContinuous(*frame, true); On 2015/11/18 23:46:07, joachim wrote: > On 2015/11/18 20:51:57, stefan-webrtc (holmer) wrote: > > On 2015/11/18 00:35:14, joachim wrote: > > > On 2015/11/18 00:02:17, stefan-webrtc (holmer) wrote: > > > > On 2015/11/16 21:58:11, joachim wrote: > > > > > On 2015/11/16 17:52:15, stefan-webrtc (holmer) wrote: > > > > > > Does this mean we require frames to be complete also for > > > decode_error_mode_ > > > > == > > > > > > kWithErrors? It's not entirely clear to me in what situations we get > in > > to > > > > > this > > > > > > "else", and why it should ignore the error mode. Do we even need an > > error > > > > mode > > > > > > check in IsContinuous? > > > > > > > > > > For "decode_error_mode_ == kWithErrors", the actual state of the frame > is > > > not > > > > > checked ("IsContinuous" immediately returns "true" unless the error mode > > is > > > > > ignored). That's why I added the second mode to "IsContinuous" that > always > > > > > checks the state of the frame (stored in "continuous_for_decoding_"). > > > > > > > > I'm trying to understand when and why it is necessary to call IsContinuous > > > with > > > > ignore_error_mode = false. Do we need to go down that code path? > > > > > > > > > > The problem is, that with ignore_error_mode = true the actual frame is never > > > checked if decoding with errors is allowed (which often is the case). This > > > however is solved by this CL, so that with disabled NACKs (and decoding with > > > errors allowed), the frame contents are still checked and if no continuous > > frame > > > for decoding are received for too long, a keyframe can be requested. > > > > Yes, but doesn't that mean that we don't decode with errors any longer? That > may > > be the right decision, but then maybe the code also shouldn't have a > > decode_error_mode_ mode? I think it would be fine to only decode frames that > are > > in sequence and complete, as long as we make sure we request a key frame if we > > have failed to decode for too long. That is what you are trying to do here, > > right? > > Hmm. With the latest changes, decoding with errors indeed will be no longer > working. That functionality was removed with patch 5 and the change to reuse > existing code. Before I was still adding these frames to "decodable_frames_" and > manually measuring the time since the last continuous and decodable frame to > detect when to request keyframes. > Reusing "NonContinuousOrIncompleteDuration" required them to be stored in > "incomplete_frames_" so they don't get decoded any more. > > I'm not sure if that's the way to go, now a keyframe will be requested either > through the existing NACK code or through the new code if no frame was decoded > for too long. > > > > > > > > > > > > > > The "else" path is taken when NACKs are disabled and/or the > > > decode_error_mode_ > > > > > is "kWithErrors" because in these cases either missing packets won't be > > > > reported > > > > > (NACKs disabled) or "IsContinuous" didn't already check the state of the > > > frame > > > > > (decode_error_mode_ == kWithErrors). > > > > > > > > So continuous_for_decoding means that the frame is complete (all packets > > > > received) and in sequence (i.e., continuous) if we have nack and not > > decoding > > > > with errors. And if we are decoding with errors it means the frame is in > > > > sequence (continuous) but not necessarily that it is complete? > > > > > > > > Do we need to check both continuous and continuous_for_decoding at lines > 806 > > > and > > > > 812, or is continuous_for_decoding enough? > > > > > > Line 806 can be changed (verified with the Jitter and EndToEnd tests). > Thanks > > > for spotting this. > > > > > > Changing line 812 causes this test to fail: > > > [ RUN ] TestRunningJitterBuffer.Full > > > > ../../webrtc/modules/video_coding/main/source/jitter_buffer_unittest.cc:2139: > > > Failure > > > Expected: (InsertFrames(kMaxNumberOfFrames, kVideoFrameDelta)) >= > (kNoError), > > > actual: -3 vs 0 > > > [ FAILED ] TestRunningJitterBuffer.Full (2 ms) > > > > > > This is because more than 10 seconds (i.e. "kMaxDiscontinuousFramesTime") of > > > frames are added and so a keyframe is requested. All other tests (Jitter and > > > EndToEnd) are running without error. > > > > > > What do you think, should this still be changed and the test adjusted > (how?), > > or > > > should I keep the condition as is right now? > > > OK, in that case I suggest that we clean up the "decode with errors = true" pieces and remove the ability to decode with errors, and instead freeze the video while waiting for the key frame. Maybe it would make sense to reduce the kMaxDiscontinuousFramesTime when NACK isn't enabled too?
On 2015/11/19 00:06:54, stefan-webrtc (holmer) wrote: > https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... > File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right): > > https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... > webrtc/modules/video_coding/main/source/jitter_buffer.cc:783: > continuous_for_decoding = IsContinuous(*frame, true); > On 2015/11/18 23:46:07, joachim wrote: > > On 2015/11/18 20:51:57, stefan-webrtc (holmer) wrote: > > > On 2015/11/18 00:35:14, joachim wrote: > > > > On 2015/11/18 00:02:17, stefan-webrtc (holmer) wrote: > > > > > On 2015/11/16 21:58:11, joachim wrote: > > > > > > On 2015/11/16 17:52:15, stefan-webrtc (holmer) wrote: > > > > > > > Does this mean we require frames to be complete also for > > > > decode_error_mode_ > > > > > == > > > > > > > kWithErrors? It's not entirely clear to me in what situations we get > > in > > > to > > > > > > this > > > > > > > "else", and why it should ignore the error mode. Do we even need an > > > error > > > > > mode > > > > > > > check in IsContinuous? > > > > > > > > > > > > For "decode_error_mode_ == kWithErrors", the actual state of the frame > > is > > > > not > > > > > > checked ("IsContinuous" immediately returns "true" unless the error > mode > > > is > > > > > > ignored). That's why I added the second mode to "IsContinuous" that > > always > > > > > > checks the state of the frame (stored in "continuous_for_decoding_"). > > > > > > > > > > I'm trying to understand when and why it is necessary to call > IsContinuous > > > > with > > > > > ignore_error_mode = false. Do we need to go down that code path? > > > > > > > > > > > > > The problem is, that with ignore_error_mode = true the actual frame is > never > > > > checked if decoding with errors is allowed (which often is the case). This > > > > however is solved by this CL, so that with disabled NACKs (and decoding > with > > > > errors allowed), the frame contents are still checked and if no continuous > > > frame > > > > for decoding are received for too long, a keyframe can be requested. > > > > > > Yes, but doesn't that mean that we don't decode with errors any longer? That > > may > > > be the right decision, but then maybe the code also shouldn't have a > > > decode_error_mode_ mode? I think it would be fine to only decode frames that > > are > > > in sequence and complete, as long as we make sure we request a key frame if > we > > > have failed to decode for too long. That is what you are trying to do here, > > > right? > > > > Hmm. With the latest changes, decoding with errors indeed will be no longer > > working. That functionality was removed with patch 5 and the change to reuse > > existing code. Before I was still adding these frames to "decodable_frames_" > and > > manually measuring the time since the last continuous and decodable frame to > > detect when to request keyframes. > > Reusing "NonContinuousOrIncompleteDuration" required them to be stored in > > "incomplete_frames_" so they don't get decoded any more. > > > > I'm not sure if that's the way to go, now a keyframe will be requested either > > through the existing NACK code or through the new code if no frame was decoded > > for too long. > > > > > > > > > > > > > > > > > > The "else" path is taken when NACKs are disabled and/or the > > > > decode_error_mode_ > > > > > > is "kWithErrors" because in these cases either missing packets won't > be > > > > > reported > > > > > > (NACKs disabled) or "IsContinuous" didn't already check the state of > the > > > > frame > > > > > > (decode_error_mode_ == kWithErrors). > > > > > > > > > > So continuous_for_decoding means that the frame is complete (all packets > > > > > received) and in sequence (i.e., continuous) if we have nack and not > > > decoding > > > > > with errors. And if we are decoding with errors it means the frame is in > > > > > sequence (continuous) but not necessarily that it is complete? > > > > > > > > > > Do we need to check both continuous and continuous_for_decoding at lines > > 806 > > > > and > > > > > 812, or is continuous_for_decoding enough? > > > > > > > > Line 806 can be changed (verified with the Jitter and EndToEnd tests). > > Thanks > > > > for spotting this. > > > > > > > > Changing line 812 causes this test to fail: > > > > [ RUN ] TestRunningJitterBuffer.Full > > > > > > ../../webrtc/modules/video_coding/main/source/jitter_buffer_unittest.cc:2139: > > > > Failure > > > > Expected: (InsertFrames(kMaxNumberOfFrames, kVideoFrameDelta)) >= > > (kNoError), > > > > actual: -3 vs 0 > > > > [ FAILED ] TestRunningJitterBuffer.Full (2 ms) > > > > > > > > This is because more than 10 seconds (i.e. "kMaxDiscontinuousFramesTime") > of > > > > frames are added and so a keyframe is requested. All other tests (Jitter > and > > > > EndToEnd) are running without error. > > > > > > > > What do you think, should this still be changed and the test adjusted > > (how?), > > > or > > > > should I keep the condition as is right now? > > > > > > > OK, in that case I suggest that we clean up the "decode with errors = true" > pieces and remove the ability to decode with errors, and instead freeze the > video while waiting for the key frame. Maybe it would make sense to reduce the > kMaxDiscontinuousFramesTime when NACK isn't enabled too? All right - just to be sure: I only change that in the code I'm touching with this CL, not anywhere else where the decode_error_mode_ might be used, right? Otherwise it will be various other changes in webrtc/modules/video_coding/ and webrtc/video_engine. Btw it looks like decoding with errors is automatically enabled if NACK gets disabled in ViEChannel::ProcessNACKRequest and VideoReceiver::SetVideoProtection. Reducing kMaxDiscontinuousFramesTime when NACK is not enabled sounds good, any suggestions? 5 seconds instead of 10?
On 2015/11/19 00:17:28, joachim wrote: > On 2015/11/19 00:06:54, stefan-webrtc (holmer) wrote: > > > https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... > > File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right): > > > > > https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... > > webrtc/modules/video_coding/main/source/jitter_buffer.cc:783: > > continuous_for_decoding = IsContinuous(*frame, true); > > On 2015/11/18 23:46:07, joachim wrote: > > > On 2015/11/18 20:51:57, stefan-webrtc (holmer) wrote: > > > > On 2015/11/18 00:35:14, joachim wrote: > > > > > On 2015/11/18 00:02:17, stefan-webrtc (holmer) wrote: > > > > > > On 2015/11/16 21:58:11, joachim wrote: > > > > > > > On 2015/11/16 17:52:15, stefan-webrtc (holmer) wrote: > > > > > > > > Does this mean we require frames to be complete also for > > > > > decode_error_mode_ > > > > > > == > > > > > > > > kWithErrors? It's not entirely clear to me in what situations we > get > > > in > > > > to > > > > > > > this > > > > > > > > "else", and why it should ignore the error mode. Do we even need > an > > > > error > > > > > > mode > > > > > > > > check in IsContinuous? > > > > > > > > > > > > > > For "decode_error_mode_ == kWithErrors", the actual state of the > frame > > > is > > > > > not > > > > > > > checked ("IsContinuous" immediately returns "true" unless the error > > mode > > > > is > > > > > > > ignored). That's why I added the second mode to "IsContinuous" that > > > always > > > > > > > checks the state of the frame (stored in > "continuous_for_decoding_"). > > > > > > > > > > > > I'm trying to understand when and why it is necessary to call > > IsContinuous > > > > > with > > > > > > ignore_error_mode = false. Do we need to go down that code path? > > > > > > > > > > > > > > > > The problem is, that with ignore_error_mode = true the actual frame is > > never > > > > > checked if decoding with errors is allowed (which often is the case). > This > > > > > however is solved by this CL, so that with disabled NACKs (and decoding > > with > > > > > errors allowed), the frame contents are still checked and if no > continuous > > > > frame > > > > > for decoding are received for too long, a keyframe can be requested. > > > > > > > > Yes, but doesn't that mean that we don't decode with errors any longer? > That > > > may > > > > be the right decision, but then maybe the code also shouldn't have a > > > > decode_error_mode_ mode? I think it would be fine to only decode frames > that > > > are > > > > in sequence and complete, as long as we make sure we request a key frame > if > > we > > > > have failed to decode for too long. That is what you are trying to do > here, > > > > right? > > > > > > Hmm. With the latest changes, decoding with errors indeed will be no longer > > > working. That functionality was removed with patch 5 and the change to reuse > > > existing code. Before I was still adding these frames to "decodable_frames_" > > and > > > manually measuring the time since the last continuous and decodable frame to > > > detect when to request keyframes. > > > Reusing "NonContinuousOrIncompleteDuration" required them to be stored in > > > "incomplete_frames_" so they don't get decoded any more. > > > > > > I'm not sure if that's the way to go, now a keyframe will be requested > either > > > through the existing NACK code or through the new code if no frame was > decoded > > > for too long. > > > > > > > > > > > > > > > > > > > > > > The "else" path is taken when NACKs are disabled and/or the > > > > > decode_error_mode_ > > > > > > > is "kWithErrors" because in these cases either missing packets won't > > be > > > > > > reported > > > > > > > (NACKs disabled) or "IsContinuous" didn't already check the state of > > the > > > > > frame > > > > > > > (decode_error_mode_ == kWithErrors). > > > > > > > > > > > > So continuous_for_decoding means that the frame is complete (all > packets > > > > > > received) and in sequence (i.e., continuous) if we have nack and not > > > > decoding > > > > > > with errors. And if we are decoding with errors it means the frame is > in > > > > > > sequence (continuous) but not necessarily that it is complete? > > > > > > > > > > > > Do we need to check both continuous and continuous_for_decoding at > lines > > > 806 > > > > > and > > > > > > 812, or is continuous_for_decoding enough? > > > > > > > > > > Line 806 can be changed (verified with the Jitter and EndToEnd tests). > > > Thanks > > > > > for spotting this. > > > > > > > > > > Changing line 812 causes this test to fail: > > > > > [ RUN ] TestRunningJitterBuffer.Full > > > > > > > > > ../../webrtc/modules/video_coding/main/source/jitter_buffer_unittest.cc:2139: > > > > > Failure > > > > > Expected: (InsertFrames(kMaxNumberOfFrames, kVideoFrameDelta)) >= > > > (kNoError), > > > > > actual: -3 vs 0 > > > > > [ FAILED ] TestRunningJitterBuffer.Full (2 ms) > > > > > > > > > > This is because more than 10 seconds (i.e. > "kMaxDiscontinuousFramesTime") > > of > > > > > frames are added and so a keyframe is requested. All other tests (Jitter > > and > > > > > EndToEnd) are running without error. > > > > > > > > > > What do you think, should this still be changed and the test adjusted > > > (how?), > > > > or > > > > > should I keep the condition as is right now? > > > > > > > > > > > OK, in that case I suggest that we clean up the "decode with errors = true" > > pieces and remove the ability to decode with errors, and instead freeze the > > video while waiting for the key frame. Maybe it would make sense to reduce the > > kMaxDiscontinuousFramesTime when NACK isn't enabled too? > > All right - just to be sure: I only change that in the code I'm touching with > this CL, not anywhere else where the decode_error_mode_ might be used, right? Sounds good. > Otherwise it will be various other changes in webrtc/modules/video_coding/ and > webrtc/video_engine. Btw it looks like decoding with errors is automatically > enabled if NACK gets disabled in ViEChannel::ProcessNACKRequest and > VideoReceiver::SetVideoProtection. Ok, I think it's ok to change that behavior. > > Reducing kMaxDiscontinuousFramesTime when NACK is not enabled sounds good, any > suggestions? 5 seconds instead of 10? Let's do 1 second.
https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... File webrtc/modules/video_coding/main/source/jitter_buffer.cc (right): https://codereview.webrtc.org/1211873004/diff/100001/webrtc/modules/video_cod... webrtc/modules/video_coding/main/source/jitter_buffer.cc:783: continuous_for_decoding = IsContinuous(*frame, true); On 2015/11/19 00:04:17, stefan-webrtc (holmer) wrote: > On 2015/11/18 23:46:07, joachim wrote: > > On 2015/11/18 20:51:57, stefan-webrtc (holmer) wrote: > > > On 2015/11/18 00:50:26, joachim wrote: > > > > On 2015/11/18 00:35:14, joachim wrote: > > > > > On 2015/11/18 00:02:17, stefan-webrtc (holmer) wrote: > > > > > > On 2015/11/16 21:58:11, joachim wrote: > > > > > > > On 2015/11/16 17:52:15, stefan-webrtc (holmer) wrote: > > > > > > > > Does this mean we require frames to be complete also for > > > > > decode_error_mode_ > > > > > > == > > > > > > > > kWithErrors? It's not entirely clear to me in what situations we > get > > > in > > > > to > > > > > > > this > > > > > > > > "else", and why it should ignore the error mode. Do we even need > an > > > > error > > > > > > mode > > > > > > > > check in IsContinuous? > > > > > > > > > > > > > > For "decode_error_mode_ == kWithErrors", the actual state of the > frame > > > is > > > > > not > > > > > > > checked ("IsContinuous" immediately returns "true" unless the error > > mode > > > > is > > > > > > > ignored). That's why I added the second mode to "IsContinuous" that > > > always > > > > > > > checks the state of the frame (stored in > "continuous_for_decoding_"). > > > > > > > > > > > > I'm trying to understand when and why it is necessary to call > > IsContinuous > > > > > with > > > > > > ignore_error_mode = false. Do we need to go down that code path? > > > > > > > > > > > > > > > > The problem is, that with ignore_error_mode = true the actual frame is > > never > > > > > checked if decoding with errors is allowed (which often is the case). > This > > > > > however is solved by this CL, so that with disabled NACKs (and decoding > > with > > > > > errors allowed), the frame contents are still checked and if no > continuous > > > > frame > > > > > for decoding are received for too long, a keyframe can be requested. > > > > > > > > Btw, I did some more testing and you are right: if I change both (always > > > > checking with ignore_error_mode = true and also updating the conditions > > > below), > > > > all Jitter/EndToEnd tests but the "TestRunningJitterBuffer.Full" are > running > > > > without errors. > > > > > > > > However in the (I think) common case (NACKs enabled and decoding with > errors > > > > allowed), this will always check the frame contents. > > > > > > I'd say the common case is NACKs enabled and decoding with errors not > allowed. > > > > Acknowledged. > > > > > > > > > > > > > > > > > > > > The "else" path is taken when NACKs are disabled and/or the > > > > > decode_error_mode_ > > > > > > > is "kWithErrors" because in these cases either missing packets won't > > be > > > > > > reported > > > > > > > (NACKs disabled) or "IsContinuous" didn't already check the state of > > the > > > > > frame > > > > > > > (decode_error_mode_ == kWithErrors). > > > > > > > > > > > > So continuous_for_decoding means that the frame is complete (all > packets > > > > > > received) and in sequence (i.e., continuous) if we have nack and not > > > > decoding > > > > > > with errors. And if we are decoding with errors it means the frame is > in > > > > > > sequence (continuous) but not necessarily that it is complete? > > > > > > > > > > > > Do we need to check both continuous and continuous_for_decoding at > lines > > > 806 > > > > > and > > > > > > 812, or is continuous_for_decoding enough? > > > > > > > > > > Line 806 can be changed (verified with the Jitter and EndToEnd tests). > > > Thanks > > > > > for spotting this. > > > > > > > > > > Changing line 812 causes this test to fail: > > > > > [ RUN ] TestRunningJitterBuffer.Full > > > > > > > > > ../../webrtc/modules/video_coding/main/source/jitter_buffer_unittest.cc:2139: > > > > > Failure > > > > > Expected: (InsertFrames(kMaxNumberOfFrames, kVideoFrameDelta)) >= > > > (kNoError), > > > > > actual: -3 vs 0 > > > > > [ FAILED ] TestRunningJitterBuffer.Full (2 ms) > > > > > > > > > > This is because more than 10 seconds (i.e. > "kMaxDiscontinuousFramesTime") > > of > > > > > frames are added and so a keyframe is requested. All other tests (Jitter > > and > > > > > EndToEnd) are running without error. > > > > > > Right, that seems like correct behavior. You may want to change the test to > do > > > the DropFrame(1) at the end, so that the non-decodable frames time is > shorter, > > > and then simply verify that it's getting full and causes a kFlushIndicator. > > > > Sorry, I don't get that. If DropFrame(1) is moved after InsertFrames(kMax..), > > the frames are not added to "incomplete_frames_", the new code doesn't trigger > > and the frames are decodable afterwards (causing the rest of the test to > fail). > > > I see, maybe if you increase the framerate (decrease the time between frames) > you can verify the behavior if the buffer gets full without triggering the new > functionality. Another possibility would be to enable NACK for this test, so the new codepath is not executed and the jitter can be filled without requesting keyframes. > > > > > > > > > > > > What do you think, should this still be changed and the test adjusted > > > (how?), > > > > or > > > > > should I keep the condition as is right now? > > > > >
@stefan: is that what you had in mind?
Description was changed from ========== Request keyframe if packets are missing and NACK is disabled. This allows enabling "EndToEndTest.ReceivesPliAndRecoversWithoutNack". BUG=webrtc:2250 ========== to ========== Request keyframe if too many packets are missing and NACK is disabled. This allows enabling "EndToEndTest.ReceivesPliAndRecoversWithoutNack". BUG=webrtc:2250 ==========
Yes, I like this. lgtm!
The CQ bit was checked by stefan@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1211873004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1211873004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios64_sim_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/3378) ios_arm64_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/5690) ios_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/10842) ios_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/9490) mac_asan on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/10686) mac_compile_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...) mac_x64_gn_dbg on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_gn_dbg/builds/5347) mac_x64_gn_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_gn_rel/builds/5293) mac_x64_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_rel/builds/10565)
On 2015/11/19 02:26:36, stefan-webrtc (holmer) wrote: > Yes, I like this. > > lgtm! You may have to rebase before we can commit this.
The CQ bit was checked by jbauch@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1211873004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1211873004/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_rel on tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/9131)
I updated the condition for fetching frames from "incomplete_frames_" (a "complete" frame is also "decodable"). This should fix the failures (will trigger CQ dry run to verify). @Stefan: ptal
The CQ bit was checked by jbauch@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1211873004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1211873004/180001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by stefan@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/1211873004/#ps180001 (title: "Updated condition for fetching from incomplete_frames_.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1211873004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1211873004/180001
On 2015/11/23 08:25:51, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1211873004/180001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/1211873004/180001 Is "EndToEndTest.ReceivesTransportFeedback" flaky? It seems to hang here and also timed out in https://build.chromium.org/p/tryserver.webrtc/builders/win_drmemory_light/bui... and https://build.chromium.org/p/tryserver.webrtc/builders/win_drmemory_light/bui... (got cancelled) and https://build.chromium.org/p/tryserver.webrtc/builders/win_drmemory_light/bui...
On 2015/11/23 10:31:28, joachim wrote: > On 2015/11/23 08:25:51, commit-bot: I haz the power wrote: > > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/patch-status/1211873004/180001 > > View timeline at > > https://chromium-cq-status.appspot.com/patch-timeline/1211873004/180001 > > Is "EndToEndTest.ReceivesTransportFeedback" flaky? It seems to hang here and > also timed out in > https://build.chromium.org/p/tryserver.webrtc/builders/win_drmemory_light/bui... > and > https://build.chromium.org/p/tryserver.webrtc/builders/win_drmemory_light/bui... > (got cancelled) and > https://build.chromium.org/p/tryserver.webrtc/builders/win_drmemory_light/bui... Seems to be. I'll look into that, shouldn't block this.
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/db81ffd6f50d487441555947ec63ccc766e75043 Cr-Commit-Position: refs/heads/master@{#10747} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
