Convert to 16 bits only at the very end

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>

This avoids the need to code a frame separator in the case that the
 last repeated extension is a short extension, and the next
 non-repeated extension is in the following frame instead of the
 current one (saving one additional byte).
Also add tests for both encoding and decoding this.

Signed-off-by: Jean-Marc Valin <jeanmarcv@google.com>

This gives more confidence that the generation code always produces
 output that the parsing code can parse.

Signed-off-by: Jean-Marc Valin <jeanmarcv@google.com>

Also adds tests which exercise generating repeated extensions as
 well as the count_ext() and parse_ext() API for parsing extensions
 in frame order.

Signed-off-by: Jean-Marc Valin <jeanmarcv@google.com>

Right now, opus_packet_extensions_parse() returns the extensions in
 the order they appear in the packet, which is no longer necessarily
 in frame order.
This adds a new (still private) API that returns parsed extensions
 in frame order, even when repeated extensions are used.
Nothing has been converted to use this new API yet.

Signed-off-by: Jean-Marc Valin <jeanmarcv@google.com>

Created test_opus_custom script which does a more heavy evaluation of the various use cases of OpusCustom, testing:
- Mixed float/fixed use cases
- Mixed Opus/OpusCustom use cases
- Wide mixture of run-time configurables
- RMS difference (if RESYNTH) is defined

Signed-off-by: Jean-Marc Valin <jeanmarcv@google.com>

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.

Test multiple buffer lengths and ensure we do not write past the
 end of the provided buffer.

With an 8+ MB packet it is possible to craft an extension length
 that would overflow, bypassing the checks to ensure the extension
 data remains inside the packet.
This patch fixes that and adds a test for it.

Without the fix, ubsan would complain about some of the
NULL pointer checks from test_opus_api.

Also, remove -march=native because of AVX512VNNI and valgrind

Xavier Claessens authored 1 year ago and Jean-Marc Valin committed 1 year ago

They timeout on GitHub actions because those runners are slower.

Chris Hold authored 1 year ago and Jean-Marc Valin committed 1 year ago

Signed-off-by: Jean-Marc Valin <jmvalin@jmvalin.ca>

Chris Hold authored 1 year ago and Jean-Marc Valin committed 1 year ago

Signed-off-by: Jean-Marc Valin <jmvalin@jmvalin.ca>

Still need more targeted tests, DRED decoding

Still don't quite know what I'm doing

Marcus Asteborg authored 1 year ago and Jean-Marc Valin committed 1 year ago

Signed-off-by: Marcus Asteborg <maastebo@microsoft.com>
Signed-off-by: Jean-Marc Valin <jmvalin@amazon.com>

Marcus Asteborg authored 1 year ago and Jean-Marc Valin committed 1 year ago

Signed-off-by: Jean-Marc Valin <jmvalin@jmvalin.ca>

Marcus Asteborg authored 1 year ago and Jean-Marc Valin committed 1 year ago

Signed-off-by: Jean-Marc Valin <jmvalin@jmvalin.ca>

Michael Klingbeil authored 1 year ago and Jean-Marc Valin committed 1 year ago

Signed-off-by: Jean-Marc Valin <jmvalin@amazon.com>

can't link woth libopus since it's already #including C files

Michael Klingbeil authored 1 year ago and Jean-Marc Valin committed 1 year ago

Add unit tests for opus_packet_extensions_count, opus_packet_extensions_parse,
and opus_packet_extensions_generate. Add various assertions for NULL buffers or
negative length arguments. Add explicit check for invalid length in
opus_packet_extensions_count. Check for extension id >127 in
opus_packet_extensions_generate. Check for invalid length input in
opus_packet_extensions_generate.

Signed-off-by: Jean-Marc Valin <jmvalin@amazon.com>

Reviewed by Mark Harris

Marcus Asteborg authored 2 years ago and Mark Harris committed 2 years ago

Signed-off-by: Mark Harris <mark.hsj@gmail.com>