Remaining stb_vorbis diff.

[AliceLR]

This is the remaining diff between the local libxmp stb_vorbis and upstream + misc. patches I contributed pending merge. I've removed irrelevant portions and regrouped bits of it.

libxmp specific changes that probably shouldn't be upstreamed:

69a69,73
> /* libxmp customizations: */
> #define STB_VORBIS_C
> #include "vorbis.h"
> 
> 
355a360
> #ifndef STB_VORBIS_NO_FLOAT_CONVERSION
357a363
> #endif
650a653,656
> /* libxmp-specific change */
> #if 1
> #include "../common.h"
> #else
656a663
> #endif
975c982
<    return sz ? malloc(sz) : NULL;
---
>    return sz ? calloc(sz, 1) : NULL;
992c999
<    return malloc(sz);
---
>    return calloc(sz, 1);
5469a5482
> #ifndef STB_VORBIS_NO_FLOAT_CONVERSION
5521a5535
> #endif // STB_VORBIS_NO_FLOAT_CONVERSION

alloca usage tweaks and STB_FORCEINLINE appear to be interconnected:

593,600d598
< 
<    // find definition of alloca if it's not in stdlib.h:
<    #if defined(_MSC_VER) || defined(__MINGW32__)
<       #include <malloc.h>
<    #endif
<    #if defined(__linux__) || defined(__linux) || defined(__sun__) || defined(__EMSCRIPTEN__) || defined(__NEWLIB__)
<       #include <alloca.h>
<    #endif
607a606,620
> /* we need alloca() regardless of STB_VORBIS_NO_CRT,
>  * because there is not a corresponding 'dealloca' */
> #if !defined(alloca)
> # if defined(HAVE_ALLOCA_H)
> #  include <alloca.h>
> # elif defined(__GNUC__)
> #  define alloca __builtin_alloca
> # elif defined(_MSC_VER)
> #  include <malloc.h>
> #  define alloca _alloca
> # elif defined(__WATCOMC__)
> #  include <malloc.h>
> # endif
> #endif
> 
610,628c623,630
< #ifdef __MINGW32__
<    // eff you mingw:
<    //     "fixed":
<    //         http://sourceforge.net/p/mingw-w64/mailman/message/32882927/
<    //     "no that broke the build, reverted, who cares about C":
<    //         http://sourceforge.net/p/mingw-w64/mailman/message/32890381/
<    #ifdef __forceinline
<    #undef __forceinline
<    #endif
<    #define __forceinline
<    #ifndef alloca
<    #define alloca __builtin_alloca
<    #endif
< #elif !defined(_MSC_VER)
<    #if __GNUC__
<       #define __forceinline inline
<    #else
<       #define __forceinline
<    #endif
---
> #ifndef STB_FORCEINLINE
>     #if defined(_MSC_VER)
>         #define STB_FORCEINLINE __forceinline
>     #elif (defined(__GNUC__) && (__GNUC__ > 3 || (__GNUC__ == 3 && __GNUC_MINOR__ >= 2))) || defined(__clang__)
>         #define STB_FORCEINLINE static __inline __attribute__((always_inline))
>     #else
>         #define STB_FORCEINLINE static __inline
>     #endif
1020c1027
< static __forceinline uint32 crc32_update(uint32 crc, uint8 byte)
---
> STB_FORCEINLINE uint32 crc32_update(uint32 crc, uint8 byte)
1653c1663
< static __forceinline void prep_huffman(vorb *f)
---
> STB_FORCEINLINE void prep_huffman(vorb *f)
2053c2063
< static __forceinline void draw_line(float *output, int x0, int y0, int x1, int y1, int n)
---
> STB_FORCEINLINE void draw_line(float *output, int x0, int y0, int x1, int y1, int n)
2573c2583
< static __forceinline void iter_54(float *z)
---
> STB_FORCEINLINE void iter_54(float *z)

"silence warnings" for which compilers/optimization flags? I haven't tried extensively but so far I haven't encountered warnings from these upstream. I'm OK with upstreaming this, but I need a compiler to cite.

1367a1375
>    return 0;  /* silence warnings */
1396a1405
>    return 0;  /* silence warnings */
1442a1452
>    return 0;  /* silence warnings */
4595a4606
>    return 0;  /* silence warnings */

Fairly useless changes to debug deadcode. Removed.

Pointers of these were provided to functions expecting uint32 *, should definitely be upstreamed.

4721c4732,4733
<    unsigned int previous_safe, end;
---
>    unsigned int previous_safe;
>    uint32 end;
4993c5005
<    unsigned int end, last_page_loc;
---
>    unsigned int last_page_loc;
4997c5009
<       unsigned int last;
---
>       uint32 end,last;

There's also the outstanding issue of this patch not having been reimplemented yet, but that should be straightforward and likely it can be upstreamed too. Currently, the only place libxmp uses stb_vorbis is for decoding OXM samples, and OggMod was apparently never aware of stereo samples and always encoded them as a single mono stream. (The removal of this patch was probably also responsible for a noticeable dip in coverage, if that matters at all.)

@sezero any thoughts?

[sezero]

libxmp specific changes that probably shouldn't be upstreamed:

Yes

#ifndef STB_VORBIS_NO_FLOAT_CONVERSION

I don't know when this happened, but it disables code that we don't use

<    return sz ? malloc(sz) : NULL;
---
>    return sz ? calloc(sz, 1) : NULL;

Don't know when this happened either, nor do I know if they're necessary

alloca usage tweaks and STB_FORCEINLINE appear to be interconnected:

They are mine and they are actually good (can possibly be done better.)
If you want to upstream them, be my guest.

"silence warnings" for which compilers/optimization flags? I haven't tried extensively but so far I haven't encountered warnings from these upstream. I'm OK with upstreaming this, but I need a compiler to cite.

>    return 0;  /* silence warnings */

They are to prevent some stupid stupid Watcom warnings

src/depackers/vorbis.c(1376): Warning! W107: Missing return value for function 'get8'
src/depackers/vorbis.c(1406): Warning! W107: Missing return value for function 'getn'
src/depackers/vorbis.c(1453): Warning! W107: Missing return value for function 'set_file_offset'
src/depackers/vorbis.c(4607): Warning! W107: Missing return value for function 'stb_vorbis_get_file_offset'

Fairly useless changes to debug deadcode.

Can be removed here I think

Pointers of these were provided to functions expecting uint32 *, should definitely be upstreamed.

Yes

[sezero]

There's also the outstanding issue of this patch not having been reimplemented yet, but that should be straightforward and likely it can be upstreamed too. Currently, the only place libxmp uses stb_vorbis is for decoding OXM samples, and OggMod was apparently never aware of stereo samples and always encoded them as a single mono stream.

Let's re-implement it

[AliceLR]

650a653,656
> /* libxmp-specific change */
> #if 1
> #include "../common.h"
> #else
656a663
> #endif

Pointers of these were provided to functions expecting uint32 *, should definitely be upstreamed.

4721c4732,4733
<    unsigned int previous_safe, end;
---
>    unsigned int previous_safe;
>    uint32 end;
4993c5005
<    unsigned int end, last_page_loc;
---
>    unsigned int last_page_loc;
4997c5009
<       unsigned int last;
---
>       uint32 end,last;

On further review, these modifications are connected: uint32 is always unsigned int upstream, it's the libxmp modification that allows unsigned long definitions of uint32 here. I'm skeptical of this common.h include, should the relevant includes just be copied directly to stb_vorbis.c (so presumably upstream could benefit too)?

/* AmigaOS fixes by Chris Young <cdyoung@ntlworld.com>, Nov 25, 2007
 */
#if defined B_BEOS_VERSION
#  include <SupportDefs.h>
#elif defined __amigaos4__
#  include <exec/types.h>
#else
typedef signed char int8;
typedef signed short int int16;
typedef signed int int32;
typedef unsigned char uint8;
typedef unsigned short int uint16;
typedef unsigned int uint32;
#endif

edit: replacing this doesn't seem to break anything, but I don't have BeOS or Amiga OS 4 toolchains right now to verify that portion is correct.

[sezero]

On further review, these modifications are connected: uint32 is always unsigned int upstream, it's the libxmp modification that allows unsigned long definitions of uint32 here. I'm skeptical of this common.h include, should the relevant includes just be copied directly to stb_vorbis.c (so presumably upstream could benefit too)?

If stb-mainstream ever change their typedefs they will have problems too, i.e. it still is a latent issue on their side. So, IMO, that uint32 change is needed. As for upstream updating their typedefs, don't know whether they care - I don't whether they do.

[sezero]

What else is left to do except for cdf072f?

Speaking of code dead-to-libxmp, we also don't use the seek api, along with a few other public functions which can shave off up to 4-5 kb from the final dll -- if we really want to try.

Speaking of public functions in stb_vorbis: I think we should apply a libxmp namespace to non-static functions in vorbis.c. Otherwise, if a project itself uses stb_vorbis and wants to link statically to libxmp, it will fail. Something like the following perhaps?
EDIT: Created #511 for this.

diff --git a/src/loaders/vorbis.h b/src/loaders/vorbis.h
index da7214e..af4a03e 100644
--- a/src/loaders/vorbis.h
+++ b/src/loaders/vorbis.h
@@ -6,6 +6,35 @@
 #define STB_VORBIS_NO_COMMENTS
 #define STB_VORBIS_NO_FLOAT_CONVERSION
 
+/* change namespace from stb_ to libxmp_ for public functions: */
+#define stb_vorbis_get_info libxmp_vorbis_get_info
+#define stb_vorbis_get_comment libxmp_vorbis_get_comment
+#define stb_vorbis_get_error libxmp_vorbis_get_error
+#define stb_vorbis_close libxmp_vorbis_close
+#define stb_vorbis_get_sample_offset libxmp_vorbis_get_sample_offset
+#define stb_vorbis_get_file_offset libxmp_vorbis_get_file_offset
+#define stb_vorbis_open_pushdata libxmp_vorbis_open_pushdata
+#define stb_vorbis_decode_frame_pushdata libxmp_vorbis_decode_frame_pushdata
+#define stb_vorbis_flush_pushdata libxmp_vorbis_flush_pushdata
+#define stb_vorbis_decode_filename libxmp_vorbis_decode_filename
+#define stb_vorbis_decode_memory libxmp_vorbis_decode_memory
+#define stb_vorbis_open_memory libxmp_vorbis_open_memory
+#define stb_vorbis_open_filename libxmp_vorbis_open_filename
+#define stb_vorbis_open_file libxmp_vorbis_open_file
+#define stb_vorbis_open_file_section libxmp_vorbis_open_file_section
+#define stb_vorbis_seek_frame libxmp_vorbis_seek_frame
+#define stb_vorbis_seek libxmp_vorbis_seek
+#define stb_vorbis_seek_start libxmp_vorbis_seek_start
+#define stb_vorbis_stream_length_in_samples libxmp_vorbis_stream_length_in_samples
+#define stb_vorbis_stream_length_in_seconds libxmp_vorbis_stream_length_in_seconds
+#define stb_vorbis_get_frame_float libxmp_vorbis_get_frame_float
+#define stb_vorbis_get_frame_short_interleaved libxmp_vorbis_get_frame_short_interleaved
+#define stb_vorbis_get_frame_short libxmp_vorbis_get_frame_short
+#define stb_vorbis_get_samples_float_interleaved libxmp_vorbis_get_samples_float_interleaved
+#define stb_vorbis_get_samples_float libxmp_vorbis_get_samples_float
+#define stb_vorbis_get_samples_short_interleaved libxmp_vorbis_get_samples_short_interleaved
+#define stb_vorbis_get_samples_short libxmp_vorbis_get_samples_short
+
 #ifndef STB_VORBIS_C
 /* client: */
 #define STB_VORBIS_HEADER_ONLY

[sezero]

What else is left to do except for cdf072f?

If I'm not missing anything, only the above patch remains to be resolved for this ticket.

[AliceLR]

I haven't PRed any of the things in the initial post upstream yet... :(

[sezero]

Should we pull in your nothings/stb#1312 from mainstream?

[AliceLR]

Should we pull in your nothings/stb#1312 from mainstream?

I don't think it needs to be pulled in separately—currently I have it as part of the UBSan fuzzing patch I'm working on: master...AliceLR:fuzz-patch-20220315

[sezero]

@AliceLR: Do we want this one here: nothings/stb#1563 ?

[ericoporto]

@AliceLR: Do we want this one here: nothings/stb#1563 ?

Hey, I wonder if this should be added in SDL_Sound too.

[AliceLR]

@AliceLR: Do we want this one here: nothings/stb#1563 ?

Yes, that seems reasonable.