Bug 1294407: Clean up H264 STAP-A handling r=pkerr a=abillings
authorRandell Jesup <rjesup@jesup.org>
Wed, 17 Aug 2016 16:31:58 -0400
changeset 342371 8ea0c38aae0b5cdb76bc32f59938424fbde469df
parent 342370 27e2b6774c007e81482176828283c9ee1d2c2960
child 342372 91505c2a68fb868bcab1e3dc03ffc2e89bf71cac
push id1183
push userraliiev@mozilla.com
push dateMon, 05 Sep 2016 20:01:49 +0000
treeherdermozilla-release@3148731bed45 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspkerr, abillings
bugs1294407
milestone49.0
Bug 1294407: Clean up H264 STAP-A handling r=pkerr a=abillings MozReview-Commit-ID: 2WkvJiQvmey
media/webrtc/trunk/webrtc/modules/video_coding/main/source/frame_buffer.cc
media/webrtc/trunk/webrtc/modules/video_coding/main/source/jitter_buffer_common.h
media/webrtc/trunk/webrtc/modules/video_coding/main/source/session_info.cc
--- a/media/webrtc/trunk/webrtc/modules/video_coding/main/source/frame_buffer.cc
+++ b/media/webrtc/trunk/webrtc/modules/video_coding/main/source/frame_buffer.cc
@@ -108,18 +108,21 @@ VCMFrameBuffer::InsertPacket(const VCMPa
         ntp_time_ms_ = packet.ntp_time_ms_;
         _codec = packet.codec;
         if (packet.frameType != kFrameEmpty) {
             // first media packet
             SetState(kStateIncomplete);
         }
     }
 
+    // add safety margin because STAP-A packets can cause it to expand by
+    // ~two bytes per NAL
     uint32_t requiredSizeBytes = Length() + packet.sizeBytes +
-                   (packet.insertStartCode ? kH264StartCodeLengthBytes : 0);
+                   (packet.insertStartCode ? kH264StartCodeLengthBytes : 0) +
+                                 kBufferSafetyMargin;
     if (requiredSizeBytes >= _size) {
         const uint8_t* prevBuffer = _buffer;
         const uint32_t increments = requiredSizeBytes /
                                           kBufferIncStepSizeBytes +
                                         (requiredSizeBytes %
                                          kBufferIncStepSizeBytes > 0);
         const uint32_t newSize = _size +
                                        increments * kBufferIncStepSizeBytes;
--- a/media/webrtc/trunk/webrtc/modules/video_coding/main/source/jitter_buffer_common.h
+++ b/media/webrtc/trunk/webrtc/modules/video_coding/main/source/jitter_buffer_common.h
@@ -25,17 +25,18 @@ enum { kMaxVideoDelayMs       = 10000 };
 enum { kPacketsPerFrameMultiplier = 5 };
 enum { kFastConvergeThreshold = 5};
 
 enum VCMJitterBufferEnum {
   kMaxConsecutiveOldFrames        = 60,
   kMaxConsecutiveOldPackets       = 300,
   kMaxPacketsInSession            = 800,
   kBufferIncStepSizeBytes         = 30000,   // >20 packets.
-  kMaxJBFrameSizeBytes            = 4000000  // sanity don't go above 4Mbyte.
+  kMaxJBFrameSizeBytes            = 4000000, // sanity don't go above 4Mbyte.
+  kBufferSafetyMargin             = 100      // enough for ~50 NALs in a STAP-A
 };
 
 enum VCMFrameBufferEnum {
   kOutOfBoundsPacket    = -7,
   kNotInitialized       = -6,
   kOldPacket            = -5,
   kGeneralError         = -4,
   kFlushIndicator       = -3,   // Indicator that a flush has occurred.
--- a/media/webrtc/trunk/webrtc/modules/video_coding/main/source/session_info.cc
+++ b/media/webrtc/trunk/webrtc/modules/video_coding/main/source/session_info.cc
@@ -173,26 +173,41 @@ size_t VCMSessionInfo::InsertBuffer(uint
   // We handle H.264 STAP-A packets in a special way as we need to remove the
   // two length bytes between each NAL unit, and potentially add start codes.
   const size_t kH264NALHeaderLengthInBytes = 1;
   const size_t kLengthFieldLength = 2;
   if (packet.codecSpecificHeader.codec == kRtpVideoH264 &&
       packet.codecSpecificHeader.codecHeader.H264.stap_a) {
     size_t required_length = 0;
     const uint8_t* nalu_ptr = packet_buffer + kH264NALHeaderLengthInBytes;
-    while (nalu_ptr < packet_buffer + packet.sizeBytes) {
+    // Must check that incoming data length doesn't extend past end of buffer.
+    // We allow for 100 bytes of expansion due to startcodes being longer than
+    // length fields.
+    while (nalu_ptr + kLengthFieldLength <= packet_buffer + packet.sizeBytes) {
       size_t length = BufferToUWord16(nalu_ptr);
-      required_length +=
+      if (nalu_ptr + kLengthFieldLength + length <= packet_buffer + packet.sizeBytes) {
+        required_length +=
           length + (packet.insertStartCode ? kH264StartCodeLengthBytes : 0);
-      nalu_ptr += kLengthFieldLength + length;
+        nalu_ptr += kLengthFieldLength + length;
+      } else {
+        // Something is very wrong!
+        LOG(LS_ERROR) << "Failed to insert packet due to corrupt H264 STAP-A";
+        return 0;
+      }
     }
+    if (required_length > packet.sizeBytes + kBufferSafetyMargin) {
+      LOG(LS_ERROR) << "Failed to insert packet due to too many NALs in a STAP-A";
+      return 0;
+    }
+
     ShiftSubsequentPackets(packet_it, required_length);
     nalu_ptr = packet_buffer + kH264NALHeaderLengthInBytes;
     uint8_t* frame_buffer_ptr = frame_buffer + offset;
-    while (nalu_ptr < packet_buffer + packet.sizeBytes) {
+    // we already know we won't go past end-of-buffer
+    while (nalu_ptr + kLengthFieldLength <= packet_buffer + packet.sizeBytes) {
       size_t length = BufferToUWord16(nalu_ptr);
       nalu_ptr += kLengthFieldLength;
       frame_buffer_ptr += Insert(nalu_ptr,
                                  length,
                                  packet.insertStartCode,
                                  const_cast<uint8_t*>(frame_buffer_ptr));
       nalu_ptr += length;
     }