Skip to content

Conversation

@dvosully
Copy link
Contributor

@dvosully dvosully commented Apr 5, 2024

The type of the nb_extensions argument to opus_multistream_decode_native() in the header does not match the implementation. It is defined as (opus_int32) in the C, and (int) in the H.
On some embedded platforms, opus_int32 is not defined as an int type. It may be long int, or other. This causes a compilation failure due to mismatched types.

This PR changes the type in the header to match the implementation to remove this compilation failure.

Bug identified on the ESP32 platform.

@dvosully dvosully changed the title Change type of function parameter in header to match implementation Fix type mismatch in opus_multistream_decode_native Apr 5, 2024
@jmvalin
Copy link
Member

jmvalin commented Apr 8, 2024

Merged. I'd be curious if you encountered any other issue with 16-bit ints, since it is hard to test.

@dvosully
Copy link
Contributor Author

dvosully commented Apr 8, 2024

Thanks @jmvalin.

My platform is actually 32-bit (Tensilica Xtensa LX6), the standard types are just defined a bit weird. int32_t is defined as a long int not as an int. While they both map to a 32-bit value, they are different nominal types and cause issues if they are mixed.

I don't recall any other issues with the fixed point build, but I have a recollection of the floating point build having some issues when I accidentally built it. I'll aim to check that out later this week.

@dvosully dvosully closed this Apr 8, 2024
@dvosully dvosully deleted the bugfix/types branch April 9, 2024 06:31
@dvosully
Copy link
Contributor Author

dvosully commented Apr 9, 2024

@jmvalin Following are the warnings I get from a floating point build of the current master for my platform. It's not entirely obvious to me which of them should be addressed. Happy to go through and put together another PR but not sure how aggressive I should be at retyping or casting things to mute warnings, especially between different modules.

There are no (meaningful) warnings if I exclude the floating point build.

[25/174] Building C object esp-idf/esp-libopus/CMakeFiles/__idf_esp-libopus.dir/opus/src/repacketizer.c.obj
C:/Espressif/development/test/components/esp-libopus/opus/src/repacketizer.c: In function 'opus_repacketizer_out_range_impl':
C:/Espressif/development/test/components/esp-libopus/opus/src/repacketizer.c:161:38: warning: passing argument 4 of 'opus_packet_extensions_parse' from incompatible pointer type [-Wincompatible-pointer-types]
  161 |          &all_extensions[ext_count], &frame_ext_count);
      |                                      ^~~~~~~~~~~~~~~~
      |                                      |
      |                                      int *
In file included from C:/Espressif/development/test/components/esp-libopus/opus/src/repacketizer.c:33:
C:/Espressif/development/test/components/esp-libopus/opus/src/opus_private.h:215:129: note: expected 'opus_int32 *' {aka 'long int *'} but argument is of type 'int *'
  215 | opus_int32 opus_packet_extensions_parse(const unsigned char *data, opus_int32 len, opus_extension_data *extensions, opus_int32 *nb_extensions);
      |                                                                                                                     ~~~~~~~~~~~~^~~~~~~~~~~~~
C:/Espressif/development/test/components/esp-libopus/opus/src/repacketizer.c:304:11: warning: unused variable 'ret' [-Wunused-variable]
  304 |       int ret = opus_packet_extensions_generate(&data[ext_begin], ext_len, all_extensions, ext_count, 0);
      |           ^~~
[1/1] cmd.exe /C "cd /D C:\Espressif\development\test\buil...spressif/development/test/build/bootloader/bootloader.bin"
Bootloader binary size 0x6860 bytes. 0x7a0 bytes (7%) free.
[127/172] Building C object esp-idf/esp-libopus/CMakeFiles/__idf_esp-libopus.dir/opus/silk/fixed/autocorr_FIX.c.obj
C:/Espressif/development/test/components/esp-libopus/opus/silk/fixed/autocorr_FIX.c: In function 'silk_autocorr':
C:/Espressif/development/test/components/esp-libopus/opus/silk/fixed/autocorr_FIX.c:47:29: warning: passing argument 1 of '_celt_autocorr' from incompatible pointer type [-Wincompatible-pointer-types]
   47 |     *scale = _celt_autocorr(inputData, results, NULL, 0, corrCount-1, inputDataSize, arch);
      |                             ^~~~~~~~~
      |                             |
      |                             const opus_int16 * {aka const short int *}
In file included from C:/Espressif/development/test/components/esp-libopus/opus/silk/fixed/autocorr_FIX.c:33:
C:/Espressif/development/test/components/esp-libopus/opus/celt/celt_lpc.h:63:38: note: expected 'const opus_val16 *' {aka 'const float *'} but argument is of type 'const opus_int16 *' {aka 'const short int *'}
   63 | int _celt_autocorr(const opus_val16 *x, opus_val32 *ac,
      |                    ~~~~~~~~~~~~~~~~~~^
C:/Espressif/development/test/components/esp-libopus/opus/silk/fixed/autocorr_FIX.c:47:40: warning: passing argument 2 of '_celt_autocorr' from incompatible pointer type [-Wincompatible-pointer-types]
   47 |     *scale = _celt_autocorr(inputData, results, NULL, 0, corrCount-1, inputDataSize, arch);
      |                                        ^~~~~~~
      |                                        |
      |                                        opus_int32 * {aka long int *}
C:/Espressif/development/test/components/esp-libopus/opus/celt/celt_lpc.h:63:53: note: expected 'opus_val32 *' {aka 'float *'} but argument is of type 'opus_int32 *' {aka 'long int *'}
   63 | int _celt_autocorr(const opus_val16 *x, opus_val32 *ac,
      |                                         ~~~~~~~~~~~~^~
[128/172] Building C object esp-idf/esp-libopus/CMakeFiles...df_esp-libopus.dir/opus/silk/fixed/burg_modified_FIX.c.obj
C:/Espressif/development/test/components/esp-libopus/opus/silk/fixed/burg_modified_FIX.c: In function 'silk_burg_modified_c':
C:/Espressif/development/test/components/esp-libopus/opus/silk/fixed/burg_modified_FIX.c:98:30: warning: passing argument 1 of 'celt_pitch_xcorr_c' from incompatible pointer type [-Wincompatible-pointer-types]
   98 |             celt_pitch_xcorr(x_ptr, x_ptr + 1, xcorr, subfr_length - D, D, arch );
      |                              ^~~~~
      |                              |
      |                              const opus_int16 * {aka const short int *}
In file included from C:/Espressif/development/test/components/esp-libopus/opus/silk/fixed/burg_modified_FIX.c:35:
C:/Espressif/development/test/components/esp-libopus/opus/celt/pitch.h:185:38: note: expected 'const opus_val16 *' {aka 'const float *'} but argument is of type 'const opus_int16 *' {aka 'const short int *'}
  185 | celt_pitch_xcorr_c(const opus_val16 *_x, const opus_val16 *_y,
      |                    ~~~~~~~~~~~~~~~~~~^~
C:/Espressif/development/test/components/esp-libopus/opus/silk/fixed/burg_modified_FIX.c:98:43: warning: passing argument 2 of 'celt_pitch_xcorr_c' from incompatible pointer type [-Wincompatible-pointer-types]
   98 |             celt_pitch_xcorr(x_ptr, x_ptr + 1, xcorr, subfr_length - D, D, arch );
      |                                     ~~~~~~^~~
      |                                           |
      |                                           const opus_int16 * {aka const short int *}
C:/Espressif/development/test/components/esp-libopus/opus/celt/pitch.h:185:60: note: expected 'const opus_val16 *' {aka 'const float *'} but argument is of type 'const opus_int16 *' {aka 'const short int *'}
  185 | celt_pitch_xcorr_c(const opus_val16 *_x, const opus_val16 *_y,
      |                                          ~~~~~~~~~~~~~~~~~~^~
C:/Espressif/development/test/components/esp-libopus/opus/silk/fixed/burg_modified_FIX.c:98:48: warning: passing argument 3 of 'celt_pitch_xcorr_c' from incompatible pointer type [-Wincompatible-pointer-types]
   98 |             celt_pitch_xcorr(x_ptr, x_ptr + 1, xcorr, subfr_length - D, D, arch );
      |                                                ^~~~~
      |                                                |
      |                                                opus_int32 * {aka long int *}
C:/Espressif/development/test/components/esp-libopus/opus/celt/pitch.h:186:19: note: expected 'opus_val32 *' {aka 'float *'} but argument is of type 'opus_int32 *' {aka 'long int *'}
  186 |       opus_val32 *xcorr, int len, int max_pitch, int arch);
      |       ~~~~~~~~~~~~^~~~~
[148/172] Building C object esp-idf/esp-libopus/CMakeFiles...-libopus.dir/opus/silk/fixed/pitch_analysis_core_FIX.c.obj
C:/Espressif/development/test/components/esp-libopus/opus/silk/fixed/pitch_analysis_core_FIX.c: In function 'silk_pitch_analysis_core':
C:/Espressif/development/test/components/esp-libopus/opus/silk/fixed/pitch_analysis_core_FIX.c:200:27: warning: passing argument 1 of 'celt_pitch_xcorr_c' from incompatible pointer type [-Wincompatible-pointer-types]
  200 |         celt_pitch_xcorr( target_ptr, target_ptr - MAX_LAG_4KHZ, xcorr32, SF_LENGTH_8KHZ, MAX_LAG_4KHZ - MIN_LAG_4KHZ + 1, arch );
      |                           ^~~~~~~~~~
      |                           |
      |                           const opus_int16 * {aka const short int *}
In file included from C:/Espressif/development/test/components/esp-libopus/opus/silk/fixed/pitch_analysis_core_FIX.c:39:
C:/Espressif/development/test/components/esp-libopus/opus/celt/pitch.h:185:38: note: expected 'const opus_val16 *' {aka 'const float *'} but argument is of type 'const opus_int16 *' {aka 'const short int *'}
  185 | celt_pitch_xcorr_c(const opus_val16 *_x, const opus_val16 *_y,
      |                    ~~~~~~~~~~~~~~~~~~^~
C:/Espressif/development/test/components/esp-libopus/opus/silk/fixed/pitch_analysis_core_FIX.c:200:50: warning: passing argument 2 of 'celt_pitch_xcorr_c' from incompatible pointer type [-Wincompatible-pointer-types]
  200 |         celt_pitch_xcorr( target_ptr, target_ptr - MAX_LAG_4KHZ, xcorr32, SF_LENGTH_8KHZ, MAX_LAG_4KHZ - MIN_LAG_4KHZ + 1, arch );
      |                                                  ^
      |                                                  |
      |                                                  const opus_int16 * {aka const short int *}
C:/Espressif/development/test/components/esp-libopus/opus/celt/pitch.h:185:60: note: expected 'const opus_val16 *' {aka 'const float *'} but argument is of type 'const opus_int16 *' {aka 'const short int *'}
  185 | celt_pitch_xcorr_c(const opus_val16 *_x, const opus_val16 *_y,
      |                                          ~~~~~~~~~~~~~~~~~~^~
C:/Espressif/development/test/components/esp-libopus/opus/silk/fixed/pitch_analysis_core_FIX.c:200:66: warning: passing argument 3 of 'celt_pitch_xcorr_c' from incompatible pointer type [-Wincompatible-pointer-types]
  200 |         celt_pitch_xcorr( target_ptr, target_ptr - MAX_LAG_4KHZ, xcorr32, SF_LENGTH_8KHZ, MAX_LAG_4KHZ - MIN_LAG_4KHZ + 1, arch );
      |                                                                  ^~~~~~~
      |                                                                  |
      |                                                                  opus_int32 * {aka long int *}
C:/Espressif/development/test/components/esp-libopus/opus/celt/pitch.h:186:19: note: expected 'opus_val32 *' {aka 'float *'} but argument is of type 'opus_int32 *' {aka 'long int *'}
  186 |       opus_val32 *xcorr, int len, int max_pitch, int arch);
      |       ~~~~~~~~~~~~^~~~~
C:/Espressif/development/test/components/esp-libopus/opus/silk/fixed/pitch_analysis_core_FIX.c: In function 'silk_P_Ana_calc_corr_st3':
C:/Espressif/development/test/components/esp-libopus/opus/silk/fixed/pitch_analysis_core_FIX.c:616:27: warning: passing argument 1 of 'celt_pitch_xcorr_c' from incompatible pointer type [-Wincompatible-pointer-types]
  616 |         celt_pitch_xcorr( target_ptr, target_ptr - start_lag - lag_high, xcorr32, sf_length, lag_high - lag_low + 1, arch );
      |                           ^~~~~~~~~~
      |                           |
      |                           const opus_int16 * {aka const short int *}
C:/Espressif/development/test/components/esp-libopus/opus/celt/pitch.h:185:38: note: expected 'const opus_val16 *' {aka 'const float *'} but argument is of type 'const opus_int16 *' {aka 'const short int *'}
  185 | celt_pitch_xcorr_c(const opus_val16 *_x, const opus_val16 *_y,
      |                    ~~~~~~~~~~~~~~~~~~^~
C:/Espressif/development/test/components/esp-libopus/opus/silk/fixed/pitch_analysis_core_FIX.c:616:62: warning: passing argument 2 of 'celt_pitch_xcorr_c' from incompatible pointer type [-Wincompatible-pointer-types]
  616 |         celt_pitch_xcorr( target_ptr, target_ptr - start_lag - lag_high, xcorr32, sf_length, lag_high - lag_low + 1, arch );
      |                                       ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
      |                                                              |
      |                                                              const opus_int16 * {aka const short int *}
C:/Espressif/development/test/components/esp-libopus/opus/celt/pitch.h:185:60: note: expected 'const opus_val16 *' {aka 'const float *'} but argument is of type 'const opus_int16 *' {aka 'const short int *'}
  185 | celt_pitch_xcorr_c(const opus_val16 *_x, const opus_val16 *_y,
      |                                          ~~~~~~~~~~~~~~~~~~^~
C:/Espressif/development/test/components/esp-libopus/opus/silk/fixed/pitch_analysis_core_FIX.c:616:74: warning: passing argument 3 of 'celt_pitch_xcorr_c' from incompatible pointer type [-Wincompatible-pointer-types]
  616 |         celt_pitch_xcorr( target_ptr, target_ptr - start_lag - lag_high, xcorr32, sf_length, lag_high - lag_low + 1, arch );
      |                                                                          ^~~~~~~
      |                                                                          |
      |                                                                          opus_int32 * {aka long int *}
C:/Espressif/development/test/components/esp-libopus/opus/celt/pitch.h:186:19: note: expected 'opus_val32 *' {aka 'float *'} but argument is of type 'opus_int32 *' {aka 'long int *'}
  186 |       opus_val32 *xcorr, int len, int max_pitch, int arch);
      |       ~~~~~~~~~~~~^~~~~
[

@jmvalin
Copy link
Member

jmvalin commented Apr 9, 2024

Looks like you might have a problem with your build system. silk/fixed/autocorr_FIX.c shouldn't even be compiled for floating point and warnings such as
expected 'opus_val32 *' {aka 'float *'} but argument is of type 'opus_int32 *' {aka 'long int *'}
also seem to imply a mix of float and fixed-point settings.

@dvosully
Copy link
Contributor Author

dvosully commented Apr 9, 2024

That's sounds about right, my top level makefile is just a glob of C files. (I know it's not recommended, but it's what has got me off the ground so far. My CMake skills are very weak.) I'll tidy that up at some stage and take another look, when I tried to integrate it nicely initially I got errors I wasn't prepared to track down.

@jmvalin
Copy link
Member

jmvalin commented Apr 9, 2024

Well, given the warnings above, your build is completely broken and likely would either crash or output garbage.

@dvosully
Copy link
Contributor Author

dvosully commented Apr 9, 2024

Yeah, you're right. I was still building with the fixed point headers, fixed now.
I'm left with only one warning, which looks to be valid:

C:/Espressif/development/test/components/esp-libopus/opus/src/repacketizer.c: In function 'opus_repacketizer_out_range_impl':
C:/Espressif/development/test/components/esp-libopus/opus/src/repacketizer.c:161:38: warning: passing argument 4 of 'opus_packet_extensions_parse' from incompatible pointer type [-Wincompatible-pointer-types]
  161 |          &all_extensions[ext_count], &frame_ext_count);
      |                                      ^~~~~~~~~~~~~~~~
      |                                      |
      |                                      int *
In file included from C:/Espressif/development/test/components/esp-libopus/opus/src/repacketizer.c:33:
C:/Espressif/development/test/components/esp-libopus/opus/src/opus_private.h:215:129: note: expected 'opus_int32 *' {aka 'long int *'} but argument is of type 'int *'
  215 | opus_int32 opus_packet_extensions_parse(const unsigned char *data, opus_int32 len, opus_extension_data *extensions, opus_int32 *nb_extensions);
      |                                                                                                                     ~~~~~~~~~~~~^~~~~~~~~~~~~

Edit: #330 has a fix for it which seemingly didn't get merged.

@jmvalin
Copy link
Member

jmvalin commented Apr 9, 2024

Should be fixed now. Please confirm

@dvosully
Copy link
Contributor Author

I confirm fixed. Building cleanly for fixed and floating point on my platform. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants