Don't detect a new frame if a previous packet is used in a previous frame.

Don't detect a new frame if a previous packet is used in a previous frame.

In this CL:
 - Removed unused variable |last_seq_num_|.
 - Fixed bug where a new incomplete frame was detected as a complete frame.
 - Added fuzzer to video_coding::PacketBuffer.

BUG=chromium:677101
Review-Url: https://codereview.webrtc.org/2613833003
Cr-Commit-Position: refs/heads/master@{#16003}
Committed: https://chromium.googlesource.com/external/webrtc/+/ea142f8de355c6b32c1f7a6d5715cde97a692502

[philipel, 2017-01-05 10:53:57]

philipel@webrtc.org changed reviewers:
+ kjellander@webrtc.org, stefan@webrtc.org

[kjellander_webrtc, 2017-01-09 12:08:16]

I'm not very familiar with the fuzzer code but I'll be happy to give a
rubberstamp for webrtc/test if stefan is happy. lgtm

[stefan-webrtc, 2017-01-10 11:50:00]

https://codereview.webrtc.org/2613833003/diff/1/webrtc/modules/video_coding/p...
File webrtc/modules/video_coding/packet_buffer.cc (right):

https://codereview.webrtc.org/2613833003/diff/1/webrtc/modules/video_coding/p...
webrtc/modules/video_coding/packet_buffer.cc:184: if
(sequence_buffer_[prev_index].frame_created)
It's not clear to me why this is needed. Why can't it be a new frame in this
case? Is it because the previous packet created a new frame, and this packet
doesn't have the frame_begin flag set?

I also don't follow how the test added would capture this... :)

https://codereview.webrtc.org/2613833003/diff/1/webrtc/modules/video_coding/v...
File webrtc/modules/video_coding/video_packet_buffer_unittest.cc (right):

https://codereview.webrtc.org/2613833003/diff/1/webrtc/modules/video_coding/v...
webrtc/modules/video_coding/video_packet_buffer_unittest.cc:467: Insert(3,
kKeyFrame, kNotFirst, kLast);
Doesn't this mean you have received the marker bit (kLast)?

Doesn't seem aligned with the name of the test.

https://codereview.webrtc.org/2613833003/diff/1/webrtc/modules/video_coding/v...
webrtc/modules/video_coding/video_packet_buffer_unittest.cc:469: ASSERT_EQ(0UL,
frames_from_callback_.size());
EXPECT_EQ

https://codereview.webrtc.org/2613833003/diff/1/webrtc/test/fuzzers/packet_bu...
File webrtc/test/fuzzers/packet_buffer_fuzzer.cc (right):

https://codereview.webrtc.org/2613833003/diff/1/webrtc/test/fuzzers/packet_bu...
webrtc/test/fuzzers/packet_buffer_fuzzer.cc:17: class DummyCallback : public
video_coding::OnReceivedFrameCallback {
NullCallback is usually the naming convention

https://codereview.webrtc.org/2613833003/diff/1/webrtc/test/fuzzers/packet_bu...
webrtc/test/fuzzers/packet_buffer_fuzzer.cc:26: if (size < 200) {
No need for this check, right?

[philipel, 2017-01-10 12:03:37]

https://codereview.webrtc.org/2613833003/diff/1/webrtc/modules/video_coding/p...
File webrtc/modules/video_coding/packet_buffer.cc (right):

https://codereview.webrtc.org/2613833003/diff/1/webrtc/modules/video_coding/p...
webrtc/modules/video_coding/packet_buffer.cc:184: if
(sequence_buffer_[prev_index].frame_created)
On 2017/01/10 11:50:00, stefan-webrtc wrote:
> It's not clear to me why this is needed. Why can't it be a new frame in this
> case? Is it because the previous packet created a new frame, and this packet
> doesn't have the frame_begin flag set?
> 
> I also don't follow how the test added would capture this... :)

This check covers this case:

p1:
 seqnum 1
 first true
 marker true

p2:
 seqnum 2
 first false
 marker true

When we receive p1 it will be correctly detected as a complete frame. When we
receive p2 it will check if the previous packet is continuous, which it is, and
then try to create a new frame since this is the last packet of the frame.

Without this fix then a frame containing p1 and p2 would be created, even though
p1 is already part of a frame. With this fix we detect that p1 is already part
of a frame and should not be used to create a new frame.

https://codereview.webrtc.org/2613833003/diff/1/webrtc/modules/video_coding/v...
File webrtc/modules/video_coding/video_packet_buffer_unittest.cc (right):

https://codereview.webrtc.org/2613833003/diff/1/webrtc/modules/video_coding/v...
webrtc/modules/video_coding/video_packet_buffer_unittest.cc:467: Insert(3,
kKeyFrame, kNotFirst, kLast);
On 2017/01/10 11:50:00, stefan-webrtc wrote:
> Doesn't this mean you have received the marker bit (kLast)?
> 
> Doesn't seem aligned with the name of the test.

Renamed it to ContinuousSeqNumDoubleMarkerBit.

https://codereview.webrtc.org/2613833003/diff/1/webrtc/modules/video_coding/v...
webrtc/modules/video_coding/video_packet_buffer_unittest.cc:469: ASSERT_EQ(0UL,
frames_from_callback_.size());
On 2017/01/10 11:50:00, stefan-webrtc wrote:
> EXPECT_EQ

Done.

https://codereview.webrtc.org/2613833003/diff/1/webrtc/test/fuzzers/packet_bu...
File webrtc/test/fuzzers/packet_buffer_fuzzer.cc (right):

https://codereview.webrtc.org/2613833003/diff/1/webrtc/test/fuzzers/packet_bu...
webrtc/test/fuzzers/packet_buffer_fuzzer.cc:17: class DummyCallback : public
video_coding::OnReceivedFrameCallback {
On 2017/01/10 11:50:00, stefan-webrtc wrote:
> NullCallback is usually the naming convention

Done.

https://codereview.webrtc.org/2613833003/diff/1/webrtc/test/fuzzers/packet_bu...
webrtc/test/fuzzers/packet_buffer_fuzzer.cc:26: if (size < 200) {
On 2017/01/10 11:50:00, stefan-webrtc wrote:
> No need for this check, right?

We need at least kMinDataNeeded bytes, but we want enough data to be able to
insert a fairly long sequence.

[stefan-webrtc, 2017-01-10 12:11:08]

lgtm

https://codereview.webrtc.org/2613833003/diff/1/webrtc/test/fuzzers/packet_bu...
File webrtc/test/fuzzers/packet_buffer_fuzzer.cc (right):

https://codereview.webrtc.org/2613833003/diff/1/webrtc/test/fuzzers/packet_bu...
webrtc/test/fuzzers/packet_buffer_fuzzer.cc:26: if (size < 200) {
On 2017/01/10 12:03:36, philipel wrote:
> On 2017/01/10 11:50:00, stefan-webrtc wrote:
> > No need for this check, right?
> 
> We need at least kMinDataNeeded bytes, but we want enough data to be able to
> insert a fairly long sequence.

The fuzzer will fairly quickly check with long sequences, so I doubt it's worth 
much to add a check like this.

If you anyway want to keep it I'd want a comment.

[philipel, 2017-01-10 12:20:46]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2017-01-10 12:20:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2613833003/#ps20001 (title:
"Feedback.")

[commit-bot: I haz the power, 2017-01-10 12:20:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[philipel, 2017-01-10 12:20:51]

The CQ bit was unchecked by philipel@webrtc.org

[philipel, 2017-01-10 12:22:05]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2017-01-10 12:22:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2613833003/#ps40001 (title:
".")

[commit-bot: I haz the power, 2017-01-10 12:22:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-10 12:23:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-10 12:24:00]

Try jobs failed on following builders:
  mac_baremetal on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_baremetal/builds/17624)

[philipel, 2017-01-10 16:05:08]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2017-01-10 16:05:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2613833003/#ps60001 (title:
"Rebase")

[commit-bot: I haz the power, 2017-01-10 16:05:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[philipel, 2017-01-10 16:05:31]

The CQ bit was unchecked by philipel@webrtc.org

[philipel, 2017-01-10 16:09:18]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2017-01-10 16:09:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2613833003/#ps80001 (title:
"Rebase 2")

[commit-bot: I haz the power, 2017-01-10 16:09:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-10 16:24:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-10 16:24:23]

Try jobs failed on following builders:
  linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)

[philipel, 2017-01-11 08:50:52]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2017-01-11 08:50:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org, stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2613833003/#ps100001 (title:
"fuzzer fix.")

[commit-bot: I haz the power, 2017-01-11 08:50:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-01-11 10:01:56]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484124652478920,
"parent_rev": "cecf86e9b0ba41c35eda360ae24b17b333ae1977", "commit_rev":
"ea142f8de355c6b32c1f7a6d5715cde97a692502"}

[commit-bot: I haz the power, 2017-01-11 10:01:59]

Description was changed from

==========
Don't detect a new frame if a previous packet is used in a previous frame.

In this CL:
 - Removed unused variable |last_seq_num_|.
 - Fixed bug where a new incomplete frame was detected as a complete frame.
 - Added fuzzer to video_coding::PacketBuffer.

BUG=chromium:677101
==========

to

==========
Don't detect a new frame if a previous packet is used in a previous frame.

In this CL:
 - Removed unused variable |last_seq_num_|.
 - Fixed bug where a new incomplete frame was detected as a complete frame.
 - Added fuzzer to video_coding::PacketBuffer.

BUG=chromium:677101

Review-Url: https://codereview.webrtc.org/2613833003
Cr-Commit-Position: refs/heads/master@{#16003}
Committed:
https://chromium.googlesource.com/external/webrtc/+/ea142f8de355c6b32c1f7a6d5...
==========

[commit-bot: I haz the power, 2017-01-11 10:02:00]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/external/webrtc/+/ea142f8de355c6b32c1f7a6d5...