ref: 60d6c560383c56d7367fd1c5a3d20c7c0cfc71a7
parent: b49c07d1c460f13a1292723ca5dc38140a782a90
author: Timothy B. Terriberry <tterribe@xiph.org>
date: Sun Nov 3 21:29:40 EST 2024
Always use L=0 for last repeated long extension. Even if it is followed by repeated short extensions with payloads. We track the total size of the short extension payloads that need to be repeated, and remove that from the available space for the last long extension. This means we can no longer use L=0 on a repeat to skip coding a frame separator when the extensions to be repeated contain a long extension followed by one or more short extensions with payloads (and there are no more non-repeated extensions in the current frame, but there are extensions in the next frame), but this case seems uncommon, and hard to explain. The savings from always being able to skip coding a length when the final extensions are repeated extensions with at least one long extension is likely higher. We can still skip a frame separator if we repeat only short extensions. Also update existing tests and add a test for the case where we do not have enough space for the repeated short extensions after the last long extension. Signed-off-by: Jean-Marc Valin <jeanmarcv@google.com>
--- a/src/extensions.c
+++ b/src/extensions.c
@@ -43,7 +43,8 @@
the repeated extension payloads.
That requires higher-level logic. */
static opus_int32 skip_extension_payload(const unsigned char **pdata,
- opus_int32 len, opus_int32 *pheader_size, int id_byte)
+ opus_int32 len, opus_int32 *pheader_size, int id_byte,
+ opus_int32 trailing_short_len)
{const unsigned char *data;
opus_int32 header_size;
@@ -64,8 +65,9 @@
} else {if (L==0)
{- data += len;
- len = 0;
+ if (len < trailing_short_len) return -1;
+ data += len - trailing_short_len;
+ len = trailing_short_len;
} else {opus_int32 bytes=0;
opus_int32 lacing;
@@ -107,7 +109,7 @@
data = *pdata;
id_byte = *data++;
len--;
- len = skip_extension_payload(&data, len, pheader_size, id_byte);
+ len = skip_extension_payload(&data, len, pheader_size, id_byte, 0);
if (len >= 0) {*pdata = data;
(*pheader_size)++;
@@ -120,11 +122,11 @@
celt_assert(len >= 0);
celt_assert(data != NULL || len == 0);
celt_assert(nb_frames >= 0 && nb_frames <= 48);
- iter->repeat_data_end = iter->repeat_data = iter->curr_data = iter->data =
- data;
- iter->src_data = NULL;
+ iter->repeat_data = iter->curr_data = iter->data = data;
+ iter->last_long = iter->src_data = NULL;
iter->curr_len = iter->len = len;
iter->repeat_len = iter->src_len = 0;
+ iter->trailing_short_len = 0;
iter->frame_max = iter->nb_frames = nb_frames;
iter->repeat_frame = iter->curr_frame = 0;
iter->repeat_l = 0;
@@ -133,9 +135,11 @@
/* Reset the iterator so it can start iterating again from the first
extension. */
void opus_extension_iterator_reset(OpusExtensionIterator *iter) {- iter->repeat_data_end = iter->repeat_data = iter->curr_data = iter->data;
+ iter->repeat_data = iter->curr_data = iter->data;
+ iter->last_long = NULL;
iter->curr_len = iter->len;
iter->repeat_frame = iter->curr_frame = 0;
+ iter->trailing_short_len = 0;
}
/* Tell the iterator not to return any extensions for frames of index
@@ -171,17 +175,17 @@
/* Don't repeat padding or frame separators with a 0 increment. */
if (repeat_id_byte <= 3) continue;
/* If the "Repeat These Extensions" extension had L == 0 and this
- is the last repeated extension, and it is a long extension,
- then force decoding the payload with L = 0. */
+ is the last repeated long extension, then force decoding the
+ payload with L = 0. */
if (iter->repeat_l == 0
&& iter->repeat_frame + 1 >= iter->nb_frames
- && iter->src_data == iter->repeat_data_end
- && repeat_id_byte >= 64) {+ && iter->src_data == iter->last_long) {repeat_id_byte &= ~1;
}
curr_data0 = iter->curr_data;
iter->curr_len = skip_extension_payload(&iter->curr_data,
- iter->curr_len, &header_size, repeat_id_byte);
+ iter->curr_len, &header_size, repeat_id_byte,
+ iter->trailing_short_len);
if (iter->curr_len < 0) {return OPUS_INVALID_PACKET;
}
@@ -205,7 +209,8 @@
iter->src_len = iter->repeat_len;
}
/* We finished repeating extensions. */
- iter->repeat_data_end = iter->repeat_data = iter->curr_data;
+ iter->repeat_data = iter->curr_data;
+ iter->last_long = NULL;
/* If L == 0, advance the frame number to handle the case where we did
not consume all of the data with an L == 0 long extension. */
if (iter->repeat_l == 0) {@@ -253,7 +258,9 @@
if (iter->curr_frame >= iter->frame_max) {iter->curr_len = 0;
}
- iter->repeat_data_end = iter->repeat_data = iter->curr_data;
+ iter->repeat_data = iter->curr_data;
+ iter->last_long = NULL;
+ iter->trailing_short_len = 0;
}
else if (id == 2) {iter->repeat_l = L;
@@ -264,14 +271,16 @@
return opus_extension_iterator_next(iter, ext);
}
else if (id > 2) {- /* Update the stopping point for repeating extension data.
- This lets us detect when we have hit the last repeated extension,
- for purposes of modifying the L flag if it is a long extension.
- Only extensions which can have a non-empty payload count, as we can
- still use L=0 to code a final long extension if there cannot be
- any more payload data, even if there are more short L=0
- extensions (or padding). */
- if (L || id >= 32) iter->repeat_data_end = iter->curr_data;
+ /* Update the location of the last long extension.
+ This lets us know when we need to modify the last L flag if we
+ repeat these extensions with L=0. */
+ if (id >= 32) {+ iter->last_long = iter->curr_data;
+ iter->trailing_short_len = 0;
+ }
+ /* Otherwise, keep track of how many payload bytes follow the last
+ long extension. */
+ else iter->trailing_short_len += L;
if (ext != NULL) {ext->id = id;
ext->frame = iter->curr_frame;
@@ -476,10 +485,12 @@
for (f=0;f<nb_frames;f++) frame_repeat_idx[f] = frame_min_idx[f];
for (f=0;f<nb_frames;f++)
{- opus_int32 repeat_data_end_idx;
+ opus_int32 last_long_idx;
+ opus_int32 trailing_short_len;
int repeat_count;
repeat_count = 0;
- repeat_data_end_idx = i;
+ last_long_idx = -1;
+ trailing_short_len = 0;
if (f + 1 < nb_frames)
{for (i=frame_min_idx[f];i<frame_max_idx[f];i++)
@@ -505,23 +516,26 @@
}
if (g < nb_frames) break;
/* We can! */
- /* If this extension can have a non-empty payload, save the
- index of the last instance, so we can modify its L flag. */
- if (extensions[i].id >= 32 || extensions[i].len) {- repeat_data_end_idx = frame_repeat_idx[nb_frames-1];
+ /* If this is a long extension, save the index of the last
+ instance, so we can modify its L flag. */
+ if (extensions[i].id >= 32) {+ last_long_idx = frame_repeat_idx[nb_frames-1];
+ trailing_short_len = 0;
}
+ /* Otherwise, keep track of how many payload bytes follow the
+ last long extension. */
+ else trailing_short_len += extensions[i].len;
/* Using the repeat mechanism almost always makes the
encoding smaller (or at least no larger).
However, there's one case where that might not be true: if
- the last repeated extension in the last frame was previously
- the last extension, but using the repeat mechanism makes
- that no longer true (because there are other non-repeated
- extensions in earlier frames that must now be coded after
- it), and coding its length requires more bytes than the
- repeat mechanism saves.
- This can only be true if it is a long extension with at least
- 255 bytes of payload data (although sometimes it requires
- even more).
+ the last repeated long extension in the last frame was
+ previously the last extension, but using the repeat
+ mechanism makes that no longer true (because there are other
+ non-repeated extensions in earlier frames that must now be
+ coded after it), and coding its length requires more bytes
+ than the repeat mechanism saves.
+ This can only be true if its length is at least 255 bytes
+ (although sometimes it requires even more).
Currently we do not check for that, and just always use the
repeat mechanism if we can. */
#if 0
@@ -616,8 +630,7 @@
/* Add the repeat indicator. */
nb_repeated = repeat_count*(nb_frames - (f + 1));
last = written + nb_repeated == nb_extensions
- || (extensions[repeat_data_end_idx].id < 32
- && i+1 >= frame_max_idx[f]);
+ || (last_long_idx < 0 && i+1 >= frame_max_idx[f]);
if (len-pos < 1)
return OPUS_BUFFER_TOO_SMALL;
if (data) data[pos] = 0x04 + !last;
@@ -630,7 +643,7 @@
if (extensions[j].frame == g)
{pos = write_extension_payload(data, len, pos,
- extensions + j, last && j == repeat_data_end_idx);
+ extensions + j, last && j == last_long_idx);
if (pos < 0) return pos;
written++;
}
--- a/src/opus_private.h
+++ b/src/opus_private.h
@@ -51,12 +51,13 @@
const unsigned char *data;
const unsigned char *curr_data;
const unsigned char *repeat_data;
- const unsigned char *repeat_data_end;
+ const unsigned char *last_long;
const unsigned char *src_data;
opus_int32 len;
opus_int32 curr_len;
opus_int32 repeat_len;
opus_int32 src_len;
+ opus_int32 trailing_short_len;
int nb_frames;
int frame_max;
int curr_frame;
--- a/tests/test_opus_extensions.c
+++ b/tests/test_opus_extensions.c
@@ -319,7 +319,10 @@
{3, 0, (const unsigned char *)"a", 1}, {33, 1, (const unsigned char *)"NOT DRED", 8}, {4, 4, (const unsigned char *)NULL, 0},- {32, 10, (const unsigned char *)"DRED", 4}+ {32, 10, (const unsigned char *)"DRED", 4},+ {32, 9, (const unsigned char *)"DRED", 4},+ {4, 9, (const unsigned char *)"b", 1},+ {4, 10, (const unsigned char *)"c", 1}};
opus_extension_data ext_out[10];
int nb_ext;
@@ -370,6 +373,20 @@
nb_frames);
expect_true(result == OPUS_BUFFER_TOO_SMALL, "expected OPUS_BUFFER_TOO_SMALL");
+ /* create repeated L=0 long extension without enough room for all of the
+ short extension payloads that need to follow it */
+ len = opus_packet_extensions_generate(packet, sizeof(packet), ext, 7, 11, 0);
+ len -= 5;
+ nb_ext = 10;
+ nb_frames = 11;
+ result = opus_packet_extensions_parse(packet, len, ext_out, &nb_ext,
+ nb_frames);
+ expect_true(result == OPUS_INVALID_PACKET, "expected OPUS_INVALID_PACKET");
+ /* note, opus_packet_extensions_count stops at the invalid long extension
+ and tells us that we have 5 extensions */
+ result = opus_packet_extensions_count(packet, len, nb_frames);
+ expect_true(result == 5, "expected opus_packet_extensions_count to return 5");
+
/* overflow for long extension length */
{/* about 8 MB */
@@ -431,8 +448,8 @@
{4, 0, (const unsigned char *)"d", 1}, {4, 1, (const unsigned char *)NULL, 0}, {4, 2, (const unsigned char *)NULL, 0},- {32, 1, (const unsigned char *)"DRED", 4}, {32, 2, (const unsigned char *)"DRED2", 5},+ {32, 1, (const unsigned char *)"DRED", 4}, {5, 1, (const unsigned char *)NULL, 0}, {5, 2, (const unsigned char *)NULL, 0}, {6, 2, (const unsigned char *)"f", 1},@@ -452,10 +469,9 @@
/* nb_ext = 6: do not repeat short extensions if the lengths do not match
... but do repeat after the first frame when the lengths do match. */
10,
- /* nb_ext = 7: code a long extension with L=0 despite having more
- extensions in future frames, because we already coded them via
- repeats. */
- 15,
+ /* nb_ext = 7: code repeated extensions with L=0 to skip a frame
+ separator because they are all short extensions. */
+ 16,
/* nb_ext = 8: repeat multiple extensions in the same frame.
code the last repeated extension with L=0 if it is a long
extension. */
@@ -462,18 +478,17 @@
21,
23,
/* nb_ext = 10: code the last repeated long extension with L=0 even if it
- is followed by repeated short extensions, as long as they have L=0. */
+ is followed by repeated short extensions with L=0. */
22,
/* nb_ext = 11: don't use L=0 to skip a frame separator if repeats end
- on a short L=0 extension if there was a preceding L=0 long
- extension. */
+ on a short extension if there was a preceding L=0 long extension. */
26,
- /* nb_ext = 12: don't code the last repeated long extension with L=0 if
+ /* nb_ext = 12: code the last repeated long extension with L=0 even if
it is followed by repeated short extensions with L=1. */
- 26,
- /* nb_ext = 13: do use L=0 to skip a frame separator if repeats end on a
- short L=1 extension */
- 36
+ 25,
+ /* nb_ext = 13: don't use L=0 to skip a frame separator if repeats end
+ on a short extension if there was a preceding L=1 long extension. */
+ 37
};
opus_int32 nb_ext;
for (nb_ext = 0; nb_ext <= NB_EXT; nb_ext++) {@@ -521,10 +536,7 @@
else if (nb_ext == 13) {/* use L=0 to skip a frame separator if a repeat has no extensions at
all */
- packet[18] = 2<<1|1;
- memmove(packet+27, packet+26, len-26);
packet[26] = 2<<1|0;
- len++;
}
else continue;
result = opus_packet_extensions_count_ext(packet, len, nb_frame_exts, 3);
@@ -537,6 +549,10 @@
check_ext_data(ext, ext_out, nb_ext);
if (nb_ext == 8) {/* allow multiple repeat indicators in the same frame */
+ memmove(packet+10, packet+9, len-9);
+ packet[9] = 2<<1|1;
+ len++;
+ /* even when there are no new extensions to repeat */
memmove(packet+6, packet+5, len-5);
packet[5] = 2<<1|1;
len++;
--
⑨