Only create H264 frames if there are no gaps in the packet sequence number.

Only create H264 frames if there are no gaps in the packet sequence number.

In the case of H264 we can't know which packet that is the fist packet of a
frame. In order to avoid creating incomplete frames we keep track of which
packets that we haven't received, and if there is a gap in the packet sequence
number leading up to this frame then a frame wont be created.

BUG=chromium:716558
Review-Url: https://codereview.webrtc.org/2926083002
Cr-Commit-Position: refs/heads/master@{#18559}
Committed: https://chromium.googlesource.com/external/webrtc/+/2c9f9f2bc9dbbb6f8abeb92615b42ce0ddfccb39

[philipel, 2017-06-07 11:22:44]

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

[stefan-webrtc, 2017-06-07 11:39:17]

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

https://codereview.webrtc.org/2926083002/diff/1/webrtc/modules/video_coding/p...
webrtc/modules/video_coding/packet_buffer.cc:216: if
(sequence_buffer_[index].seq_num != seq_num)
Not sure I follow this. If the sequence number received isn't the same as what
was already received it's not a new frame? When would this happen? Seems
unlikely except if we're receiving broken packets?

https://codereview.webrtc.org/2926083002/diff/1/webrtc/modules/video_coding/p...
webrtc/modules/video_coding/packet_buffer.cc:312: // the last keyframe to be
considered received.
What does this imply? Are those packets even interesting to us after a keyframe
has been received?

https://codereview.webrtc.org/2926083002/diff/1/webrtc/modules/video_coding/p...
webrtc/modules/video_coding/packet_buffer.cc:398:
missing_packets_.insert(*last_inserted_seq_num_);
Should we guard for not inserting 65000 sequence numbers here in some way?

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

https://codereview.webrtc.org/2926083002/diff/1/webrtc/modules/video_coding/p...
webrtc/modules/video_coding/packet_buffer.h:58: void PaddingReceived(uint16_t
seq_num);
It can probably be discussed how important this is. We could perhaps decide not
to support padding in the same ssrc as an h.264 stream isn't allowed?

Maybe it's better to do it separately after a PSA...

[philipel, 2017-06-07 14:04:27]

Description was changed from

==========
Only create H264 frames if there is no gaps in the packet sequence number.

In the case of H264 we can't know which packet that is the fist packet of a
frame. In order to avoid creating incomplete frames we keep track of which
packets that we haven't received, and if there is a gap in the packet sequence
number leading up to this frame then a frame wont be created.

BUG=chromium:716558
==========

to

==========
Only create H264 frames if there are no gaps in the packet sequence number.

In the case of H264 we can't know which packet that is the fist packet of a
frame. In order to avoid creating incomplete frames we keep track of which
packets that we haven't received, and if there is a gap in the packet sequence
number leading up to this frame then a frame wont be created.

BUG=chromium:716558
==========

[philipel, 2017-06-07 14:04:35]

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

https://codereview.webrtc.org/2926083002/diff/1/webrtc/modules/video_coding/p...
webrtc/modules/video_coding/packet_buffer.cc:216: if
(sequence_buffer_[index].seq_num != seq_num)
On 2017/06/07 11:39:16, stefan-webrtc wrote:
> Not sure I follow this. If the sequence number received isn't the same as what
> was already received it's not a new frame? When would this happen? Seems
> unlikely except if we're receiving broken packets?

The problem we have now is that we can have received frames in our buffer that
we did not create a frame object for. This means we don't set the frame_created
flat to true for those packets, and in the case of the index wrapping this can
cause problems.

For example, if the packet buffer has a size of 16:
0 KF begin
1 KF end     <-- received keyframe, create frame [0, 1]
2 DROPPED!
3 DF begin
4 DF end     <-- received frame but no frame object due to gap
...
16 KF begin  <-- inserted at index 0
17 KF end    <-- inserted at index 1, created frame [16, 17]
18 DF begin  <-- inserted at index 2

When 18 is inserted it will be continuous (first packet of frame), we will then
check 19 (same index as packet 3), and since frame_created == false we will
check frame_begin, which is true. Lastly we check sequence number 20 (packet 4)
and it has the marker bit set. Now we will try to create a frame containing
packet 18, 3 and 4.

https://codereview.webrtc.org/2926083002/diff/1/webrtc/modules/video_coding/p...
webrtc/modules/video_coding/packet_buffer.cc:312: // the last keyframe to be
considered received.
On 2017/06/07 11:39:17, stefan-webrtc wrote:
> What does this imply? Are those packets even interesting to us after a
keyframe
> has been received?

I don't understand my own comment :)

What I wanted to say was that we can still create old (relative to the most
newest keyframe) incomplete H264 delta frames. Maybe it's easier to just remove
the comment all together.

https://codereview.webrtc.org/2926083002/diff/1/webrtc/modules/video_coding/p...
webrtc/modules/video_coding/packet_buffer.cc:398:
missing_packets_.insert(*last_inserted_seq_num_);
On 2017/06/07 11:39:16, stefan-webrtc wrote:
> Should we guard for not inserting 65000 sequence numbers here in some way?

We can insert at most 2^15 missing packets, otherwise |seq_num| would be older
than |last_inserted_seq_num_|. We should still guard against inserting a large
amount of missing packets though...

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

https://codereview.webrtc.org/2926083002/diff/1/webrtc/modules/video_coding/p...
webrtc/modules/video_coding/packet_buffer.h:58: void PaddingReceived(uint16_t
seq_num);
On 2017/06/07 11:39:17, stefan-webrtc wrote:
> It can probably be discussed how important this is. We could perhaps decide
not
> to support padding in the same ssrc as an h.264 stream isn't allowed?
> 
> Maybe it's better to do it separately after a PSA...

I think we should make a PSA that only media packets will be allowed in the
media stream, so padding/FEC will always be sent on a separate SSRC.

[philipel, 2017-06-12 08:54:53]

ping

[philipel, 2017-06-12 08:54:53]

ping

[stefan-webrtc, 2017-06-12 08:56:21]

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

https://codereview.webrtc.org/2926083002/diff/1/webrtc/modules/video_coding/p...
webrtc/modules/video_coding/packet_buffer.cc:312: // the last keyframe to be
considered received.
On 2017/06/07 14:04:35, philipel wrote:
> On 2017/06/07 11:39:17, stefan-webrtc wrote:
> > What does this imply? Are those packets even interesting to us after a
> keyframe
> > has been received?
> 
> I don't understand my own comment :)
> 
> What I wanted to say was that we can still create old (relative to the most
> newest keyframe) incomplete H264 delta frames. Maybe it's easier to just
remove
> the comment all together.

Is this a better comment?
// Frames prior to this key frame are no longer relevant.

In theory though, isn't there a possibility that the frames prior to this key
frame actually will arrive, meaning that we potentially could decode them on
time?

https://codereview.webrtc.org/2926083002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/packet_buffer.cc (right):

https://codereview.webrtc.org/2926083002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet_buffer.cc:395: // Guard agains inserting a
large amount of missing packets if there is a
against

https://codereview.webrtc.org/2926083002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet_buffer.cc:402:
missing_packets_.insert(*last_inserted_seq_num_);
last_inserted_seq_num_ to me sounds like it's the last one that we actually
received. But in practice it's the "largest" one, right?

[stefan-webrtc, 2017-06-12 08:57:13]

lgtm

[philipel, 2017-06-12 10:47:49]

https://codereview.webrtc.org/2926083002/diff/20001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/packet_buffer.cc (right):

https://codereview.webrtc.org/2926083002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet_buffer.cc:395: // Guard agains inserting a
large amount of missing packets if there is a
On 2017/06/12 08:56:21, stefan-webrtc wrote:
> against

Done.

https://codereview.webrtc.org/2926083002/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/packet_buffer.cc:402:
missing_packets_.insert(*last_inserted_seq_num_);
On 2017/06/12 08:56:21, stefan-webrtc wrote:
> last_inserted_seq_num_ to me sounds like it's the last one that we actually
> received. But in practice it's the "largest" one, right?

Yes, that is correct, hard to find a good name for it, maybe rename it to
|newest_inserted_seq_num_|?

[terelius, 2017-06-12 11:50:55]

lgtm

[philipel, 2017-06-12 12:09:22]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2017-06-12 12:09:22]

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/2926083002/#ps40001 (title:
"Feedback")

[commit-bot: I haz the power, 2017-06-12 12:09:34]

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-06-12 12:11:02]

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

[commit-bot: I haz the power, 2017-06-12 12:11:03]

Try jobs failed on following builders:
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/26364)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/25323)

[philipel, 2017-06-13 09:16:48]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2017-06-13 09:16:48]

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

[commit-bot: I haz the power, 2017-06-13 09:16:56]

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-06-13 09:47:28]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497345408413670,
"parent_rev": "fc309750a9cb7014f4504c67761cc6fca7256780", "commit_rev":
"2c9f9f2bc9dbbb6f8abeb92615b42ce0ddfccb39"}

[commit-bot: I haz the power, 2017-06-13 09:47:32]

Description was changed from

==========
Only create H264 frames if there are no gaps in the packet sequence number.

In the case of H264 we can't know which packet that is the fist packet of a
frame. In order to avoid creating incomplete frames we keep track of which
packets that we haven't received, and if there is a gap in the packet sequence
number leading up to this frame then a frame wont be created.

BUG=chromium:716558
==========

to

==========
Only create H264 frames if there are no gaps in the packet sequence number.

In the case of H264 we can't know which packet that is the fist packet of a
frame. In order to avoid creating incomplete frames we keep track of which
packets that we haven't received, and if there is a gap in the packet sequence
number leading up to this frame then a frame wont be created.

BUG=chromium:716558

Review-Url: https://codereview.webrtc.org/2926083002
Cr-Commit-Position: refs/heads/master@{#18559}
Committed:
https://chromium.googlesource.com/external/webrtc/+/2c9f9f2bc9dbbb6f8abeb9261...
==========

[commit-bot: I haz the power, 2017-06-13 09:47:33]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/webrtc/+/2c9f9f2bc9dbbb6f8abeb9261...