Add H264 bitstream rewriting to limit frame reordering marker in header

Add H264 bitstream rewriting to limit frame reordering marker in header

The VUI part an SPS may specify max_num_reorder_frames and
max_dec_frame_buffering. These may cause a decoder to buffer a number
of frame prior allowing decode, leading to delays, even if no frames
using such references (ie B-frames) are sent.

Because of this we update any SPS block emitted by the encoder.

Also, a bunch of refactoring of H264-related code to reduce code
duplication.

BUG=

Committed: https://crrev.com/52033d6ea119223179cc0813f95ff31a21c3ac72
Cr-Commit-Position: refs/heads/master@{#13010}

[sprang_webrtc, 2016-05-16 12:44:02]

sprang@webrtc.org changed reviewers:
+ pbos@webrtc.org

[sprang_webrtc, 2016-05-16 12:44:03]

Please take an initial look. I've more tests and stats in the pipeline.

[sprang_webrtc, 2016-05-17 13:06:59]

sprang@webrtc.org changed reviewers:
+ noahric@chromium.org
- pbos@webrtc.org

[sprang_webrtc, 2016-05-17 13:06:59]

Noah, Peter tells me you looove h264. Do you wanna have a look? :)

[noahric, 2016-05-18 01:35:07]

I don't think you can do this only on the encoder, because you can't guarantee
that both sides of a "webrtc" call are this implementation of webrtc. The only
safe way is to do it on both sides or do it only for android receivers, since
they are the only ones (that we know of) affected by this.

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/h264/bitstream_rewriter.cc (right):

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/bitstream_rewriter.cc:79: sps_state_ =
sps_parser.GetState();
If you want sps_state_ to be exposed, have it as an out parameter as well, along
with output_buffer_.

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/bitstream_rewriter.cc:102: // SpsParser will
have read the vui_params_present flag, which we want to
That's a suuuuuper dangerous coupling. I think you need to have a design that
doesn't rely on the implementation detail of SpsParser if this isn't also a part
of SpsParser.

In the case you move it, you could have an internal helper like ParseSpsUpToVui,
and then have the qp parser parse the Vui and do the rest, and this
functionality use the function below. That's safer, especially when defined
("UpToVui").

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/bitstream_rewriter.cc:144: bool
H264BitstreamRewriter::CopyAndRewriteVui(rtc::BitBuffer* source,
If you pass sps_state as an arg, the class doesn't need to hold state anymore.
Generally, that seems better, since it's an implementation detail that it has
state versus just returning whatever it needs from the function.

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/h264/bitstream_rewriter.h (right):

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/bitstream_rewriter.h:26: // A class that
copies H264 data from a source to a destination and rewrites
Since this version only rewrites SPS and only in a specific way, I'd update the
comment (and maybe class name) to make it more obvious.

And it could(should?) just go on SpsParser, since that all it rewrites.

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/bitstream_rewriter.h:42: bool
ParseAndRewriteSps(const uint8_t* buffer, size_t length);
Make output_buffer an out param; there's no reason for this class to store
these. And then make this a static function, since there shouldn't be any state
at all.

I'm not a fan of functions that use properties/fields as a form of multiple
return values. Out values in C++ are kinda lame, but I still think they are
cleaner than the alternative (storing unnecessary state).

Up to you about the change in API (taking a buffer to write to vs. returning a
new ByteBufferWriter). Assuming that the caller is just going to copy it again,
you're forcing an allocation + a copy on successful calls. It's a small piece of
data, so it probably doesn't matter much, but I think the out param on this is
harder to get right as well (pointer to a std::unique_ptr? reference to a
std::unique_ptr? A raw pointer that the caller has to remember to clear?).

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/bitstream_rewriter.h:48: bool
CopyHrdParameters(rtc::BitBuffer* source,
These don't need to be on the class (they weren't in my original version on
purpose), so there's no strong reason to have them in the header. You can always
declare them at the top of the .cc, if you want them to be more obvious.

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/h264/h264_common.cc (right):

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/h264_common.cc:79: // Walk length backwards
to remove any empty bytes at the end.
I don't remember if that's generally correct. It was an implementation detail of
the bitstream rewriter, but I can't remember if it's legal to end in 0s or not
(I think it isn't, but you should double-check the standard).

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/h264_common.cc:80: while (length > 0 &&
bytes[length - 1] == 0)
Man, webrtc engineers and hating on braces for single line conditional blocks
:-p

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/h264/h264_common.h (right):

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/h264_common.h:24: // The size of a NALU
header {0 0 0 1}.
Three byte start codes are also valid, but assuming something else (jitter
buffer?) is ensuring a 4 byte header, I'd add that to the comment.

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/h264_common.h:30: enum NaluType : uint8_t {
kSps = 0x7, kPps = 0x8, kIdr = 0x5 };
While you are here, add Sei, in case someone needs it. They get inserted by some
implementations (e.g. the h264 encoder on iOS).

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/h264_common.h:33: // Returns a vector of the
NALU start sequences (0 0 0 1) in the given buffer.
My bad (it was my comment originally), but I'd rephrase slightly:
// Returns a vector of NALU start sequence (0 0 0 1) locations in the given
buffer.

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/h264_common.h:36: static NaluType
ParseNaluType(uint8_t data);
Comments for these would help.

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/h264/pps_parser.h (right):

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/pps_parser.h:25: bool Parse();
I know this is unrelated, but this class would be cleaner as a single static
method (Parse) that returned a bool and had an out parameter for state.

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/h264/sps_parser.h (right):

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/sps_parser.h:26: SpsParser();
Yeah, this is really weird now; you use this constructor + ParseFrom, or the
other constructor + Parse(). This should really just have two (static)
Parse[...] methods that return SpsState as an out param.

So:
class SpsParser {
 public:
  // Parses the given buffer for SpsState. On success, returns true and writes
SPS
  // information to |out_sps_state|, which must be non-null.
  static bool Parse(const uint8_t* buffer, size_t buffer_length, SpsState*
out_sps_state);

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/sps_parser.h:35: bool
ParseFromRbspDecodedBitBuffer(rtc::BitBuffer* buffer);
I still think you want to get rid of this. It's strange how it modifies buffer
(e.g. it makes more sense, in abstract, to have the caller do buffer.Data(),
buffer.Length() on the other parse function), and requires the parser at a
certain point.

[sprang_webrtc, 2016-05-20 16:10:58]

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

[sprang_webrtc, 2016-05-20 16:11:00]

Changed a lot of parsing methods to statics, and number of bug fixes and
improvements.
Rewriting is btw done on both send and receive side now.
Stefan, you may want to have a look at the packetization bits.

I still have tests I haven't included yet, and some stats to add.

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/h264/bitstream_rewriter.cc (right):

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/bitstream_rewriter.cc:79: sps_state_ =
sps_parser.GetState();
On 2016/05/18 01:35:06, noahric wrote:
> If you want sps_state_ to be exposed, have it as an out parameter as well,
along
> with output_buffer_.

Done.

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/bitstream_rewriter.cc:102: // SpsParser will
have read the vui_params_present flag, which we want to
On 2016/05/18 01:35:06, noahric wrote:
> That's a suuuuuper dangerous coupling. I think you need to have a design that
> doesn't rely on the implementation detail of SpsParser if this isn't also a
part
> of SpsParser.
> 
> In the case you move it, you could have an internal helper like
ParseSpsUpToVui,
> and then have the qp parser parse the Vui and do the rest, and this
> functionality use the function below. That's safer, especially when defined
> ("UpToVui").

Yeah, this was a bit iffy. Screwups should be caught be tests though.
Anyway, made a protected internal method SpsParser::ParseSpsUpToVui() that is
used
instead.

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/bitstream_rewriter.cc:144: bool
H264BitstreamRewriter::CopyAndRewriteVui(rtc::BitBuffer* source,
On 2016/05/18 01:35:06, noahric wrote:
> If you pass sps_state as an arg, the class doesn't need to hold state anymore.
> Generally, that seems better, since it's an implementation detail that it has
> state versus just returning whatever it needs from the function.

Done.

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/h264/bitstream_rewriter.h (right):

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/bitstream_rewriter.h:26: // A class that
copies H264 data from a source to a destination and rewrites
On 2016/05/18 01:35:06, noahric wrote:
> Since this version only rewrites SPS and only in a specific way, I'd update
the
> comment (and maybe class name) to make it more obvious.
> 
> And it could(should?) just go on SpsParser, since that all it rewrites.

I'd rather keep it out of SpsParser, since this is quite large even with the SPS
parsing removed.
It's use case is also quite specific. Renamed and recommented it though.

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/bitstream_rewriter.h:42: bool
ParseAndRewriteSps(const uint8_t* buffer, size_t length);
On 2016/05/18 01:35:06, noahric wrote:
> Make output_buffer an out param; there's no reason for this class to store
> these. And then make this a static function, since there shouldn't be any
state
> at all.
> 
> I'm not a fan of functions that use properties/fields as a form of multiple
> return values. Out values in C++ are kinda lame, but I still think they are
> cleaner than the alternative (storing unnecessary state).
> 
> Up to you about the change in API (taking a buffer to write to vs. returning a
> new ByteBufferWriter). Assuming that the caller is just going to copy it
again,
> you're forcing an allocation + a copy on successful calls. It's a small piece
of
> data, so it probably doesn't matter much, but I think the out param on this is
> harder to get right as well (pointer to a std::unique_ptr? reference to a
> std::unique_ptr? A raw pointer that the caller has to remember to clear?).

I quite dislike output params, but fine :P
I went with a raw pointer to an rtc::Buffer since it allows the caller to 
easily have things prepended and appended to the modified sps.

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/bitstream_rewriter.h:48: bool
CopyHrdParameters(rtc::BitBuffer* source,
On 2016/05/18 01:35:06, noahric wrote:
> These don't need to be on the class (they weren't in my original version on
> purpose), so there's no strong reason to have them in the header. You can
always
> declare them at the top of the .cc, if you want them to be more obvious.

Done.

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/h264/h264_common.cc (right):

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/h264_common.cc:79: // Walk length backwards
to remove any empty bytes at the end.
On 2016/05/18 01:35:06, noahric wrote:
> I don't remember if that's generally correct. It was an implementation detail
of
> the bitstream rewriter, but I can't remember if it's legal to end in 0s or not
> (I think it isn't, but you should double-check the standard).

Changed things around so this isn't necessary anymore.

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/h264_common.cc:80: while (length > 0 &&
bytes[length - 1] == 0)
On 2016/05/18 01:35:06, noahric wrote:
> Man, webrtc engineers and hating on braces for single line conditional blocks
> :-p

I've been slapping in the back of the head by Peter so many times now, it's a
reflex! >)

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/h264/h264_common.h (right):

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/h264_common.h:24: // The size of a NALU
header {0 0 0 1}.
On 2016/05/18 01:35:06, noahric wrote:
> Three byte start codes are also valid, but assuming something else (jitter
> buffer?) is ensuring a 4 byte header, I'd add that to the comment.

I think this has worked because the standard mandates 4 byte start sequences for
SPS, PPS and the first NAL of an access unit. And that's what we care about.
Have updated this and parsing to handle both though.

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/h264_common.h:30: enum NaluType : uint8_t {
kSps = 0x7, kPps = 0x8, kIdr = 0x5 };
On 2016/05/18 01:35:07, noahric wrote:
> While you are here, add Sei, in case someone needs it. They get inserted by
some
> implementations (e.g. the h264 encoder on iOS).

Done.

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/h264_common.h:33: // Returns a vector of the
NALU start sequences (0 0 0 1) in the given buffer.
On 2016/05/18 01:35:06, noahric wrote:
> My bad (it was my comment originally), but I'd rephrase slightly:
> // Returns a vector of NALU start sequence (0 0 0 1) locations in the given
> buffer.

Changed this to contain both offset of nal header and payload, plus size, which
simplifies a bunch of code at the expense of a few bytes of extra memory usage.

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/h264_common.h:36: static NaluType
ParseNaluType(uint8_t data);
On 2016/05/18 01:35:07, noahric wrote:
> Comments for these would help.

Done.

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/h264/pps_parser.h (right):

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/pps_parser.h:25: bool Parse();
On 2016/05/18 01:35:07, noahric wrote:
> I know this is unrelated, but this class would be cleaner as a single static
> method (Parse) that returned a bool and had an out parameter for state.

Acknowledged.

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/h264/sps_parser.h (right):

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/sps_parser.h:26: SpsParser();
On 2016/05/18 01:35:07, noahric wrote:
> Yeah, this is really weird now; you use this constructor + ParseFrom, or the
> other constructor + Parse(). This should really just have two (static)
> Parse[...] methods that return SpsState as an out param.
> 
> So:
> class SpsParser {
>  public:
>   // Parses the given buffer for SpsState. On success, returns true and writes
> SPS
>   // information to |out_sps_state|, which must be non-null.
>   static bool Parse(const uint8_t* buffer, size_t buffer_length, SpsState*
> out_sps_state);

Acknowledged.

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/sps_parser.h:35: bool
ParseFromRbspDecodedBitBuffer(rtc::BitBuffer* buffer);
On 2016/05/18 01:35:07, noahric wrote:
> I still think you want to get rid of this. It's strange how it modifies buffer
> (e.g. it makes more sense, in abstract, to have the caller do buffer.Data(),
> buffer.Length() on the other parse function), and requires the parser at a
> certain point.

Acknowledged.

[stefan-webrtc, 2016-05-22 23:11:50]

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/h264/h264_common.h (right):

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/h264/h264_common.h:22: class H264Common {
Should this be a namespace instead, maybe?

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/h264/h264_common.h:45: enum SliceType : uint8_t {
P = 0, B = 1, I = 2, Sp = 3, Si = 4 };
Empty line above.

You should probably add k before each enum value too, right?

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/h264/sps_parser.cc (right):

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/h264/sps_parser.cc:18: typedef
rtc::Optional<webrtc::SpsParser::SpsState> Sps;
Call this OptionalSps, or remove it and write the full type name.

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/h264/sps_parser_unittest.cc (right):

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/h264/sps_parser_unittest.cc:129: return sps_ ?
true : false;
I think you can return sps_().

Wouldn't it be simpler to just call SpsParser::ParseSps() directly from the
tests? I don't think this makes the code that much easier to read :)

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right):

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:66: offsets->push_back(offset
+ kLengthFieldSize + base_offset);
Why do we have to pass in base_offset? It seems to always be kNalHeaderSize, so
maybe just replace it with that?

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:112: // decoding.
Please comment on the possible side effects, if any.

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:160:
packets_.push(PacketUnit(Fragment(fragment.buffer + offset, packet_length),
What happens here if fragment is a rewritten sps? Are we sure that fragment will
outlive this new Fragment?

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:334: bool
RtpDepacketizerH264::ParseSingleNalu(ParsedPayload* parsed_payload,
Could you point out which parts of these functions actually changed?

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.h (right):

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.h:121:
std::unique_ptr<rtc::Buffer> modified_buffer_;
Did you consider propagating the copy on modify buffer all the way in from the
network layers instead?

Are we sure that a second packet won't overwrite this buffer while the first
packet is still being processed? Should that be ensured by this class? Seems a
bit brittle to leave that to the caller.

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc (right):

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:417: 0x00, 0x00,
0x03, 0x2A, 0xE0, 0xF1, 0x83, 0x25};
Why did this change?

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:456: 0x00, 0x11};
... and this?

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:572: 
Would be good with explicit test cases for the sps rewriting here.

[sprang_webrtc, 2016-05-25 09:06:03]

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/h264/h264_common.h (right):

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/h264/h264_common.h:22: class H264Common {
On 2016/05/22 23:11:50, stefan-webrtc (holmer) wrote:
> Should this be a namespace instead, maybe?

Done.

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/h264/h264_common.h:45: enum SliceType : uint8_t {
P = 0, B = 1, I = 2, Sp = 3, Si = 4 };
On 2016/05/22 23:11:50, stefan-webrtc (holmer) wrote:
> Empty line above.
> 
> You should probably add k before each enum value too, right?

Done.

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/h264/sps_parser.cc (right):

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/h264/sps_parser.cc:18: typedef
rtc::Optional<webrtc::SpsParser::SpsState> Sps;
On 2016/05/22 23:11:50, stefan-webrtc (holmer) wrote:
> Call this OptionalSps, or remove it and write the full type name.

Done.

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/h264/sps_parser_unittest.cc (right):

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/h264/sps_parser_unittest.cc:129: return sps_ ?
true : false;
On 2016/05/22 23:11:50, stefan-webrtc (holmer) wrote:
> I think you can return sps_().

Nope, "type 'rtc::Optional<SpsParser::SpsState>' does not provide a call
operator". static_cast<bool> works though. Ugly, but maybe less so.

> Wouldn't it be simpler to just call SpsParser::ParseSps() directly from the
> tests? I don't think this makes the code that much easier to read :)
Right, I did this because I moved rbsp-parsing out form SpsParser::ParseSps, but
I've since moved it back in and so this isn't useful anymore.

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right):

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:66: offsets->push_back(offset
+ kLengthFieldSize + base_offset);
On 2016/05/22 23:11:50, stefan-webrtc (holmer) wrote:
> Why do we have to pass in base_offset? It seems to always be kNalHeaderSize,
so
> maybe just replace it with that?

I intended to use it in a second place, but ended up not to. Removed.

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:112: // decoding.
On 2016/05/22 23:11:50, stefan-webrtc (holmer) wrote:
> Please comment on the possible side effects, if any.

Consequences for rewriting the VUI, in general? I'll add a comment and an assert
that don't get any b-frames as well.

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:160:
packets_.push(PacketUnit(Fragment(fragment.buffer + offset, packet_length),
On 2016/05/22 23:11:50, stefan-webrtc (holmer) wrote:
> What happens here if fragment is a rewritten sps? Are we sure that fragment
will
> outlive this new Fragment?

Yes. In the case of FU-A, the input fragment won't be popped from the queue
until the last PacketUnit for that fragment has been consumed (from
RtpPacketizerH264::NextFragmentPacket below):

if (packet->last_fragment)
   input_fragments_.pop_front();

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:334: bool
RtpDepacketizerH264::ParseSingleNalu(ParsedPayload* parsed_payload,
On 2016/05/22 23:11:50, stefan-webrtc (holmer) wrote:
> Could you point out which parts of these functions actually changed?

391-417 and 442-444, essentially. Otherwise maybe some formatting changes
(moving enums to h264_common, etc)

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.h (right):

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.h:121:
std::unique_ptr<rtc::Buffer> modified_buffer_;
On 2016/05/22 23:11:50, stefan-webrtc (holmer) wrote:
> Did you consider propagating the copy on modify buffer all the way in from the
> network layers instead?

Yes, but that turned out to be a bit more work than I'd like - and something
mflodman@ has already begun working on but hasn't finished yet. We can revisit
this at that time instead, I'll add a TODO.

> Are we sure that a second packet won't overwrite this buffer while the first
> packet is still being processed? Should that be ensured by this class? Seems a
> bit brittle to leave that to the caller.

I've added an assert that we don't have two SPS blocks, that both needs to be
modified, packet together within a single RTP packet using STAP-A. That should
really not happen.

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc (right):

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:417: 0x00, 0x00,
0x03, 0x2A, 0xE0, 0xF1, 0x83, 0x25};
On 2016/05/22 23:11:50, stefan-webrtc (holmer) wrote:
> Why did this change?

Parsing revealed that VUI contained:
max_num_reorder_frames	uint32_t = 2	
max_dec_frame_buffering	uint32_t = 4	

So, I added a modified version in there.

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:456: 0x00, 0x11};
On 2016/05/22 23:11:50, stefan-webrtc (holmer) wrote:
> ... and this?

...same here. How did you come up with these magic numbers?

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:572: 
On 2016/05/22 23:11:50, stefan-webrtc (holmer) wrote:
> Would be good with explicit test cases for the sps rewriting here.

Yes, in the pipeline.

[sprang_webrtc, 2016-05-26 15:07:48]

Ping?
I'd really like to get this landed before EOW...

[stefan-webrtc, 2016-05-26 18:08:13]

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right):

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:118:
output_buffer->AppendData((*rbsp_buffer)[0]);
Was this wrong? Covered by a test?

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:160:
packets_.push(PacketUnit(Fragment(fragment.buffer + offset, packet_length),
On 2016/05/25 09:06:03, språng wrote:
> On 2016/05/22 23:11:50, stefan-webrtc (holmer) wrote:
> > What happens here if fragment is a rewritten sps? Are we sure that fragment
> will
> > outlive this new Fragment?
> 
> Yes. In the case of FU-A, the input fragment won't be popped from the queue
> until the last PacketUnit for that fragment has been consumed (from
> RtpPacketizerH264::NextFragmentPacket below):
> 
> if (packet->last_fragment)
>    input_fragments_.pop_front();

Acknowledged.

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.h (right):

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.h:121:
std::unique_ptr<rtc::Buffer> modified_buffer_;
On 2016/05/25 09:06:03, språng wrote:
> On 2016/05/22 23:11:50, stefan-webrtc (holmer) wrote:
> > Did you consider propagating the copy on modify buffer all the way in from
the
> > network layers instead?
> 
> Yes, but that turned out to be a bit more work than I'd like - and something
> mflodman@ has already begun working on but hasn't finished yet. We can revisit
> this at that time instead, I'll add a TODO.

Acknowledged.

> 
> > Are we sure that a second packet won't overwrite this buffer while the first
> > packet is still being processed? Should that be ensured by this class? Seems
a
> > bit brittle to leave that to the caller.
> 
> I've added an assert that we don't have two SPS blocks, that both needs to be
> modified, packet together within a single RTP packet using STAP-A. That should
> really not happen.

That's not what I was thinking of. What if the first packet is parsed and sent
off to the jitter buffer, but before the jitter buffer is done with it a new
packet comes in which also needs to be modified, effectively overwriting the
first packet.

I think that will become increasingly likely with philipel's new jitter buffer
which doesn't copy the packets until they are passed to the decoder, see
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc....

I think we'll need a better way of handling this... The best way would really be
to have a copy-on-modify, or if we could simply pass on packet ownership?

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc (right):

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:456: 0x00, 0x11};
On 2016/05/25 09:06:03, språng wrote:
> On 2016/05/22 23:11:50, stefan-webrtc (holmer) wrote:
> > ... and this?
> 
> ...same here. How did you come up with these magic numbers?

I think pbos added these.

https://codereview.webrtc.org/1979443004/diff/170001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/h264/pps_parser.cc (right):

https://codereview.webrtc.org/1979443004/diff/170001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/h264/pps_parser.cc:80: // See 8.2.2.2
Specification for dispersed slice group map type.
Could you comment on why this is needed, and what the impact of not supporting
it is?

https://codereview.webrtc.org/1979443004/diff/170001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right):

https://codereview.webrtc.org/1979443004/diff/170001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:99: if (nalu_type ==
H264::NaluType::kSps) {
This code feels misplaced to me. Shouldn't it be handled in the parsing step as
it is for STAP-A?

https://codereview.webrtc.org/1979443004/diff/170001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:337: bool
RtpDepacketizerH264::ParseSingleNalu(ParsedPayload* parsed_payload,
Should we rename this method maybe? It does quite a lot...

https://codereview.webrtc.org/1979443004/diff/170001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:397:
RTC_CHECK(!modified_buffer_);
Not sure the right thing is to crash though. I'd prefer logging a warning and
not rewriting the second SPS.

[noahric, 2016-05-26 18:56:52]

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/h264/h264_common.cc (right):

https://codereview.webrtc.org/1979443004/diff/70001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/h264/h264_common.cc:80: while (length > 0 &&
bytes[length - 1] == 0)
On 2016/05/20 16:11:00, språng wrote:
> On 2016/05/18 01:35:06, noahric wrote:
> > Man, webrtc engineers and hating on braces for single line conditional
blocks
> > :-p
> 
> I've been slapping in the back of the head by Peter so many times now, it's a
> reflex! >)

I'd slap him back for you, except geography intervenes :(

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right):

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:118:
output_buffer->AppendData((*rbsp_buffer)[0]);
On 2016/05/26 18:08:13, stefan-webrtc (holmer) wrote:
> Was this wrong? Covered by a test?

And either way, it definitely needs a comment, since the next line appears to
use rbsp_buffer[0] again.

Also, slightly prefer rbsp_buffer->data()[0], to be a bit more obvious (the
parens and explicit unique_ptr deref are sorta confusing).

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc (right):

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264_unittest.cc:456: 0x00, 0x11};
On 2016/05/26 18:08:13, stefan-webrtc (holmer) wrote:
> On 2016/05/25 09:06:03, språng wrote:
> > On 2016/05/22 23:11:50, stefan-webrtc (holmer) wrote:
> > > ... and this?
> > 
> > ...same here. How did you come up with these magic numbers?
> 
> I think pbos added these.

And I think they came from my code originally, which had some comments
explaining them. See:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

https://codereview.webrtc.org/1979443004/diff/170001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right):

https://codereview.webrtc.org/1979443004/diff/170001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:397:
RTC_CHECK(!modified_buffer_);
On 2016/05/26 18:08:13, stefan-webrtc (holmer) wrote:
> Not sure the right thing is to crash though. I'd prefer logging a warning and
> not rewriting the second SPS.

In that case, you'd want to rewrite the last one, not the first one, since the
last one is the one that "wins".

[sprang_webrtc, 2016-05-27 13:12:22]

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right):

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:118:
output_buffer->AppendData((*rbsp_buffer)[0]);
On 2016/05/26 18:08:13, stefan-webrtc (holmer) wrote:
> Was this wrong? Covered by a test?

Yes. Uncovered and cover by a test I added.

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:118:
output_buffer->AppendData((*rbsp_buffer)[0]);
On 2016/05/26 18:56:52, noahric wrote:
> On 2016/05/26 18:08:13, stefan-webrtc (holmer) wrote:
> > Was this wrong? Covered by a test?
> 
> And either way, it definitely needs a comment, since the next line appears to
> use rbsp_buffer[0] again.
> 
> Also, slightly prefer rbsp_buffer->data()[0], to be a bit more obvious (the
> parens and explicit unique_ptr deref are sorta confusing).
> 

Acknowledged.

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.h (right):

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.h:121:
std::unique_ptr<rtc::Buffer> modified_buffer_;
On 2016/05/26 18:08:13, stefan-webrtc (holmer) wrote:
> On 2016/05/25 09:06:03, språng wrote:
> > On 2016/05/22 23:11:50, stefan-webrtc (holmer) wrote:
> > > Did you consider propagating the copy on modify buffer all the way in from
> the
> > > network layers instead?
> > 
> > Yes, but that turned out to be a bit more work than I'd like - and something
> > mflodman@ has already begun working on but hasn't finished yet. We can
revisit
> > this at that time instead, I'll add a TODO.
> 
> Acknowledged.
> 
> > 
> > > Are we sure that a second packet won't overwrite this buffer while the
first
> > > packet is still being processed? Should that be ensured by this class?
Seems
> a
> > > bit brittle to leave that to the caller.
> > 
> > I've added an assert that we don't have two SPS blocks, that both needs to
be
> > modified, packet together within a single RTP packet using STAP-A. That
should
> > really not happen.
> 
> That's not what I was thinking of. What if the first packet is parsed and sent
> off to the jitter buffer, but before the jitter buffer is done with it a new
> packet comes in which also needs to be modified, effectively overwriting the
> first packet.
> 
> I think that will become increasingly likely with philipel's new jitter buffer
> which doesn't copy the packets until they are passed to the decoder, see
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc....
> 
> I think we'll need a better way of handling this... The best way would really
be
> to have a copy-on-modify, or if we could simply pass on packet ownership?

I've talked to Philip about this. We'll need a copy somewhere anyway, to hand
ownership over to the jitter buffer, and this seems like the right place. For
now, let's keep this and then in a follow up CL, have ParsedPayload contain a
unique_ptr with a copy of the parsed buffer. Then we can std::move that into the
jitter buffer making ownership clear, and any codec specific pre-processing
needed can take place in the depacketizer instead of inside the jitter buffer,
which is the case today.

https://codereview.webrtc.org/1979443004/diff/170001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/h264/pps_parser.cc (right):

https://codereview.webrtc.org/1979443004/diff/170001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/h264/pps_parser.cc:80: // See 8.2.2.2
Specification for dispersed slice group map type.
On 2016/05/26 18:08:13, stefan-webrtc (holmer) wrote:
> Could you comment on why this is needed, and what the impact of not supporting
> it is?

To be honest I have no idea :)
While refactoring I just noticed we missed an if-clause for one of the valid
values.
Noah, this was your code originally, can you comment?

https://codereview.webrtc.org/1979443004/diff/170001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right):

https://codereview.webrtc.org/1979443004/diff/170001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:99: if (nalu_type ==
H264::NaluType::kSps) {
On 2016/05/26 18:08:13, stefan-webrtc (holmer) wrote:
> This code feels misplaced to me. Shouldn't it be handled in the parsing step
as
> it is for STAP-A?

This is in the packetizer (on the send side), before potentially being being
re-packed in Stap-A or Fu-A. Intended to protect legacy receive clients from new
send clients with bad encoders.

The other code doing the same thing is for the receive side (protecting newer
receive clients from legacy send clients sending bad bitstreams).
We currently only support Stap-A/Single NALU there, as otherwise we'd have to
wait until complete frames have been assembled by the jitter buffer and then
parse out the NAL units, rewrite, and reassemble....

I'll try to clarify with some more comments.

https://codereview.webrtc.org/1979443004/diff/170001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:397:
RTC_CHECK(!modified_buffer_);
On 2016/05/26 18:56:52, noahric wrote:
> On 2016/05/26 18:08:13, stefan-webrtc (holmer) wrote:
> > Not sure the right thing is to crash though. I'd prefer logging a warning
and
> > not rewriting the second SPS.
> 
> In that case, you'd want to rewrite the last one, not the first one, since the
> last one is the one that "wins".

Done.

[stefan-webrtc, 2016-05-27 20:57:00]

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right):

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:112: // decoding.
On 2016/05/25 09:06:03, språng wrote:
> On 2016/05/22 23:11:50, stefan-webrtc (holmer) wrote:
> > Please comment on the possible side effects, if any.
> 
> Consequences for rewriting the VUI, in general? I'll add a comment and an
assert
> that don't get any b-frames as well.

Acknowledged.

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.h (right):

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.h:121:
std::unique_ptr<rtc::Buffer> modified_buffer_;
On 2016/05/27 13:12:21, språng wrote:
> On 2016/05/26 18:08:13, stefan-webrtc (holmer) wrote:
> > On 2016/05/25 09:06:03, språng wrote:
> > > On 2016/05/22 23:11:50, stefan-webrtc (holmer) wrote:
> > > > Did you consider propagating the copy on modify buffer all the way in
from
> > the
> > > > network layers instead?
> > > 
> > > Yes, but that turned out to be a bit more work than I'd like - and
something
> > > mflodman@ has already begun working on but hasn't finished yet. We can
> revisit
> > > this at that time instead, I'll add a TODO.
> > 
> > Acknowledged.
> > 
> > > 
> > > > Are we sure that a second packet won't overwrite this buffer while the
> first
> > > > packet is still being processed? Should that be ensured by this class?
> Seems
> > a
> > > > bit brittle to leave that to the caller.
> > > 
> > > I've added an assert that we don't have two SPS blocks, that both needs to
> be
> > > modified, packet together within a single RTP packet using STAP-A. That
> should
> > > really not happen.
> > 
> > That's not what I was thinking of. What if the first packet is parsed and
sent
> > off to the jitter buffer, but before the jitter buffer is done with it a new
> > packet comes in which also needs to be modified, effectively overwriting the
> > first packet.
> > 
> > I think that will become increasingly likely with philipel's new jitter
buffer
> > which doesn't copy the packets until they are passed to the decoder, see
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc....
> > 
> > I think we'll need a better way of handling this... The best way would
really
> be
> > to have a copy-on-modify, or if we could simply pass on packet ownership?
> 
> I've talked to Philip about this. We'll need a copy somewhere anyway, to hand
> ownership over to the jitter buffer, and this seems like the right place. For
> now, let's keep this and then in a follow up CL, have ParsedPayload contain a
> unique_ptr with a copy of the parsed buffer. Then we can std::move that into
the
> jitter buffer making ownership clear, and any codec specific pre-processing
> needed can take place in the depacketizer instead of inside the jitter buffer,
> which is the case today.

Yes, a copy will be needed, at least for the near future. It's possible to pass
ownership of the packet all the way from the socket to the jitter buffer though.

But if we keep this for now, does that mean we're making the assumption that we
won't run into the risk of races when one packet is on its way to the JB (but
hasn't been copied yet) and another packet with an sps to be modified arrives?
Is that safe? If you think it is I think you at least have to document it. Or
should we add a lock or something for now to ensure it's not an issue?

https://codereview.webrtc.org/1979443004/diff/170001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right):

https://codereview.webrtc.org/1979443004/diff/170001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:99: if (nalu_type ==
H264::NaluType::kSps) {
On 2016/05/27 13:12:22, språng wrote:
> On 2016/05/26 18:08:13, stefan-webrtc (holmer) wrote:
> > This code feels misplaced to me. Shouldn't it be handled in the parsing step
> as
> > it is for STAP-A?
> 
> This is in the packetizer (on the send side), before potentially being being
> re-packed in Stap-A or Fu-A. Intended to protect legacy receive clients from
new
> send clients with bad encoders.
> 
> The other code doing the same thing is for the receive side (protecting newer
> receive clients from legacy send clients sending bad bitstreams).
> We currently only support Stap-A/Single NALU there, as otherwise we'd have to
> wait until complete frames have been assembled by the jitter buffer and then
> parse out the NAL units, rewrite, and reassemble....
> 
> I'll try to clarify with some more comments.

Ah, my mistake. :)

https://codereview.webrtc.org/1979443004/diff/250001/webrtc/common_video/h264...
File webrtc/common_video/h264/sps_parser.cc (right):

https://codereview.webrtc.org/1979443004/diff/250001/webrtc/common_video/h264...
webrtc/common_video/h264/sps_parser.cc:20: #define RETURN_FALSE_ON_FAIL(x) \
Shouldn't the name be changed now that it doesn't return false? :)

[sprang_webrtc, 2016-05-30 08:57:23]

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.h (right):

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.h:121:
std::unique_ptr<rtc::Buffer> modified_buffer_;
On 2016/05/27 20:56:59, stefan-webrtc (holmer) wrote:
> On 2016/05/27 13:12:21, språng wrote:
> > On 2016/05/26 18:08:13, stefan-webrtc (holmer) wrote:
> > > On 2016/05/25 09:06:03, språng wrote:
> > > > On 2016/05/22 23:11:50, stefan-webrtc (holmer) wrote:
> > > > > Did you consider propagating the copy on modify buffer all the way in
> from
> > > the
> > > > > network layers instead?
> > > > 
> > > > Yes, but that turned out to be a bit more work than I'd like - and
> something
> > > > mflodman@ has already begun working on but hasn't finished yet. We can
> > revisit
> > > > this at that time instead, I'll add a TODO.
> > > 
> > > Acknowledged.
> > > 
> > > > 
> > > > > Are we sure that a second packet won't overwrite this buffer while the
> > first
> > > > > packet is still being processed? Should that be ensured by this class?
> > Seems
> > > a
> > > > > bit brittle to leave that to the caller.
> > > > 
> > > > I've added an assert that we don't have two SPS blocks, that both needs
to
> > be
> > > > modified, packet together within a single RTP packet using STAP-A. That
> > should
> > > > really not happen.
> > > 
> > > That's not what I was thinking of. What if the first packet is parsed and
> sent
> > > off to the jitter buffer, but before the jitter buffer is done with it a
new
> > > packet comes in which also needs to be modified, effectively overwriting
the
> > > first packet.
> > > 
> > > I think that will become increasingly likely with philipel's new jitter
> buffer
> > > which doesn't copy the packets until they are passed to the decoder, see
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc....
> > > 
> > > I think we'll need a better way of handling this... The best way would
> really
> > be
> > > to have a copy-on-modify, or if we could simply pass on packet ownership?
> > 
> > I've talked to Philip about this. We'll need a copy somewhere anyway, to
hand
> > ownership over to the jitter buffer, and this seems like the right place.
For
> > now, let's keep this and then in a follow up CL, have ParsedPayload contain
a
> > unique_ptr with a copy of the parsed buffer. Then we can std::move that into
> the
> > jitter buffer making ownership clear, and any codec specific pre-processing
> > needed can take place in the depacketizer instead of inside the jitter
buffer,
> > which is the case today.
> 
> Yes, a copy will be needed, at least for the near future. It's possible to
pass
> ownership of the packet all the way from the socket to the jitter buffer
though.
> 
> But if we keep this for now, does that mean we're making the assumption that
we
> won't run into the risk of races when one packet is on its way to the JB (but
> hasn't been copied yet) and another packet with an sps to be modified arrives?
> Is that safe? If you think it is I think you at least have to document it. Or
> should we add a lock or something for now to ensure it's not an issue?

The incoming packet goes out of scope when the IncomingRtpPacket() callback
returns, so the jitter buffer can really only use it directly if it's a single
packet frame that's received. Otherwise a copy will be needed somewhere even in
the future; it can never just hold on to the pointer without being racy.

After discussing with with philipel@, we agreed that _always_ cloning incoming
payload in the depackterizer is the right thing to do. Not only for H264, but
all codecs. I'll add a comment to this effect in the RtpDepacketizer interface.

https://codereview.webrtc.org/1979443004/diff/250001/webrtc/common_video/h264...
File webrtc/common_video/h264/sps_parser.cc (right):

https://codereview.webrtc.org/1979443004/diff/250001/webrtc/common_video/h264...
webrtc/common_video/h264/sps_parser.cc:20: #define RETURN_FALSE_ON_FAIL(x) \
On 2016/05/27 20:57:00, stefan-webrtc (holmer) wrote:
> Shouldn't the name be changed now that it doesn't return false? :)

Done.

[stefan-webrtc, 2016-05-30 11:00:55]

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.h (right):

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.h:121:
std::unique_ptr<rtc::Buffer> modified_buffer_;
On 2016/05/30 08:57:23, språng wrote:
> On 2016/05/27 20:56:59, stefan-webrtc (holmer) wrote:
> > On 2016/05/27 13:12:21, språng wrote:
> > > On 2016/05/26 18:08:13, stefan-webrtc (holmer) wrote:
> > > > On 2016/05/25 09:06:03, språng wrote:
> > > > > On 2016/05/22 23:11:50, stefan-webrtc (holmer) wrote:
> > > > > > Did you consider propagating the copy on modify buffer all the way
in
> > from
> > > > the
> > > > > > network layers instead?
> > > > > 
> > > > > Yes, but that turned out to be a bit more work than I'd like - and
> > something
> > > > > mflodman@ has already begun working on but hasn't finished yet. We can
> > > revisit
> > > > > this at that time instead, I'll add a TODO.
> > > > 
> > > > Acknowledged.
> > > > 
> > > > > 
> > > > > > Are we sure that a second packet won't overwrite this buffer while
the
> > > first
> > > > > > packet is still being processed? Should that be ensured by this
class?
> > > Seems
> > > > a
> > > > > > bit brittle to leave that to the caller.
> > > > > 
> > > > > I've added an assert that we don't have two SPS blocks, that both
needs
> to
> > > be
> > > > > modified, packet together within a single RTP packet using STAP-A.
That
> > > should
> > > > > really not happen.
> > > > 
> > > > That's not what I was thinking of. What if the first packet is parsed
and
> > sent
> > > > off to the jitter buffer, but before the jitter buffer is done with it a
> new
> > > > packet comes in which also needs to be modified, effectively overwriting
> the
> > > > first packet.
> > > > 
> > > > I think that will become increasingly likely with philipel's new jitter
> > buffer
> > > > which doesn't copy the packets until they are passed to the decoder, see
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc....
> > > > 
> > > > I think we'll need a better way of handling this... The best way would
> > really
> > > be
> > > > to have a copy-on-modify, or if we could simply pass on packet
ownership?
> > > 
> > > I've talked to Philip about this. We'll need a copy somewhere anyway, to
> hand
> > > ownership over to the jitter buffer, and this seems like the right place.
> For
> > > now, let's keep this and then in a follow up CL, have ParsedPayload
contain
> a
> > > unique_ptr with a copy of the parsed buffer. Then we can std::move that
into
> > the
> > > jitter buffer making ownership clear, and any codec specific
pre-processing
> > > needed can take place in the depacketizer instead of inside the jitter
> buffer,
> > > which is the case today.
> > 
> > Yes, a copy will be needed, at least for the near future. It's possible to
> pass
> > ownership of the packet all the way from the socket to the jitter buffer
> though.
> > 
> > But if we keep this for now, does that mean we're making the assumption that
> we
> > won't run into the risk of races when one packet is on its way to the JB
(but
> > hasn't been copied yet) and another packet with an sps to be modified
arrives?
> > Is that safe? If you think it is I think you at least have to document it.
Or
> > should we add a lock or something for now to ensure it's not an issue?
> 
> The incoming packet goes out of scope when the IncomingRtpPacket() callback
> returns, so the jitter buffer can really only use it directly if it's a single
> packet frame that's received. Otherwise a copy will be needed somewhere even
in
> the future; it can never just hold on to the pointer without being racy.

I was thinking that ownership would be passed on to the jitter buffer. Would
that not be possible? Not sure if it would make sense, but you could for
instance have a pool of packets at receiver and handle it similarly to how we
handle frames.

> 
> After discussing with with philipel@, we agreed that _always_ cloning incoming
> payload in the depackterizer is the right thing to do. Not only for H264, but
> all codecs. I'll add a comment to this effect in the RtpDepacketizer
interface.

But if we do that now we'll be doing two memcpys for all packets with the old
jitter buffer, since the old jitter buffer does this memcpy already. We should
only do it for all packets with the new one, right?

[sprang_webrtc, 2016-05-30 12:39:30]

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.h (right):

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.h:121:
std::unique_ptr<rtc::Buffer> modified_buffer_;
On 2016/05/30 11:00:55, stefan-webrtc (holmer) wrote:
> On 2016/05/30 08:57:23, språng wrote:
> > On 2016/05/27 20:56:59, stefan-webrtc (holmer) wrote:
> > > On 2016/05/27 13:12:21, språng wrote:
> > > > On 2016/05/26 18:08:13, stefan-webrtc (holmer) wrote:
> > > > > On 2016/05/25 09:06:03, språng wrote:
> > > > > > On 2016/05/22 23:11:50, stefan-webrtc (holmer) wrote:
> > > > > > > Did you consider propagating the copy on modify buffer all the way
> in
> > > from
> > > > > the
> > > > > > > network layers instead?
> > > > > > 
> > > > > > Yes, but that turned out to be a bit more work than I'd like - and
> > > something
> > > > > > mflodman@ has already begun working on but hasn't finished yet. We
can
> > > > revisit
> > > > > > this at that time instead, I'll add a TODO.
> > > > > 
> > > > > Acknowledged.
> > > > > 
> > > > > > 
> > > > > > > Are we sure that a second packet won't overwrite this buffer while
> the
> > > > first
> > > > > > > packet is still being processed? Should that be ensured by this
> class?
> > > > Seems
> > > > > a
> > > > > > > bit brittle to leave that to the caller.
> > > > > > 
> > > > > > I've added an assert that we don't have two SPS blocks, that both
> needs
> > to
> > > > be
> > > > > > modified, packet together within a single RTP packet using STAP-A.
> That
> > > > should
> > > > > > really not happen.
> > > > > 
> > > > > That's not what I was thinking of. What if the first packet is parsed
> and
> > > sent
> > > > > off to the jitter buffer, but before the jitter buffer is done with it
a
> > new
> > > > > packet comes in which also needs to be modified, effectively
overwriting
> > the
> > > > > first packet.
> > > > > 
> > > > > I think that will become increasingly likely with philipel's new
jitter
> > > buffer
> > > > > which doesn't copy the packets until they are passed to the decoder,
see
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc....
> > > > > 
> > > > > I think we'll need a better way of handling this... The best way would
> > > really
> > > > be
> > > > > to have a copy-on-modify, or if we could simply pass on packet
> ownership?
> > > > 
> > > > I've talked to Philip about this. We'll need a copy somewhere anyway, to
> > hand
> > > > ownership over to the jitter buffer, and this seems like the right
place.
> > For
> > > > now, let's keep this and then in a follow up CL, have ParsedPayload
> contain
> > a
> > > > unique_ptr with a copy of the parsed buffer. Then we can std::move that
> into
> > > the
> > > > jitter buffer making ownership clear, and any codec specific
> pre-processing
> > > > needed can take place in the depacketizer instead of inside the jitter
> > buffer,
> > > > which is the case today.
> > > 
> > > Yes, a copy will be needed, at least for the near future. It's possible to
> > pass
> > > ownership of the packet all the way from the socket to the jitter buffer
> > though.
> > > 
> > > But if we keep this for now, does that mean we're making the assumption
that
> > we
> > > won't run into the risk of races when one packet is on its way to the JB
> (but
> > > hasn't been copied yet) and another packet with an sps to be modified
> arrives?
> > > Is that safe? If you think it is I think you at least have to document it.
> Or
> > > should we add a lock or something for now to ensure it's not an issue?
> > 
> > The incoming packet goes out of scope when the IncomingRtpPacket() callback
> > returns, so the jitter buffer can really only use it directly if it's a
single
> > packet frame that's received. Otherwise a copy will be needed somewhere even
> in
> > the future; it can never just hold on to the pointer without being racy.
> 
> I was thinking that ownership would be passed on to the jitter buffer. Would
> that not be possible? Not sure if it would make sense, but you could for
> instance have a pool of packets at receiver and handle it similarly to how we
> handle frames.

Then I don't think CopyOnWriteBuffer is the way to go, either std::unique_ptr or
something ref-counted.
In either case, that's a lot of plumbing. I think that should be done in
conjunction with enabling the new jitterbuffer instead of this CL.

> > 
> > After discussing with with philipel@, we agreed that _always_ cloning
incoming
> > payload in the depackterizer is the right thing to do. Not only for H264,
but
> > all codecs. I'll add a comment to this effect in the RtpDepacketizer
> interface.
> 
> But if we do that now we'll be doing two memcpys for all packets with the old
> jitter buffer, since the old jitter buffer does this memcpy already. We should
> only do it for all packets with the new one, right?
> 

Yes, this needs to be enabled in conjunction with enabling the new jitter
buffer, so it's something that should be done in this CL. Copying h264 packets
with a suboptimal sps in it should be fine though.

[danilchap, 2016-05-30 12:55:59]

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.h (right):

https://codereview.webrtc.org/1979443004/diff/110001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.h:121:
std::unique_ptr<rtc::Buffer> modified_buffer_;
On 2016/05/30 12:39:30, språng wrote:
> On 2016/05/30 11:00:55, stefan-webrtc (holmer) wrote:
> > On 2016/05/30 08:57:23, språng wrote:
> > > On 2016/05/27 20:56:59, stefan-webrtc (holmer) wrote:
> > > > On 2016/05/27 13:12:21, språng wrote:
> > > > > On 2016/05/26 18:08:13, stefan-webrtc (holmer) wrote:
> > > > > > On 2016/05/25 09:06:03, språng wrote:
> > > > > > > On 2016/05/22 23:11:50, stefan-webrtc (holmer) wrote:
> > > > > > > > Did you consider propagating the copy on modify buffer all the
way
> > in
> > > > from
> > > > > > the
> > > > > > > > network layers instead?
> > > > > > > 
> > > > > > > Yes, but that turned out to be a bit more work than I'd like - and
> > > > something
> > > > > > > mflodman@ has already begun working on but hasn't finished yet. We
> can
> > > > > revisit
> > > > > > > this at that time instead, I'll add a TODO.
> > > > > > 
> > > > > > Acknowledged.
> > > > > > 
> > > > > > > 
> > > > > > > > Are we sure that a second packet won't overwrite this buffer
while
> > the
> > > > > first
> > > > > > > > packet is still being processed? Should that be ensured by this
> > class?
> > > > > Seems
> > > > > > a
> > > > > > > > bit brittle to leave that to the caller.
> > > > > > > 
> > > > > > > I've added an assert that we don't have two SPS blocks, that both
> > needs
> > > to
> > > > > be
> > > > > > > modified, packet together within a single RTP packet using STAP-A.
> > That
> > > > > should
> > > > > > > really not happen.
> > > > > > 
> > > > > > That's not what I was thinking of. What if the first packet is
parsed
> > and
> > > > sent
> > > > > > off to the jitter buffer, but before the jitter buffer is done with
it
> a
> > > new
> > > > > > packet comes in which also needs to be modified, effectively
> overwriting
> > > the
> > > > > > first packet.
> > > > > > 
> > > > > > I think that will become increasingly likely with philipel's new
> jitter
> > > > buffer
> > > > > > which doesn't copy the packets until they are passed to the decoder,
> see
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc....
> > > > > > 
> > > > > > I think we'll need a better way of handling this... The best way
would
> > > > really
> > > > > be
> > > > > > to have a copy-on-modify, or if we could simply pass on packet
> > ownership?
> > > > > 
> > > > > I've talked to Philip about this. We'll need a copy somewhere anyway,
to
> > > hand
> > > > > ownership over to the jitter buffer, and this seems like the right
> place.
> > > For
> > > > > now, let's keep this and then in a follow up CL, have ParsedPayload
> > contain
> > > a
> > > > > unique_ptr with a copy of the parsed buffer. Then we can std::move
that
> > into
> > > > the
> > > > > jitter buffer making ownership clear, and any codec specific
> > pre-processing
> > > > > needed can take place in the depacketizer instead of inside the jitter
> > > buffer,
> > > > > which is the case today.
> > > > 
> > > > Yes, a copy will be needed, at least for the near future. It's possible
to
> > > pass
> > > > ownership of the packet all the way from the socket to the jitter buffer
> > > though.
> > > > 
> > > > But if we keep this for now, does that mean we're making the assumption
> that
> > > we
> > > > won't run into the risk of races when one packet is on its way to the JB
> > (but
> > > > hasn't been copied yet) and another packet with an sps to be modified
> > arrives?
> > > > Is that safe? If you think it is I think you at least have to document
it.
> > Or
> > > > should we add a lock or something for now to ensure it's not an issue?
> > > 
> > > The incoming packet goes out of scope when the IncomingRtpPacket()
callback
> > > returns, so the jitter buffer can really only use it directly if it's a
> single
> > > packet frame that's received. Otherwise a copy will be needed somewhere
even
> > in
> > > the future; it can never just hold on to the pointer without being racy.
> > 
> > I was thinking that ownership would be passed on to the jitter buffer. Would
> > that not be possible? Not sure if it would make sense, but you could for
> > instance have a pool of packets at receiver and handle it similarly to how
we
> > handle frames.
> 
> Then I don't think CopyOnWriteBuffer is the way to go, either std::unique_ptr
or
> something ref-counted.

CopyOnWriteBuffer is actually a ref-counted rtc::Buffer underneath.
(with an extra feature: copy on write if needed, i.e. ref_counter > 1).

[stefan-webrtc, 2016-05-30 12:56:35]

lgtm after offline discussion.

[sprang_webrtc, 2016-05-30 13:20:57]

sprang@webrtc.org changed reviewers:
+ mflodman@webrtc.org

[sprang_webrtc, 2016-05-30 13:20:58]

+mflodman for webrtc/base

[mflodman, 2016-05-31 07:52:44]

webrtc/base LGTM

[sprang_webrtc, 2016-05-31 20:24:59]

Ping, Noah?

[noahric, 2016-06-01 17:56:43]

LGTM % some little comments.

https://codereview.webrtc.org/1979443004/diff/250001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right):

https://codereview.webrtc.org/1979443004/diff/250001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:121:
output_buffer->AppendData(buffer[0]);
Ah, I see. It wasn't an existing bug, it's just the trickiness about which
things want the header or not.

Consider (around line 97) adding a few extra variables.
uint8_t nalu_header = buffer[0];
H264::NaluType nalu_type = H264::ParseNaluType(nalu_header);
if (sps) {
  const uint8_t* payload = buffer + H264::kNaluTypeSize;
  const size_t payload_length = length - H264::kNaluTypeSize;

and then when you use those later on, it may be cleaner. e.g.
std::unique_ptr<rtc::Buffer> rbsp_buffer = H264::ParseRbsp(payload,
payload_length);

Same with this append (appending nalu_header instead of buffer[0]).

https://codereview.webrtc.org/1979443004/diff/270001/webrtc/common_video/h264...
File webrtc/common_video/h264/h264_common.cc (right):

https://codereview.webrtc.org/1979443004/diff/270001/webrtc/common_video/h264...
webrtc/common_video/h264/h264_common.cc:86: // Writes bytes into RBSP, adding
emulation (0x03) bytes where necessary.
Could you either quote the section of the standard or add some more information
about "where necessary"?

Basically:
// The following sequences are illegal, and need to be replaced with:
// 00 00 00 -> 00 00 03 00
// 00 00 01 -> 00 00 03 01
// 00 00 02 -> 00 00 03 02
// And things in the source that look like the emulation byte pattern (00 00 03)
// need to have an extra emulation byte added:
// 00 00 03 -> 00 00 03 03

https://codereview.webrtc.org/1979443004/diff/270001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right):

https://codereview.webrtc.org/1979443004/diff/270001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:109: //
RtpDepacketizerH264::ParseSingleNalu (receive side, in oter to protect
order

https://codereview.webrtc.org/1979443004/diff/270001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:110: // us from legacy send
clients).
maybe "unknown or legacy", since the other side may not be this implementation
of webrtc.

https://codereview.webrtc.org/1979443004/diff/270001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:134:
RTC_HISTOGRAM_ENUMERATION(kSpsValidHistogramName,
Consider adding histograms for the various states: POC type correct, POC type
wrong but VUI already present (not sure if that's returned anywhere), POC type
rewritten, parse failure.

Parse failure is the only one not captured here. We're hoping it never happens,
but the histogram would help confirm that.

https://codereview.webrtc.org/1979443004/diff/270001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:440: }
consider else histogram for failed parsing

[sprang_webrtc, 2016-06-02 08:53:15]

https://codereview.webrtc.org/1979443004/diff/270001/webrtc/common_video/h264...
File webrtc/common_video/h264/h264_common.cc (right):

https://codereview.webrtc.org/1979443004/diff/270001/webrtc/common_video/h264...
webrtc/common_video/h264/h264_common.cc:86: // Writes bytes into RBSP, adding
emulation (0x03) bytes where necessary.
On 2016/06/01 17:56:43, noahric wrote:
> Could you either quote the section of the standard or add some more
information
> about "where necessary"?
> 
> Basically:
> // The following sequences are illegal, and need to be replaced with:
> // 00 00 00 -> 00 00 03 00
> // 00 00 01 -> 00 00 03 01
> // 00 00 02 -> 00 00 03 02
> // And things in the source that look like the emulation byte pattern (00 00
03)
> // need to have an extra emulation byte added:
> // 00 00 03 -> 00 00 03 03

Added comments in the header file.

https://codereview.webrtc.org/1979443004/diff/270001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc (right):

https://codereview.webrtc.org/1979443004/diff/270001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:109: //
RtpDepacketizerH264::ParseSingleNalu (receive side, in oter to protect
On 2016/06/01 17:56:43, noahric wrote:
> order

Done.

https://codereview.webrtc.org/1979443004/diff/270001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:110: // us from legacy send
clients).
On 2016/06/01 17:56:43, noahric wrote:
> maybe "unknown or legacy", since the other side may not be this implementation
> of webrtc.

Done.

https://codereview.webrtc.org/1979443004/diff/270001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:134:
RTC_HISTOGRAM_ENUMERATION(kSpsValidHistogramName,
On 2016/06/01 17:56:43, noahric wrote:
> Consider adding histograms for the various states: POC type correct, POC type
> wrong but VUI already present (not sure if that's returned anywhere), POC type
> rewritten, parse failure.
> 
> Parse failure is the only one not captured here. We're hoping it never
happens,
> but the histogram would help confirm that.

Done.

https://codereview.webrtc.org/1979443004/diff/270001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_h264.cc:440: }
On 2016/06/01 17:56:43, noahric wrote:
> consider else histogram for failed parsing

Done.

[sprang_webrtc, 2016-06-02 09:41:21]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2016-06-02 09:41:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
noahric@chromium.org, stefan@webrtc.org, mflodman@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1979443004/#ps290001 (title:
"Addressed comments")

[commit-bot: I haz the power, 2016-06-02 09:41:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1979443004/290001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1979443004/290001

[commit-bot: I haz the power, 2016-06-02 09:43:36]

Committed patchset #16 (id:290001)

[commit-bot: I haz the power, 2016-06-02 09:43:46]

Description was changed from

==========
Add H264 bitstream rewriting to limit frame reordering marker in header

The VUI part an SPS may specify max_num_reorder_frames and
max_dec_frame_buffering. These may cause a decoder to buffer a number
of frame prior allowing decode, leading to delays, even if no frames
using such references (ie B-frames) are sent.

Because of this we update any SPS block emitted by the encoder.

Also, a bunch of refactoring of H264-related code to reduce code
duplication.

BUG=
==========

to

==========
Add H264 bitstream rewriting to limit frame reordering marker in header

The VUI part an SPS may specify max_num_reorder_frames and
max_dec_frame_buffering. These may cause a decoder to buffer a number
of frame prior allowing decode, leading to delays, even if no frames
using such references (ie B-frames) are sent.

Because of this we update any SPS block emitted by the encoder.

Also, a bunch of refactoring of H264-related code to reduce code
duplication.

BUG=

Committed: https://crrev.com/52033d6ea119223179cc0813f95ff31a21c3ac72
Cr-Commit-Position: refs/heads/master@{#13010}
==========

[commit-bot: I haz the power, 2016-06-02 09:43:48]

Patchset 16 (id:??) landed as
https://crrev.com/52033d6ea119223179cc0813f95ff31a21c3ac72
Cr-Commit-Position: refs/heads/master@{#13010}