Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2993513002:
Fix off-by-one bugs in video_coding::PacketBuffer when the buffer is filled with a single frame. (Closed)
|
Created:
3 years, 4 months ago by philipel Modified:
3 years, 4 months ago Reviewers:
stefan-webrtc CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, zhengzhonghou_agora.io, video-team_agora.io, stefan-webrtc, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFix off-by-one bugs in video_coding::PacketBuffer when the buffer is filled with a single frame.
BUG=webrtc:8028
Review-Url: https://codereview.webrtc.org/2993513002
Cr-Commit-Position: refs/heads/master@{#19209}
Committed: https://chromium.googlesource.com/external/webrtc/+/ee13e8919c20de5860a510e91fac71fd5a7e9b8d
Patch Set 1 #
Total comments: 11
Patch Set 2 : Feedback #Patch Set 3 : Feedback #Patch Set 4 : const --> static constexpr #Patch Set 5 : new --> new[] #
Messages
Total messages: 23 (10 generated)
philipel@webrtc.org changed reviewers: + stefan@webrtc.org
https://codereview.webrtc.org/2993513002/diff/1/webrtc/modules/video_coding/p... File webrtc/modules/video_coding/packet_buffer.cc (left): https://codereview.webrtc.org/2993513002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.cc:260: // we will have at most |size_ - 1| packets left to check. Was this comment not correct? Looks like we have changed it to check size_ packets instead of size_ - 1. https://codereview.webrtc.org/2993513002/diff/1/webrtc/modules/video_coding/p... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/2993513002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.cc:356: data_buffer_[index].seqNum != seq_num) { What does this mean? That the frame doesn't have all packets? Should that happen here? https://codereview.webrtc.org/2993513002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.cc:362: return false; Should we log that the frame is too large for the decode buffer? Should that even be possible?
https://codereview.webrtc.org/2993513002/diff/1/webrtc/modules/video_coding/p... File webrtc/modules/video_coding/packet_buffer.cc (left): https://codereview.webrtc.org/2993513002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.cc:260: // we will have at most |size_ - 1| packets left to check. On 2017/08/01 11:24:17, stefan-webrtc wrote: > Was this comment not correct? Looks like we have changed it to check size_ > packets instead of size_ - 1. It was semi-correct. In this version |start_seq_num| was decremented at the end of the loop, and when we exited the loop due to the loop condition it was in the correct state. The problem was that we did not execute line 262-265 for the last (actually the first in frame) packet. This means the size of that packet was not added to the total size of the frame. https://codereview.webrtc.org/2993513002/diff/1/webrtc/modules/video_coding/p... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/2993513002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.cc:356: data_buffer_[index].seqNum != seq_num) { On 2017/08/01 11:24:17, stefan-webrtc wrote: > What does this mean? That the frame doesn't have all packets? Should that happen > here? It should not happen, no. I'll change it to a DCHECK. https://codereview.webrtc.org/2993513002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.cc:362: return false; On 2017/08/01 11:24:17, stefan-webrtc wrote: > Should we log that the frame is too large for the decode buffer? Should that > even be possible? This check is not really related to the bug at hand, it's just a check I added to avoid writing OOB, which can make many people sad.
https://codereview.webrtc.org/2993513002/diff/1/webrtc/modules/video_coding/p... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/2993513002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.cc:356: data_buffer_[index].seqNum != seq_num) { On 2017/08/01 11:51:23, philipel wrote: > On 2017/08/01 11:24:17, stefan-webrtc wrote: > > What does this mean? That the frame doesn't have all packets? Should that > happen > > here? > > It should not happen, no. I'll change it to a DCHECK. Is it safe to CHECK, or is it possible to trigger with an intentionally broken bitstream? What happens if it's triggered? Maybe we should have a DCHECK and catch it with an if statement? https://codereview.webrtc.org/2993513002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.cc:362: return false; On 2017/08/01 11:51:23, philipel wrote: > On 2017/08/01 11:24:17, stefan-webrtc wrote: > > Should we log that the frame is too large for the decode buffer? Should that > > even be possible? > > This check is not really related to the bug at hand, it's just a check I added > to avoid writing OOB, which can make many people sad. > Right, but it means that the frame is too large. We probably want to know that, so maybe we should log at least?
https://codereview.webrtc.org/2993513002/diff/1/webrtc/modules/video_coding/p... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/2993513002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.cc:356: data_buffer_[index].seqNum != seq_num) { On 2017/08/01 12:06:22, stefan-webrtc wrote: > On 2017/08/01 11:51:23, philipel wrote: > > On 2017/08/01 11:24:17, stefan-webrtc wrote: > > > What does this mean? That the frame doesn't have all packets? Should that > > happen > > > here? > > > > It should not happen, no. I'll change it to a DCHECK. > > Is it safe to CHECK, or is it possible to trigger with an intentionally broken > bitstream? What happens if it's triggered? Maybe we should have a DCHECK and > catch it with an if statement? It should not be possible to have the sequence_buffer_[index].seq_num != data_buffer_[index].seqNum. Changed it to a DCHECK. https://codereview.webrtc.org/2993513002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.cc:362: return false; On 2017/08/01 12:06:22, stefan-webrtc wrote: > On 2017/08/01 11:51:23, philipel wrote: > > On 2017/08/01 11:24:17, stefan-webrtc wrote: > > > Should we log that the frame is too large for the decode buffer? Should that > > > even be possible? > > > > This check is not really related to the bug at hand, it's just a check I added > > to avoid writing OOB, which can make many people sad. > > > > Right, but it means that the frame is too large. We probably want to know that, > so maybe we should log at least? Added warning if this happens.
lgtm https://codereview.webrtc.org/2993513002/diff/1/webrtc/modules/video_coding/p... File webrtc/modules/video_coding/packet_buffer.cc (right): https://codereview.webrtc.org/2993513002/diff/1/webrtc/modules/video_coding/p... webrtc/modules/video_coding/packet_buffer.cc:356: data_buffer_[index].seqNum != seq_num) { On 2017/08/01 13:00:05, philipel wrote: > On 2017/08/01 12:06:22, stefan-webrtc wrote: > > On 2017/08/01 11:51:23, philipel wrote: > > > On 2017/08/01 11:24:17, stefan-webrtc wrote: > > > > What does this mean? That the frame doesn't have all packets? Should that > > > happen > > > > here? > > > > > > It should not happen, no. I'll change it to a DCHECK. > > > > Is it safe to CHECK, or is it possible to trigger with an intentionally broken > > bitstream? What happens if it's triggered? Maybe we should have a DCHECK and > > catch it with an if statement? > > It should not be possible to have the sequence_buffer_[index].seq_num != > data_buffer_[index].seqNum. > > Changed it to a DCHECK. My question was if it should be a CHECK instead. But maybe not? Depends on how bad it would be if it actually happened... :)
The CQ bit was checked by philipel@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_x64_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_dbg/builds/11083)
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2993513002/#ps60001 (title: "const --> static constexpr")
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_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/26750)
The CQ bit was checked by philipel@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2993513002/#ps80001 (title: "new --> new[]")
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": 80001, "attempt_start_ts": 1501663405354640,
"parent_rev": "6f1f8e972df5226233d45fb46e72dc2ad28f52e6", "commit_rev":
"ee13e8919c20de5860a510e91fac71fd5a7e9b8d"}
Message was sent while issue was closed.
Description was changed from ========== Fix off-by-one bugs in video_coding::PacketBuffer when the buffer is filled with a single frame. BUG=webrtc:8028 ========== to ========== Fix off-by-one bugs in video_coding::PacketBuffer when the buffer is filled with a single frame. BUG=webrtc:8028 Review-Url: https://codereview.webrtc.org/2993513002 Cr-Commit-Position: refs/heads/master@{#19209} Committed: https://chromium.googlesource.com/external/webrtc/+/ee13e8919c20de5860a510e91... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/external/webrtc/+/ee13e8919c20de5860a510e91...
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2990183002/ by philipel@webrtc.org. The reason for reverting is: Break performance bots.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
