Add extension iterator.

Rather than repeating the code to iterate through extensions in
 three different places, each with slight differences, different
 edge cases, different error handling, etc., create an iterator
 that can be used everywhere.

src/extensions.c

@@ -38,7 +38,8 @@
 
 /* Given an extension payload, advance data to the next extension and return the
    length of the remaining extensions. */
-opus_int32 skip_extension(const unsigned char **data, opus_int32 len, opus_int32 *header_size)
+static opus_int32 skip_extension(const unsigned char **data, opus_int32 len,
+ opus_int32 *header_size)
 {
    int id, L;
    if (len==0)
@@ -91,95 +92,114 @@ opus_int32 skip_extension(const unsigned char **data, opus_int32 len, opus_int32
    }
 }
 
+void opus_extension_iterator_init(OpusExtensionIterator *iter,
+ const unsigned char *data, opus_int32 len) {
+   celt_assert(len >= 0);
+   celt_assert(data != NULL || len == 0);
+   iter->curr_data = iter->data = data;
+   iter->curr_len = iter->len = len;
+   iter->curr_frame = 0;
+}
+
+/* Reset the iterator so it can start iterating again from the first
+    extension. */
+void opus_extension_iterator_reset(OpusExtensionIterator *iter) {
+   iter->curr_data = iter->data;
+   iter->curr_len = iter->len;
+   iter->curr_frame = 0;
+}
+
+/* Return the next extension (excluding real padding and separators). */
+int opus_extension_iterator_next(OpusExtensionIterator *iter,
+ opus_extension_data *ext) {
+   opus_int32 header_size;
+   if (iter->curr_len < 0) {
+      return OPUS_INVALID_PACKET;
+   }
+   while (iter->curr_len > 0) {
+      const unsigned char *curr_data0;
+      int id;
+      int L;
+      curr_data0 = iter->curr_data;
+      id = *curr_data0>>1;
+      L = *curr_data0&1;
+      iter->curr_len = skip_extension(&iter->curr_data, iter->curr_len,
+       &header_size);
+      if (iter->curr_len < 0) {
+         return OPUS_INVALID_PACKET;
+      }
+      celt_assert(iter->curr_data - iter->data == iter->len - iter->curr_len);
+      if (id == 1) {
+         if (L == 0) {
+            iter->curr_frame++;
+         }
+         else {
+            iter->curr_frame += curr_data0[1];
+         }
+         if (iter->curr_frame >= 48) {
+            iter->curr_len = -1;
+            return OPUS_INVALID_PACKET;
+         }
+      }
+      else if (id > 1) {
+         if (ext != NULL) {
+            ext->id = id;
+            ext->frame = iter->curr_frame;
+            ext->data = curr_data0 + header_size;
+            ext->len = iter->curr_data - curr_data0 - header_size;
+         }
+         return 1;
+      }
+   }
+   return 0;
+}
+
+int opus_extension_iterator_find(OpusExtensionIterator *iter,
+ opus_extension_data *ext, int id) {
+   opus_extension_data curr_ext;
+   int ret;
+   for(;;) {
+      ret = opus_extension_iterator_next(iter, &curr_ext);
+      if (ret <= 0) {
+         return ret;
+      }
+      if (curr_ext.id == id) {
+         *ext = curr_ext;
+         return ret;
+      }
+   }
+}
+
 /* Count the number of extensions, excluding real padding and separators. */
 opus_int32 opus_packet_extensions_count(const unsigned char *data, opus_int32 len)
 {
-   opus_int32 curr_len;
+   OpusExtensionIterator iter;
-   opus_int32 count=0;
+   int count;
-   const unsigned char *curr_data = data;
+   opus_extension_iterator_init(&iter, data, len);
-
+   for (count=0; opus_extension_iterator_next(&iter, NULL) > 0; count++);
-   celt_assert(len >= 0);
-   celt_assert(data != NULL || len == 0);
-
-   curr_len = len;
-   while (curr_len > 0)
-   {
-      int id;
-      opus_int32 header_size;
-      id = *curr_data>>1;
-      curr_len = skip_extension(&curr_data, curr_len, &header_size);
-      if (curr_len < 0)
-         return OPUS_INVALID_PACKET;
-      if (id > 1)
-         count++;
-   }
    return count;
 }
 
 /* Extract extensions from Opus padding (excluding real padding and separators) */
 opus_int32 opus_packet_extensions_parse(const unsigned char *data, opus_int32 len, opus_extension_data *extensions, opus_int32 *nb_extensions)
 {
-   const unsigned char *curr_data;
+   OpusExtensionIterator iter;
-   opus_int32 curr_len;
+   int count;
-   int curr_frame=0;
+   int ret;
-   opus_int32 count=0;
-
-   celt_assert(len >= 0);
-   celt_assert(data != NULL || len == 0);
    celt_assert(nb_extensions != NULL);
    celt_assert(extensions != NULL || *nb_extensions == 0);
-
+   opus_extension_iterator_init(&iter, data, len);
-   curr_data = data;
+   for (count=0;; count++) {
-   curr_len = len;
+      opus_extension_data ext;
-   while (curr_len > 0)
+      ret = opus_extension_iterator_next(&iter, &ext);
-   {
+      if (ret <= 0) break;
-      int id;
+      if (count == *nb_extensions) {
-      opus_int32 header_size;
-      opus_extension_data curr_ext;
-      id = *curr_data>>1;
-      if (id > 1)
-      {
-         curr_ext.id = id;
-         curr_ext.frame = curr_frame;
-         curr_ext.data = curr_data;
-      } else if (id == 1)
-      {
-         int L = *curr_data&1;
-         if (L==0)
-            curr_frame++;
-         else {
-            if (curr_len >= 2)
-               curr_frame += curr_data[1];
-            /* Else we're at the end and it doesn't matter. */
-         }
-         if (curr_frame >= 48)
-         {
-            *nb_extensions = count;
-            return OPUS_INVALID_PACKET;
-         }
-      }
-      curr_len = skip_extension(&curr_data, curr_len, &header_size);
-      /* printf("curr_len = %d, header_size = %d\n", curr_len, header_size); */
-      if (curr_len < 0)
-      {
-         *nb_extensions = count;
-         return OPUS_INVALID_PACKET;
-      }
-      celt_assert(curr_data - data == len - curr_len);
-      if (id > 1)
-      {
-         if (count == *nb_extensions)
-         {
          return OPUS_BUFFER_TOO_SMALL;
       }
-         curr_ext.len = curr_data - curr_ext.data - header_size;
+      extensions[count] = ext;
-         curr_ext.data += header_size;
-         extensions[count++] = curr_ext;
    }
-   }
-   celt_assert(curr_len == 0);
    *nb_extensions = count;
-   return OPUS_OK;
+   return ret;
 }
 
 opus_int32 opus_packet_extensions_generate(unsigned char *data, opus_int32 len, const opus_extension_data  *extensions, opus_int32 nb_extensions, int pad)

src/opus_decoder.c

@@ -1296,70 +1296,49 @@ bad_arg:
 #ifdef ENABLE_DRED
 static int dred_find_payload(const unsigned char *data, opus_int32 len, const unsigned char **payload, int *dred_frame_offset)
 {
-   const unsigned char *data0;
+   OpusExtensionIterator iter;
-   opus_int32 len0;
+   opus_extension_data ext;
-   int frame = 0;
+   const unsigned char *padding;
+   opus_int32 padding_len;
    int nb_frames;
    const unsigned char *frames[48];
    opus_int16 size[48];
    int frame_size;
+   int ret;
 
    *payload = NULL;
    /* Get the padding section of the packet. */
-   nb_frames = opus_packet_parse_impl(data, len, 0, NULL, frames, size, NULL, NULL, &data0, &len0);
+   ret = opus_packet_parse_impl(data, len, 0, NULL, frames, size, NULL, NULL,
-   if (nb_frames < 0)
+    &padding, &padding_len);
-      return nb_frames;
+   if (ret < 0)
+      return ret;
+   nb_frames = ret;
    frame_size = opus_packet_get_samples_per_frame(data, 48000);
-   data = data0;
+   opus_extension_iterator_init(&iter, padding, padding_len);
-   len = len0;
+   for (;;) {
-   /* Scan extensions in order until we find the earliest frame with DRED data. */
+      ret = opus_extension_iterator_find(&iter, &ext, DRED_EXTENSION_ID);
-   while (len > 0)
+      if (ret <= 0)
-   {
+         return ret;
-      opus_int32 header_size;
+      if (ext.frame >= nb_frames)
-      int id, L;
-      len0 = len;
-      data0 = data;
-      id = *data0 >> 1;
-      L = *data0 & 0x1;
-      len = skip_extension(&data, len, &header_size);
-      if (len < 0)
          break;
-      if (id == 1)
-      {
-         if (L==0)
-         {
-            frame++;
-         } else {
-            frame += data0[1];
-         }
-         if (frame >= nb_frames) {
-           break;
-         }
-      } else if (id == DRED_EXTENSION_ID)
-      {
-         const unsigned char *curr_payload;
-         opus_int32 curr_payload_len;
-         curr_payload = data0+header_size;
-         curr_payload_len = (data-data0)-header_size;
       /* DRED position in the packet, in units of 2.5 ms like for the signaled DRED offset. */
-         *dred_frame_offset = frame*frame_size/120;
+      *dred_frame_offset = ext.frame*frame_size/120;
 #ifdef DRED_EXPERIMENTAL_VERSION
       /* Check that temporary extension type and version match.
          This check will be removed once extension is finalized. */
-         if (curr_payload_len > DRED_EXPERIMENTAL_BYTES && curr_payload[0] == 'D' && curr_payload[1] == DRED_EXPERIMENTAL_VERSION) {
+      if (ext.len > DRED_EXPERIMENTAL_BYTES && ext.data[0] == 'D'
-            *payload = curr_payload+2;
+       && ext.data[1] == DRED_EXPERIMENTAL_VERSION) {
-            return curr_payload_len-2;
+         *payload = ext.data+2;
+         return ext.len-2;
       }
 #else
-         if (curr_payload_len > 0) {
+      if (ext.len > 0) {
-            *payload = curr_payload;
+         *payload = ext.data;
-            return curr_payload_len;
+         return ext.len;
       }
 #endif
    }
 }
-   return 0;
-}
 #endif
 
 int opus_dred_get_size(void)

src/opus_private.h

@@ -46,6 +46,14 @@ struct OpusRepacketizer {
    opus_int32 padding_len[48];
 };
 
+typedef struct OpusExtensionIterator {
+   const unsigned char *data;
+   const unsigned char *curr_data;
+   opus_int32 len;
+   opus_int32 curr_len;
+   int curr_frame;
+} OpusExtensionIterator;
+
 typedef struct {
    int id;
    int frame;
@@ -53,6 +61,15 @@ typedef struct {
    opus_int32 len;
 } opus_extension_data;
 
+void opus_extension_iterator_init(OpusExtensionIterator *iter,
+ const unsigned char *data, opus_int32 len);
+
+void opus_extension_iterator_reset(OpusExtensionIterator *iter);
+int opus_extension_iterator_next(OpusExtensionIterator *iter,
+ opus_extension_data *ext);
+int opus_extension_iterator_find(OpusExtensionIterator *iter,
+ opus_extension_data *ext, int id);
+
 typedef struct ChannelLayout {
    int nb_channels;
    int nb_streams;
@@ -171,9 +188,6 @@ static OPUS_INLINE int align(int i)
     return ((i + alignment - 1) / alignment) * alignment;
 }
 
-/* More than that is ridiculous for now (3 * max frames per packet)*/
-opus_int32 skip_extension(const unsigned char **data, opus_int32 len, opus_int32 *header_size);
-
 int opus_packet_parse_impl(const unsigned char *data, opus_int32 len,
       int self_delimited, unsigned char *out_toc,
       const unsigned char *frames[48], opus_int16 size[48],

tests/test_opus_extensions.c

@@ -300,8 +300,10 @@ void test_extensions_parse_fail(void)
    nb_ext = 10;
    result = opus_packet_extensions_parse(packet, len, ext_out, &nb_ext);
    expect_true(result == OPUS_INVALID_PACKET, "expected OPUS_INVALID_PACKET");
+   /* note, opus_packet_extensions_count stops at the invalid frame increment
+      and tells us that we have 1 extension */
    result = opus_packet_extensions_count(packet, len);
-   expect_true(result == OPUS_INVALID_PACKET, "expected OPUS_INVALID_PACKET");
+   expect_true(result == 1, "expected opus_packet_extensions_count to return 1");
 
    /* create invalid frame increment */
    nb_ext = 10;
@@ -309,10 +311,10 @@ void test_extensions_parse_fail(void)
    packet[14] = 255;
    result = opus_packet_extensions_parse(packet, len, ext_out, &nb_ext);
    expect_true(result == OPUS_INVALID_PACKET, "expected OPUS_INVALID_PACKET");
-   /* note, opus_packet_extensions_count does not read the invalid frame increment
+   /* note, opus_packet_extensions_count stops at the invalid frame increment
-      and tells us that we have 4 extensions */
+      and tells us that we have 2 extensions */
    result = opus_packet_extensions_count(packet, len);
-   expect_true(result == 4, "expected opus_packet_extensions_count to return 4");
+   expect_true(result == 2, "expected opus_packet_extensions_count to return 2");
 
    /* not enough space */
    nb_ext = 1;