Add H264 bitstream rewriting to limit frame reordering marker in header

webrtc/modules/rtp_rtcp/source/h264/bitstream_rewriter.h

+/*
+ *  Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ *
+ */
+
+#ifndef WEBRTC_MODULES_RTP_RTCP_SOURCE_H264_BITSTREAM_REWRITER_H_
+#define WEBRTC_MODULES_RTP_RTCP_SOURCE_H264_BITSTREAM_REWRITER_H_
+
+#include "webrtc/base/optional.h"
+#include "webrtc/modules/rtp_rtcp/source/h264/sps_parser.h"
+
+namespace rtc {
+class BitBuffer;
+class BitBufferWriter;
+class ByteBufferWriter;
+}
+
+namespace webrtc {
+
+// A class that copies H264 data from a source to a destination and rewrites

[noahric, 2016/05/18 01:35:06]

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.

[sprang_webrtc, 2016/05/20 16:10:59]

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.

+// the bitstream in the process to allow for faster decoding for streams that
+// use picture order count type 0. Streams in that format incur additional delay
+// because it allows decode order to differ from render order.
+// The mechanism used is to rewrite (edit or add) the SPS's VUI to contain
+// restrictions on the maximum number of reordered pictures. This reduces
+// latency significantly, though it still adds about a frame of latency to
+// decoding.
+class H264BitstreamRewriter {
+ public:
+  H264BitstreamRewriter() {}
+  // Parses an SPS NALU and if necessary copies it and rewrites the VUI.
+  // Returns true on success, and if so sets output_buffer_ to the processed
+  // payload. Otherwise return false and leaves output_buffer_ unchanged.
+  // Buffer assumes NALU header (first 4 bytes) has already been parsed and is
+  // NOT part of this buffer.
+  bool ParseAndRewriteSps(const uint8_t* buffer, size_t length);

[noahric, 2016/05/18 01:35:06]

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?).

[sprang_webrtc, 2016/05/20 16:10:59]

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.

+
+  rtc::Optional<SpsParser::SpsState> sps_state_;
+  std::unique_ptr<rtc::ByteBufferWriter> output_buffer_;
+
+ private:
+  bool CopyHrdParameters(rtc::BitBuffer* source,

[noahric, 2016/05/18 01:35:06]

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.

[sprang_webrtc, 2016/05/20 16:10:59]

Done.

+                         rtc::BitBufferWriter* destination);
+  bool AddBitstreamRestriction(rtc::BitBufferWriter* destination,
+                               uint32_t max_num_ref_frames);
+  bool CopyRemainingBits(rtc::BitBuffer* source,
+                         rtc::BitBufferWriter* destination);
+  bool CopyAndRewriteVui(rtc::BitBuffer* source,
+                         rtc::BitBufferWriter* destination,
+                         bool* out_vui_rewritten);
+};
+
+}  // namespace webrtc
+
+#endif  // WEBRTC_MODULES_RTP_RTCP_SOURCE_H264_BITSTREAM_REWRITER_H_