H264SpsPpsTracker class which keep tracks of SPS/PPS.

H264SpsPpsTracker class which keep tracks of SPS/PPS.

The H264SpsPpsTracker class:
 - Keeps track of all received SPS/PPS.
 - Decides whether a packet should be inserted into the PacketBuffer or not.
   - Don't insert if this packet only contains SPS and/or PPS.
   - Don't insert if this is the first packet of and IDR and we have not
     received the required SPS/PPS.
 - Insert start codes, and in the case of the first packet of an IDR prepend
   the bitstream with the given SPS/PPS for this IDR.

BUG=webrtc:5514
Committed: https://crrev.com/34852cf7072fb7eb596da10dce7a80798c7e8743
Cr-Commit-Position: refs/heads/master@{#14906}

[philipel, 2016-11-02 12:03:40]

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

[philipel, 2016-11-02 12:13:33]

philipel@webrtc.org changed reviewers:
+ danilchap@webrtc.org

[stefan-webrtc, 2016-11-02 13:54:17]

What if packets are received where we don't have the sps/pps because that packet
has been reordered and arrives 10 packets later. Will we be able to decode once
that packet arrives?

https://codereview.webrtc.org/2466993003/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right):

https://codereview.webrtc.org/2466993003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:414: nalu.offset =
start_offset;
Should we set these for FuA too, or make clear they are unset for FuA?

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

https://codereview.webrtc.org/2466993003/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/h264_sps_pps_tracker.cc:37: // if it contains
anything other than just an SPS/PPS nalu.
It would be better if this comment explained why that is the case.

https://codereview.webrtc.org/2466993003/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/h264_sps_pps_tracker.cc:72: if
(!pps_data_[nalu.pps_id].data) {
I think a find would be more appropriate here, or?

https://codereview.webrtc.org/2466993003/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/h264_sps_pps_tracker.cc:91: default: {
What if this is a non-idr but referring to a pps which we don't have?

https://codereview.webrtc.org/2466993003/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/h264_sps_pps_tracker.cc:107: // inserted.
I think this comment is a bit weird. We should always prepend start codes on all
nal units. It has nothing to do with isFirstPacket, right?

FuA is the special case where it's only the start of a FuA that should get a
start code.

https://codereview.webrtc.org/2466993003/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/h264_sps_pps_tracker.cc:129: // to the bitstream
with start codes inserted.
So here we basically always add pps/sps in front of all packets? Is it enough to
do it for key frames?

[philipel, 2016-11-02 14:39:43]

https://codereview.webrtc.org/2466993003/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right):

https://codereview.webrtc.org/2466993003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:414: nalu.offset =
start_offset;
On 2016/11/02 13:54:17, stefan-webrtc (holmer) wrote:
> Should we set these for FuA too, or make clear they are unset for FuA?

Done.

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

https://codereview.webrtc.org/2466993003/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/h264_sps_pps_tracker.cc:37: // if it contains
anything other than just an SPS/PPS nalu.
On 2016/11/02 13:54:17, stefan-webrtc (holmer) wrote:
> It would be better if this comment explained why that is the case.

Done.

https://codereview.webrtc.org/2466993003/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/h264_sps_pps_tracker.cc:72: if
(!pps_data_[nalu.pps_id].data) {
On 2016/11/02 13:54:17, stefan-webrtc (holmer) wrote:
> I think a find would be more appropriate here, or?

Done.

https://codereview.webrtc.org/2466993003/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/h264_sps_pps_tracker.cc:91: default: {
On 2016/11/02 13:54:17, stefan-webrtc (holmer) wrote:
> What if this is a non-idr but referring to a pps which we don't have?

Then that packet will be inserted into the PacketBuffer. The thing is if we only
check the first packet of an IDR frame and then decides to drop that packet (due
to missing SPS/PPS) then the PacketBuffer will never create an RtpFrameObject
since it is missing its first packet.

In short, if we drop the first packet of a keyframe then we ensure that the rest
of the stream is non-decodable, and if we have received a keyframe then we
require all packets in the stream to be received in order to decode it, so we
can't miss any mid-stream SPS/PPS.

https://codereview.webrtc.org/2466993003/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/h264_sps_pps_tracker.cc:107: // inserted.
On 2016/11/02 13:54:17, stefan-webrtc (holmer) wrote:
> I think this comment is a bit weird. We should always prepend start codes on
all
> nal units. It has nothing to do with isFirstPacket, right?
> 
> FuA is the special case where it's only the start of a FuA that should get a
> start code.

Right, removed confusing comment.

https://codereview.webrtc.org/2466993003/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/h264_sps_pps_tracker.cc:129: // to the bitstream
with start codes inserted.
On 2016/11/02 13:54:17, stefan-webrtc (holmer) wrote:
> So here we basically always add pps/sps in front of all packets? Is it enough
to
> do it for key frames?

We only do it for the first packet of an IDR. If you look at line 66 then you
can see that pps_id is only set if this packet is the first packet and it
contains an IDR nalu.

[stefan-webrtc, 2016-11-02 14:54:38]

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

https://codereview.webrtc.org/2466993003/diff/20001/webrtc/modules/video_codi...
webrtc/modules/video_coding/h264_sps_pps_tracker.cc:129: // to the bitstream
with start codes inserted.
On 2016/11/02 14:39:43, philipel wrote:
> On 2016/11/02 13:54:17, stefan-webrtc (holmer) wrote:
> > So here we basically always add pps/sps in front of all packets? Is it
enough
> to
> > do it for key frames?
> 
> We only do it for the first packet of an IDR. If you look at line 66 then you
> can see that pps_id is only set if this packet is the first packet and it
> contains an IDR nalu.

Right, maybe add a comment that we rely on sequence numbers being in sequence
for delta frames to be decodable?

https://codereview.webrtc.org/2466993003/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/h264_sps_pps_tracker.cc (right):

https://codereview.webrtc.org/2466993003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/h264_sps_pps_tracker.cc:37: // to avoid frames being
created contining only these two nalus we don't
containing

https://codereview.webrtc.org/2466993003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/h264_sps_pps_tracker.cc:39: // prepend the bitstream
of first packet of an IDR with the SPS/PPS instead.
"of an IDR referring to the corresponding sps/pps id."?

https://codereview.webrtc.org/2466993003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/h264_sps_pps_tracker.cc:127: required_size +=
data_size;
You still need to add start code if this is the first packet of the fua, right?

[philipel, 2016-11-02 15:11:07]

https://codereview.webrtc.org/2466993003/diff/40001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/h264_sps_pps_tracker.cc (right):

https://codereview.webrtc.org/2466993003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/h264_sps_pps_tracker.cc:37: // to avoid frames being
created contining only these two nalus we don't
On 2016/11/02 14:54:38, stefan-webrtc (holmer) wrote:
> containing

Done.

https://codereview.webrtc.org/2466993003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/h264_sps_pps_tracker.cc:39: // prepend the bitstream
of first packet of an IDR with the SPS/PPS instead.
On 2016/11/02 14:54:38, stefan-webrtc (holmer) wrote:
> "of an IDR referring to the corresponding sps/pps id."?

Done.

https://codereview.webrtc.org/2466993003/diff/40001/webrtc/modules/video_codi...
webrtc/modules/video_coding/h264_sps_pps_tracker.cc:127: required_size +=
data_size;
On 2016/11/02 14:54:38, stefan-webrtc (holmer) wrote:
> You still need to add start code if this is the first packet of the fua,
right?

Yes, added a unittest for this case.

[stefan-webrtc, 2016-11-02 15:14:39]

https://codereview.webrtc.org/2466993003/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/h264_sps_pps_tracker.cc (right):

https://codereview.webrtc.org/2466993003/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/h264_sps_pps_tracker.cc:154: // The first two bytes
describes the length of a segment.
describe

https://codereview.webrtc.org/2466993003/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/h264_sps_pps_tracker.cc:155: uint16_t segment_length
= nalu_ptr[0] << 8 | nalu_ptr[1];
Verify that this isn't out of bounds. before memcpy.

[philipel, 2016-11-02 15:41:05]

https://codereview.webrtc.org/2466993003/diff/60001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/h264_sps_pps_tracker.cc (right):

https://codereview.webrtc.org/2466993003/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/h264_sps_pps_tracker.cc:154: // The first two bytes
describes the length of a segment.
On 2016/11/02 15:14:38, stefan-webrtc (holmer) wrote:
> describe

Done.

https://codereview.webrtc.org/2466993003/diff/60001/webrtc/modules/video_codi...
webrtc/modules/video_coding/h264_sps_pps_tracker.cc:155: uint16_t segment_length
= nalu_ptr[0] << 8 | nalu_ptr[1];
On 2016/11/02 15:14:39, stefan-webrtc (holmer) wrote:
> Verify that this isn't out of bounds. before memcpy.

Done, also added unittest.

[stefan-webrtc, 2016-11-02 15:56:37]

lgtm

[philipel, 2016-11-02 15:59:40]

The CQ bit was checked by philipel@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 15:59:49]

Dry run: 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, 2016-11-02 16:02:33]

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

[commit-bot: I haz the power, 2016-11-02 16:02:34]

Dry run: Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/9758)

[philipel, 2016-11-03 08:39:21]

The CQ bit was checked by philipel@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 08:39:28]

Dry run: 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, 2016-11-03 09:00:17]

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

[commit-bot: I haz the power, 2016-11-03 09:00:18]

Dry run: 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/19153)

[philipel, 2016-11-03 10:41:19]

The CQ bit was checked by philipel@webrtc.org

[philipel, 2016-11-03 10:41:19]

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/2466993003/#ps140001 (title:
"Free copied data")

[commit-bot: I haz the power, 2016-11-03 10:41:28]

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, 2016-11-03 11:03:04]

Committed patchset #8 (id:140001)

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

Description was changed from

==========
H264SpsPpsTracker class which keep tracks of SPS/PPS.

The H264SpsPpsTracker class:
 - Keeps track of all received SPS/PPS.
 - Decides whether a packet should be inserted into the PacketBuffer or not.
   - Don't insert if this packet only contains SPS and/or PPS.
   - Don't insert if this is the first packet of and IDR and we have not
     received the required SPS/PPS.
 - Insert start codes, and in the case of the first packet of an IDR prepend
   the bitstream with the given SPS/PPS for this IDR.

BUG=webrtc:5514
==========

to

==========
H264SpsPpsTracker class which keep tracks of SPS/PPS.

The H264SpsPpsTracker class:
 - Keeps track of all received SPS/PPS.
 - Decides whether a packet should be inserted into the PacketBuffer or not.
   - Don't insert if this packet only contains SPS and/or PPS.
   - Don't insert if this is the first packet of and IDR and we have not
     received the required SPS/PPS.
 - Insert start codes, and in the case of the first packet of an IDR prepend
   the bitstream with the given SPS/PPS for this IDR.

BUG=webrtc:5514

Committed: https://crrev.com/34852cf7072fb7eb596da10dce7a80798c7e8743
Cr-Commit-Position: refs/heads/master@{#14906}
==========

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

Patchset 8 (id:??) landed as
https://crrev.com/34852cf7072fb7eb596da10dce7a80798c7e8743
Cr-Commit-Position: refs/heads/master@{#14906}