Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2844643002:
Add warning about timestamp non-monotonicity in the frame buffer. (Closed)
DescriptionAdd warning about timestamp non-monotonicity in the frame buffer.
BUG=None
Review-Url: https://codereview.webrtc.org/2844643002
Cr-Commit-Position: refs/heads/master@{#17910}
Committed: https://chromium.googlesource.com/external/webrtc/+/9078d8cf05f6307db640509ab9950592d3956e20
Patch Set 1 #
Total comments: 6
Patch Set 2 : philipel comments 1. #
Total comments: 3
Patch Set 3 : Rebase. #Patch Set 4 : philipel comments 1. #
Total comments: 2
Patch Set 5 : Reuse |next_frame_it_->first|. #Patch Set 6 : Nits. #Patch Set 7 : Log before we advance |last_decoded_frame_it_|. #Messages
Total messages: 26 (8 generated)
brandtr@webrtc.org changed reviewers: + philipel@webrtc.org, stefan@webrtc.org
Please take a look. This log message is purely to help with debugging. Any changes to the actual logic is out-of-scope for this CL.
I'm fine with doing logical changes in a separate CL, but maybe we can discuss it :) https://codereview.webrtc.org/2844643002/diff/1/webrtc/modules/video_coding/f... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/2844643002/diff/1/webrtc/modules/video_coding/f... webrtc/modules/video_coding/frame_buffer2.cc:156: << ")."; Should we drop the frame if this is the case, or is it right to decode it, hoping that the timestamp has jumped? Maybe the best thing to do is to reset the jitter buffer?
On 2017/04/26 09:04:58, stefan-webrtc wrote: > I'm fine with doing logical changes in a separate CL, but maybe we can discuss > it :) I'm fine with discussing possible logical changes, but my point is that this CL would be useful, as is, when debugging distortion reports. After this CL has landed, the existence or lack of the log message in future distortions reports could then inform our discussions on possible changing the the logic.
https://codereview.webrtc.org/2844643002/diff/1/webrtc/modules/video_coding/f... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/2844643002/diff/1/webrtc/modules/video_coding/f... webrtc/modules/video_coding/frame_buffer2.cc:156: << ")."; On 2017/04/26 09:04:57, stefan-webrtc wrote: > Should we drop the frame if this is the case, or is it right to decode it, > hoping that the timestamp has jumped? Maybe the best thing to do is to reset the > jitter buffer? In order to avoid distortions due to incorrect decode order, it might be safest to reset the jitter buffer. This basically means trusting RTP timestamps over picture_id, when it comes to the final order check. My rationale is that the RTP timestamp order should correlate perfectly with the encode order, and therefore the decode order, since we don't (?) support B-frames (*). This might be a naive view of the system however, so if you know more I would be interested in learning. Also, of course, this assumes a sane sender, which does not generate random jumps in RTP timestamps... (*) Looking through the different packetization RFCs, I noticed that H.264 has a "Decoding Order Number" in some particular packetization mode. No idea if this is something that we support?. https://tools.ietf.org/html/rfc6184#section-5.5
lgtm
https://codereview.webrtc.org/2844643002/diff/1/webrtc/modules/video_coding/f... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/2844643002/diff/1/webrtc/modules/video_coding/f... webrtc/modules/video_coding/frame_buffer2.cc:156: << ")."; On 2017/04/26 09:43:51, brandtr wrote: > On 2017/04/26 09:04:57, stefan-webrtc wrote: > > Should we drop the frame if this is the case, or is it right to decode it, > > hoping that the timestamp has jumped? Maybe the best thing to do is to reset > the > > jitter buffer? > > In order to avoid distortions due to incorrect decode order, it might be safest > to reset the jitter buffer. This basically means trusting RTP timestamps over > picture_id, when it comes to the final order check. My rationale is that the RTP > timestamp order should correlate perfectly with the encode order, and therefore > the decode order, since we don't (?) support B-frames (*). This might be a naive > view of the system however, so if you know more I would be interested in > learning. Also, of course, this assumes a sane sender, which does not generate > random jumps in RTP timestamps... > > (*) Looking through the different packetization RFCs, I noticed that H.264 has a > "Decoding Order Number" in some particular packetization mode. No idea if this > is something that we support?. > https://tools.ietf.org/html/rfc6184#section-5.5 We have no plans of supporting interleaved packetization, i.e., STAB-B and MTAP. I agree that making a reset when this happens is likely a good thing to do. WDYT Philip?
https://codereview.webrtc.org/2844643002/diff/1/webrtc/modules/video_coding/f... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/2844643002/diff/1/webrtc/modules/video_coding/f... webrtc/modules/video_coding/frame_buffer2.cc:156: << ")."; On 2017/04/26 10:44:49, stefan-webrtc wrote: > On 2017/04/26 09:43:51, brandtr wrote: > > On 2017/04/26 09:04:57, stefan-webrtc wrote: > > > Should we drop the frame if this is the case, or is it right to decode it, > > > hoping that the timestamp has jumped? Maybe the best thing to do is to reset > > the > > > jitter buffer? > > > > In order to avoid distortions due to incorrect decode order, it might be > safest > > to reset the jitter buffer. This basically means trusting RTP timestamps over > > picture_id, when it comes to the final order check. My rationale is that the > RTP > > timestamp order should correlate perfectly with the encode order, and > therefore > > the decode order, since we don't (?) support B-frames (*). This might be a > naive > > view of the system however, so if you know more I would be interested in > > learning. Also, of course, this assumes a sane sender, which does not generate > > random jumps in RTP timestamps... > > > > (*) Looking through the different packetization RFCs, I noticed that H.264 has > a > > "Decoding Order Number" in some particular packetization mode. No idea if this > > is something that we support?. > > https://tools.ietf.org/html/rfc6184#section-5.5 > > We have no plans of supporting interleaved packetization, i.e., STAB-B and MTAP. > > I agree that making a reset when this happens is likely a good thing to do. WDYT > Philip? I think a reset might be the right thing to do, not sure but I will think on it. https://codereview.webrtc.org/2844643002/diff/1/webrtc/modules/video_coding/f... File webrtc/modules/video_coding/frame_buffer2.h (right): https://codereview.webrtc.org/2844643002/diff/1/webrtc/modules/video_coding/f... webrtc/modules/video_coding/frame_buffer2.h:157: int last_decoded_frame_picture_id_ GUARDED_BY(crit_); Use |last_decoded_frame_it_->first.picture_id| instead.
https://codereview.webrtc.org/2844643002/diff/1/webrtc/modules/video_coding/f... File webrtc/modules/video_coding/frame_buffer2.h (right): https://codereview.webrtc.org/2844643002/diff/1/webrtc/modules/video_coding/f... webrtc/modules/video_coding/frame_buffer2.h:157: int last_decoded_frame_picture_id_ GUARDED_BY(crit_); On 2017/04/26 11:05:31, philipel wrote: > Use |last_decoded_frame_it_->first.picture_id| instead. Done. https://codereview.webrtc.org/2844643002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/2844643002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:148: AheadOrAt(last_decoded_frame_timestamp_, frame->timestamp)) { This is correct being >=, right? I don't think we ever want to send the same timestamp to the decoder more than once?
https://codereview.webrtc.org/2844643002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/2844643002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:148: AheadOrAt(last_decoded_frame_timestamp_, frame->timestamp)) { On 2017/04/26 11:26:33, brandtr wrote: > This is correct being >=, right? I don't think we ever want to send the same > timestamp to the decoder more than once? When we use SVC we will send frame with the same timestamp to the decoder. What you can do is to check if we send a frame with the same timestamp as the last frame and (last_decoded_frame_it_->first < key), then we know that this frame is just a higher spatial layer frame in a superframe.
Rebase.
https://codereview.webrtc.org/2844643002/diff/20001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/2844643002/diff/20001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:148: AheadOrAt(last_decoded_frame_timestamp_, frame->timestamp)) { On 2017/04/26 12:27:28, philipel wrote: > On 2017/04/26 11:26:33, brandtr wrote: > > This is correct being >=, right? I don't think we ever want to send the same > > timestamp to the decoder more than once? > > When we use SVC we will send frame with the same timestamp to the decoder. > > What you can do is to check if we send a frame with the same timestamp as the > last frame and (last_decoded_frame_it_->first < key), then we know that this > frame is just a higher spatial layer frame in a superframe. Right. What about this?
https://codereview.webrtc.org/2844643002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/2844643002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:150: if (last_decoded_frame_it_ != frames_.end()) { This should work, right? if (last_decoded_frame_it_ != frames_.end() && AheadOrAt(last_decoded_frame_timestamp_, frame->timestamp) && next_frame_it_->first <= last_decoded_frame_it_->first) { LOG(LS_WARNING) << "Frame with (timestamp:picture_id:spatial_id) (" << frame->timestamp << ":" << frame->picture_id << ":" << static_cast<int>(frame->spatial_layer) << ")" << " sent to decoder after frame with" << " (timestamp:picture_id:spatial_id) (" << last_decoded_frame_timestamp_ << ":" << last_decoded_frame_key.picture_id << ":" << static_cast<int>( last_decoded_frame_key.spatial_layer) << ")."; }
https://codereview.webrtc.org/2844643002/diff/60001/webrtc/modules/video_codi... File webrtc/modules/video_coding/frame_buffer2.cc (right): https://codereview.webrtc.org/2844643002/diff/60001/webrtc/modules/video_codi... webrtc/modules/video_coding/frame_buffer2.cc:150: if (last_decoded_frame_it_ != frames_.end()) { On 2017/04/26 15:26:37, philipel wrote: > This should work, right? > > if (last_decoded_frame_it_ != frames_.end() && > AheadOrAt(last_decoded_frame_timestamp_, frame->timestamp) && > next_frame_it_->first <= last_decoded_frame_it_->first) { > LOG(LS_WARNING) << "Frame with (timestamp:picture_id:spatial_id) (" > << frame->timestamp << ":" << frame->picture_id << ":" > << static_cast<int>(frame->spatial_layer) << ")" > << " sent to decoder after frame with" > << " (timestamp:picture_id:spatial_id) (" > << last_decoded_frame_timestamp_ << ":" > << last_decoded_frame_key.picture_id << ":" > << static_cast<int>( > last_decoded_frame_key.spatial_layer) > << ")."; > } I don't think that would catch this situation: * frame->timestamp <= last_decoded_frame_timestamp_ (RTP timestamp reordering) * next_frame_it_->first > last_decoded_frame_it_->first (PictureID correctly ordered) ? Note that on lines 156-157, I'm doing neither * last_decoded_frame_key < frame_key * last_decoded_frame_key <= frame_key, since I'm constraining the |picture_id| fields to be equal.
lgtm
The CQ bit was checked by brandtr@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org, philipel@webrtc.org Link to the patchset: https://codereview.webrtc.org/2844643002/#ps100001 (title: "Nits.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_msan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_msan/builds/19993)
The CQ bit was checked by brandtr@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2844643002/#ps120001 (title: "Log before we advance |last_decoded_frame_it_|.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1493300709690260,
"parent_rev": "ace5c8836d23dda287fe1232f94c7c3192408ddd", "commit_rev":
"9078d8cf05f6307db640509ab9950592d3956e20"}
Message was sent while issue was closed.
Description was changed from ========== Add warning about timestamp non-monotonicity in the frame buffer. BUG=None ========== to ========== Add warning about timestamp non-monotonicity in the frame buffer. BUG=None Review-Url: https://codereview.webrtc.org/2844643002 Cr-Commit-Position: refs/heads/master@{#17910} Committed: https://chromium.googlesource.com/external/webrtc/+/9078d8cf05f6307db640509ab... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/external/webrtc/+/9078d8cf05f6307db640509ab... |
