mpg123 / Bugs / #344 portability issues and possible libmpg123 v2 API and ABI considerations

Thomas Orgis - 2022-09-20

I will be taking some time considering the steps you propose. My initial reaction is laid out in the other bug. Maybe things change after some days (being busy with other things, anyway).

I do see your point that the file handling in libmpg123 seems overly eclectic. Some of that comes from it being born out of cutting code out of the mpg123 console player. There are frameworks that like to do their own I/O, and hence even the feeder API is popular there, but I got repeatedly complimented for the easy to use API … some folks actually write programs that load MP3s directly and they like doing open/read/close just like with a normal file. In the end, that's a minor thing for the library. It is a decision if you want the API to be absolutely minimal in symbol count, or if convenience wrappers should exist.

Before getting carried away in details, I just wanted to mention, on your point 15, that we do have a github mirror on https://github.com/madebr/mpg123 . It is mentioned on the download page, but it is semi-official only. We could add CI stuff there. Not sure what sf.net actually would offer here. Anyhow, I am not opposed to automatic testing. I have reservations about the main repo being on github, mainly because just about everyone is there and people assume that no other hosting exists. All eggs in one basket … don't like that just out of principle. I'd rather eye something like sourcehut. But maybe the github one would be needed for the Microsoft-friendly (surprise!) CI pipes. We do have nightly regression tests on Linux, but no platform port testing.

Anyhow, Let's see if I can chew on and digest this over the weekend. My hunch is that I'll go mostly with my plan — and do the work inside our codebase, but I do want to give your suggestions a fair chance of persuading me at least on some points.

Last edit: Thomas Orgis 2022-09-20

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Thomas Orgis - 2022-09-25

Damn, now I'm reading up on the history of long long in the standard. https://yarchive.net/comp/longlong.html funny read … especially the projections about computer hardware in 2020.

I'll pick up some points now. My thought is about extending the current mpg123 API, not replacing it, but most of your list still applies for the new stuff, I presume.

0. I am willing to offer a preprocessor switch that omits the off_t and ssize_t API (as the latter is tied to off_t-using functions anyway). I won't call it deprecation, though. I am not intending to delete the other stuff during the lifetime of libmpg123 version 1. There might be a version 2 in the distant future, probably by someone else than me. That would also be the point where libmpg123 got fully split from mpg123 the application and the other libs, I guess. Right now, they share a deal of internal code, are developed together, and there's no benefit for the very small developer team in increasing the count of repositories.

I hope you can limit the scope of the API relevant to you with that switch in mind and I have to worry about the other baggage.

I'll call it MPG123_PORTABLE_API for now. I guess this should also hide mpg123_open_fd() and the plain mpg123_replace_reader(), as it could be easily misused. Anything that opens questions regarding compatible toolchains, etc. What jumps into my eye there:

/** Decode from windows-1252 (the encoding ICY metainfo used) to UTF-8. * Note that this is very similar to mpg123_store_utf8(&sb, mpg123_text_icy, icy_text, strlen(icy_text+1)) . * \param icy_text The input data in ICY encoding * \return pointer to newly allocated buffer with UTF-8 data (You free() it!) */ MPG123_EXPORT char* mpg123_icy2utf8(const char* icy_text);

Memory allocated inside libmpg123 and freed by the user. Dangerous. I could introduce a mpg123_mem_free() for this purpose, just wrapping over free() in the mpg123 build … but since this is a rather fringe use case and the same can be achieved using mpg123_store_utf8(), just dropping the definition with MPG123_PORTABLE_API is good enough.

1. We need headers for size_t and the types we intend to use. That's stddef.h for one, and stdint.h if int64_t enters the game. We get ptrdiff_t with those, but not ssize_t for the non-portable API. Actually, ptrdiff_t for something that is not a pointer difference irks me. I am currently leaning towards asking what's the deal with all the typedefs if we throw out semantics anyway … and just using longs, long longs or this-is-64-bit-integer.

2. When mpg123.h is installed system-wide, it better should default to the system-wide fmt123.h. If you want to use the local one, add -I. to flags. But then, include with quotes is implementation-defined, anyway. You sure that it would be an improvement to switch to that in a header that is usually (heh, let's assume use cases!;-) installed system-wide?

3. As I said, I am wondering what good typedefs really do. Somehow, since we're wrapping stuff anyway, I wonder if the (since when? still reading …) standard long long wouldn't be better, even. At least 64 bits, but possibly more on a crazy platform that does 128 bit offsets.

4. Possibly. If we're not doing a fully new API, this point is moot. It would be mpg123_enum_or_bitwise_combination_of_them;-) I think just int is darn practical there. Somehow an API that just uses basic types without any _t stuff gains appeal for me …

5. Also, only relevant for a v2 API. Cannot just change things. But we can decide what to use for the read callback in the added portable mpg123_replace_reader_handle() (maybe with a shorter name, mpg123_iosetup()?). The restricted range of ssize_t return with size_t argument is already stupid. Could both be long, actually. You see, I'm getting a bit annoyed by a thousand names for a handful of basic types;-) But yes, long would be bad for Windows. I do like being able to drop in pointers to the system's read() and lseek() on POSIX (or emulations thereof) …

6. Careful there. Most places really are fine. For debug prints, there's OFF_P and the cast to off_p, which can resort to long if intmax_t is not there. You mean there are overflows in the casts to long in that case? Or did you find places where the cast is missing? Using more long long can indeed ease things with %lld being usable. The long type is not useless. It's for variables that are fine with 32 bits and should be implemented using the most convenient type for that on the current architecture. For computations, it can make a huge performance difference if you are throwing 32 bits or 64 bits around per number. I'd assume long to be something sensible that maps to machine registers on a 32 bit system.

7. Well, wishlist for a v2 API, but yes, all API exposed with MPG123_PORTABLE_API shall not be subject to the largefile renames. I'd have to keep the enum renames, though. You are not suggesting dropping those in favour of the possibly build-dependent enums?

8. API version from header … you'd like to get away from a preprocessed header (.h.in to .h). I can somewhat understand that. Well, one could replace code in configure.ac to grep and cut them out of the headers.

9. Only with a v2 API … and even then, too many numbers, for my taste. Actually, mpg123v2 prefix alone is an argument for me to not to do a separate API, from an aesthetic viewpoint;-)

10. Same.

11. Ok, this is another can of worms. Actually, only on Windows does this matter at all. POSIX just doesn't care about the encoding of the binary blob handed to open(). So we settled on everything being UTF-8 and added code to funnel wide characters in and out. The Windows build of mpg123 gets wide characters in argv, translates them to UTF-8, compat_open() translates back using u2wlongpath(). Encoding sucks. So I'll take something that works when everything's ugly. In fact, mpg123 the application isn't using plain mpg123_open() right now, only mpg123_open_fd() and mpg123_open_handle(). The extra tools mpg123-id3dump and mpg123-strip do, as do examples, since it is a lot simpler than juggling with callbacks.

It could be a separate issue to add a mpg123_wopen() … but I'd fear to implement that right on POSIX where there is no underlying wopen(). I'd have to decide which encoding to convert to for the current locale. I'd rather really not do that.

12. hidden with portable API

13. my problem, internally

14. Except that the long names are annoying. But I do get your point. The header is a measure to avoid conflicts with non-static internal symbols for static linking. I'd prefer a world where that is not necessary. Maybe one could rework things to make more symbols local/static so that fewer of these ugly names are needed. But really, it's a separate issue for the future.

15. I covered that earlier. There is a github mirror. We could add CI there. Or elsewhere. A topic separate from the API.

16. Good.

Last edit: Thomas Orgis 2022-09-25
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Thomas Orgis - 2022-10-02

You got something to add?

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

manx - 2022-10-06

I am sorry for the late reply.

If you want to keep all current ABI and API, MPG123_PORTABLE_API may be a way to go. I still suggest new symbol names for the new functions: The portable API functions would not rely on ssize_t, but instead ptrdiff_t (or anything else that is actually available everywhere). As these types only somewhat guarantee the bit width and in particular not the exact type used (int/long/long long), a change here could cause a mismatch of declared and defined function type between the 2 APIs. I.e. you would not be able to use the same function implementations for both the old API and the portable API due to that anyway.

There already is mpg123_free(void*), so mpg123_icy2utf8() is fine as is. See https://sourceforge.net/p/mpg123/bugs/276/, which is exactly the same issue that I was talking about. Documentation of mpg123_icy2utf8() could be improved to mention mpg123_free().

ptrdiff_t is the only available type in the C standard that somewhat meets the requirements: 1. about the size of size_t, 2. signed. There is also intptr_t, but that type is optional in the standard. For the old API, you need to include the non-standard POSIX header <sys/types.h>, however that must only happen for not MPT123_PORTABLE_API. Frankly, I do not like using ptrdiff_t as ssize_t that much either, it's just the closest we have got that is actually portable across all platforms. FWIW, I am not aware of a platform where sizeof(ptrdiff_t) != sizeof(size_t), even though that would be allowed by the standard. But keeping that in mind, providing an easy way to use a different type via a simple typedef ptrdiff_t mpg123_ssize_t; in the header is probably a good idea. As the only use of ssize_t is the reader API, getting rid of it completly and just using int64_t might also be an option. Another, arguably even better approach would be to just not overload a single return value with 2 different meanings (1. returned data amount, 2 error code) by e.g. returning a struct, or returning an error and using an out parameter for the returned data.

I think you maybe somewhat missing the point here. The system-installed mpg123.h will catch the system-installed one with both <fmt123.h> or "fmt123.h". The local development mpg123.h will currently always catch the system-installed <fmt123.h>. It would catch the local one with "fmt123.h". A local development mpg123.h should better NOT catch the system-installed <fmt123.h>, because then you cannot ever develop an updated fmt123.h when libmpg123 is installed. <> prefers the (system) include path. "" preferes the local directory of the current file. Headers of the same project should always use public headers from the same project via the current directory, otherwise you do get version mismatch if another version of the project is installed. Adding -I. or whereever the relative path to a local fmt123.h would be, works around this issue, however that in turn again poses additional requirements on the build system that just do not need to be there. This has actually bitten us when building libopenmpt with XCode (https://github.com/OpenMPT/openmpt/commit/c818518b0afe65a26f1b11829b6a9b5d1ce14515, I do not remember the precise cause here though; might have been a different order in which XCode appends/prepends project include paths to the compiler's set of system include paths, and/or some interaction with some include paths being -isystem and some overriding behaviour). I guess there would also be similar problems if you build multiple dependend projects with Autotools without installing the intermediate libraries system-wide (but I am not familiar with such build systems).

With some non-existent 128bit platform, you will have to update the API and implementation anyway. There is absolutely no point in optimizing the API for non-existent platforms. Just use an ABI-forcing 64bit int64_t. Otherwise, you absolutely will have the very same stupid off_t issues with long long in the future on 128bit platforms. Semantic typedefs could the API more readable. It is actually very helpful to be able to distinguish values in the API if they are clearly named mpg123_off_t (local file offset) or mpg123_samplecount_t (sample offset). I do not have a strong opinion on that though, so just int64_twould also be fine. Also, 64bit is just enough, so there is nothing gained from planning for hypothetical 128bit file or sample offsets (63bit sample offsets allow for 6 million years at 48kHz, 63bit file offsets at 320kbps allow for even more). long long exists since C99 and C++11 (in practice also before C++11 as a non-standard extensions, when the same compiler also supported C99).

Yeah, int is fine I guess, as that is the type of enum values in the C18 standard (that changed with C23, see https://thephd.dev/c23-is-coming-here-is-what-is-on-the-menu / https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3030.htm).

Just reusing pointers to system functions will not work portably. Force-casting them to function pointers with different declared argument types is undefined behaviour. And, no, you cannot use long here at all. On LLP64, long is 32bit and size_t is 64bit. sizeof(long) is unrelated to both, sizeof(int) and sizeof(size_t) when looking at a common subset of different platforms. There is no situation in a portable API to ever use long. It makes no sense. Just using int64_t for file offsets and return values would be fine for me. I do not have any strong opinion about how functions are called, so renaming would be ok too.

Yes, a lot of the debug printf do cast off_t to long, which is just wrong. long long with %lld or int64_t with PRId64 would avoid truncation on ILP32 platforms. long is not fine here, as it cannot represent 64bit values on all platforms. If you would want (which you do not, and I frankly cannot imagine any situation why anyone would ever want that such a type as optimizing compilers exist and we are not in the 80s any more) the "most convenient type of at lest N bits width", there is int_fastN_t; long is not the type you are looking for here. The only places where fundamental types make sense are in situations that they are used/specified by the standard directly (int for errno and enum, for example). There is no place where long (except fseek/ftell, which is broken anyway) is used exclusively in the standard.

Yes, without also dropping the enum renames, this will IMHO again cause maintanence hassle in the future. It's just hard to use and unneccessarily difficult to read for users (you, as the author, are probably familiar with all the renames, but for me as a user they are just very much annoying and wasting time for absolutely no reason). Just mark the old not-to-be-used API deprecated, let the user explicitly call the new names. Yes, this will require users to change their code, but the major upside is that code will converge towards a state where all this trickery can be removed, instead of piling up even more trcikery basically forever.

That should not be too hard I guess: grep MPG123_API_VERSION mpg123.h | awk '{print $3;}'. I do not think anything more fancy would be waranted here. However, getting rid of modifying the header also requires getting rid of the conditional largefile renaming, which, when you want to keep all that, might need a separate NEW header mpg123_portable.h for the portable API. Applications would need to check MPG123_API_VERSION (from the old header) in order to decide whether mpg123_portable.h would be available though, and MPG123_PORTABLE_API would still be required to remove all non-portable things from the old header. However, mpg123.h would still depend on the build system, and I absolutely hate that. So this does not work. What could maybe be possible is moving all renaming into a separate mpg123_deprecated.h and including that from mpg123.h only if !MPG123_PORTABLE_API. That way, mpg123.h could become static.
Now, this might actually turn out to be a good first step in any case: grep the version from the header, move anything that is not portable into a separate header, and add MPG123_PORTABLE_API which disables inclusion of that header. After this first step, MPG123_PORTABLE_API would be dysfunctional as it is missing crucial functionality, but all of that can be added back later, as even now, code could set MPG123_PORTABLE_API, then check MPG123_API_VERSION and if that's too low, still #include <mpg123_deprecated.h> until the new portable API would be complete. I will explicitly state that I consider being able to detect and solve all these conditions solely in client code via the preprocessor is very important to me. Otherwise, this would need to induce logic into the build system of client code.

From an aesthetic viewpoint, any preprocessor renaming of API names is also in the "disgusting as hell" department for me.

Renaming the library itself would be kind of necessary when breaking ABI though, as there otherwise would be no way to use the old and new one side-by-side in the same process (which is basically a requirement to allow for smooth transitioning).

Yeah, that is kind of a pet peeve of mine: There is absolutely no way to portably open a file without being aware and vary about encodings and string types. So, assuming the way forward would be some kind of MPG123_PORTABLE_API and keeping all current APIs, I suggest just also disabling mpg123_open and related functions for MPG123_PORTABLE_API. The correct portable way involves opening in client code and using mpg123_replace_reader already anyway.

Agreed.

sure

14./15. Yeah, separate concerns.

Last edit: manx 2023-01-23
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Thomas Orgis - 2023-02-12

Comments on the prior of syn123 (only two off_t-using functions) in revision 5238? There's src/libsyn123/syn123.h now (no h.in) and syn123_resample_total64() as explicit int64_t symbol now, along with a(nother) switch toget rid of off_t API in the header, but still wrapper/alias definitions. On x86_64, I get

$ nm -g src/libsyn123/libsyn123.a |grep _total 000000000000ee49 T syn123_resample_total 000000000000eeab T syn123_resample_total_64 000000000000eb60 T syn123_resample_total64

Those all amount to the same functionality, but with differing prototypes. I'd need to do a 32 bit build, too, which should add a _32 one (and change what the suffix-less one is). On LFS-insensitive systems, you get the _64, anyway, to keep the header fixed. I hope BSD people are not too furious at that waste of bits.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Thomas Orgis - 2023-02-12

Description has changed:

Diff:

--- old +++ new @@ -1,4 +1,3 @@ - Part 3/3 I'll reply in 3 parts:
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Thomas Orgis - 2023-02-12

Hm, this needs fixup for 32 bit platforms … I need to get rid of the largefile stuff in configure that turns off_t into 64 bits and use _LARGEFILE64_SOURCE instead.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Thomas Orgis - 2023-02-12

Rev. 5240 now. Please don't fret too much about the off_t aliasing/wrapping, but focus on what you get with SYN123_PORTABLE_API.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Thomas Orgis - 2023-04-18

I/O stuff can be handled, turning non-portable off_t API into wrappers over the new stuff. But for the portable case, there's some more functions that might need suffix-64 variants. So, all funcs with off_t int them someplace:

int mpg123_decode_frame( mpg123_handle *mh, off_t *num, unsigned char **audio, size_t *bytes );

int mpg123_framebyframe_decode( mpg123_handle *mh, off_t *num, unsigned char **audio, size_t *bytes );

off_t mpg123_framepos(mpg123_handle *mh);

off_t mpg123_tell(mpg123_handle *mh);

off_t mpg123_tellframe(mpg123_handle *mh);

off_t mpg123_tell_stream(mpg123_handle *mh);

off_t mpg123_seek( mpg123_handle *mh, off_t sampleoff, int whence );

off_t mpg123_feedseek( mpg123_handle *mh, off_t sampleoff, int whence, off_t *input_offset );

off_t mpg123_seek_frame( mpg123_handle *mh, off_t frameoff, int whence );

off_t mpg123_timeframe(mpg123_handle *mh, double sec);

int mpg123_index( mpg123_handle *mh, off_t **offsets, off_t *step, size_t *fill );

int mpg123_set_index( mpg123_handle *mh, off_t *offsets, off_t step, size_t fill );

int mpg123_position( mpg123_handle *mh, off_t frame_offset, off_t buffered_bytes, off_t *current_frame, off_t *frames_left, double *current_seconds, double *seconds_left);

off_t mpg123_framelength(mpg123_handle *mh);

off_t mpg123_length(mpg123_handle *mh);

int mpg123_set_filesize(mpg123_handle *mh, off_t size);

int mpg123_replace_reader( mpg123_handle *mh, mpg123_ssize_t (*r_read) (int, void *, size_t), off_t (*r_lseek)(int, off_t, int));

int mpg123_replace_reader_handle( mpg123_handle *mh, mpg123_ssize_t (*r_read) (void *, void *, size_t), off_t (*r_lseek)(void *, off_t, int), void (*cleanup)(void*) );

Not all of them need to be present in portable API. Obvious ones that are necessary are simple inquiry functions like mpg123_tell, mpg123_tellframe. mpg123_position is a relic, anyway, and should be replaced by code in the mpg123 main program doing the calculations. One thing that's a bit nasty is mpg123_index. It directly returns the pointer to the internal index array. If we were to drop off_t from the build, this function would need to construct a fake buffer for that purpose to be able to offer the off_t shim. Oh, dear. I just see that I did exactly that in lfs_wrap.c already. Meh, so deep in there already. Have to think if I continue to define mpg123_index64. I guess I could, as the wrapper code already exists for inefficient handling of 32 bit conversion. I hope you don't look that close when I make the 64 bit off_t variant of mpg123_index just cast the internal int64_t pointer to off_t, the 32 bit one doing what it does now with the copy;-)

One might add a more proper mpg123_get_index64 to mirror mpg123_set_index64, always copying the index and managing the user buffer, but I intend to make use of the fact that I am switching to an unambiguous type. and avoid the unnecessary copy of the buffer just to print it out in the mpg123 program (or store away to a file in other applications).

Not sure if I'll include framebyframe_decode in portable API. This was actually rather specific API for a previous project member's own player software. This is 1 of 18 functions, so no big difference.

I wonder what functions I could omit from a 64 bit API. I could replace a number of position query functions with a single one with optional pointer arguments, but that's less usable than a plain mpg123_tellframe64. Skipping parts doesn't really make sense for code that is contained in libmpg123, anyway (like the feeder buffer handling). Omitting certain stuff from the portable API would open the path for a trimmed-down libmpg123 build (optional) that actually removes code for the old API. But then, I do already carry configure switches for getting rid of code like that. A minimal build with only handle I/O with 64 bit integers will be available naturally, just not the default.

So I guess the default build will just grow with 16 additional symbols (dropping mpg123_position and only adding one variant of mpg123_replace_reader simply as mpg123_reader --- or as mpg123_iosetup, even if there is only I, not O):

int mpg123_decode_frame64( mpg123_handle *mh, int64_t *num, unsigned char **audio, size_t *bytes );

int mpg123_framebyframe_decode64( mpg123_handle *mh, int64_t *num, unsigned char **audio, size_t *bytes );

int64_t mpg123_framepos64(mpg123_handle *mh);

int64_t mpg123_tell64(mpg123_handle *mh);

int64_t mpg123_tellframe64(mpg123_handle *mh);

int64_t mpg123_tell_stream64(mpg123_handle *mh);

int64_t mpg123_seek64( mpg123_handle *mh, int64_t sampleoff, int whence );

int64_t mpg123_feedsee64k( mpg123_handle *mh, int64_t sampleoff, int whence, int_t *input_offset );

int64_t mpg123_seek_frame64( mpg123_handle *mh, int64_t frameoff, int whence );

int64_t mpg123_timeframe64(mpg123_handle *mh, double sec);

int mpg123_index64( mpg123_handle *mh, int64_t **offsets, int64_t *step, size_t *fill );

int mpg123_set_index64( mpg123_handle *mh, int64_t *offsets, int64_t step, size_t fill );

int64_t mpg123_framelength64(mpg123_handle *mh);

int64_t mpg123_length64(mpg123_handle *mh);

int mpg123_set_filesize64(mpg123_handle *mh, int64_t size);

int mpg123_reader( mpg123_handle *mh, int (*r_read) (void *, void *, size_t, size_t *), int64_t (*r_lseek)(void *, int64_t, int), void (*cleanup)(void*) );

Would it make sense to rather name the last one mpg123_reader64 (or mpg123_iosetup64, or mpg123_stream_setup64 … opinion about that?) for consistency? So that all int64_t-based functions have that suffix? Or try to coin new names altogether that don't conflict with the old ones, avoiding the explicit suffix, but deviating from some rather obvious names?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

manx - 2023-04-20

I/O stuff can be handled, turning non-portable off_t API into wrappers over the new stuff. But for the portable case, there's some more functions that might need suffix-64 variants. So, all funcs with off_t int them someplace

sounds fine

mpg123_position

Given the documentation already deprecates it, maybe make the source also deprecate it. Almost nobody reads documentation, but __attribute__((deprecated)) (and similar) will get noticed. There is of course no need to carry any deprecated function over to a new API.

mpg123_index

So, the old API leaks the off_t internal implementation detail to API users. For the new API, the internal table obviously needs to be 64bit anyway, so an emulation of the old API on top of that would need to store that copy somehere, which is unfortunate.

mpg123_get_index64 sounds fine to me.

framebyframe API

It's documented as "experimental", thus not bringing it to the portable API sounds totally fine to me. If someone wants it, it can probably always be added later on. If the API is so much experimental that it should not be used at all, marking it deprecated might also make sense.

I wonder what functions I could omit from a 64 bit API. I could replace a number of position query functions with a single one with optional pointer arguments, but that's less usable than a plain mpg123_tellframe64.

I do not have any real opinion on that one. There are valid arugments for single purpose functions, as well as for single functions querying all such info at once.

Omitting certain stuff from the portable API would open the path for a trimmed-down libmpg123 build (optional) that actually removes code for the old API. But then, I do already carry configure switches for getting rid of code like that. A minimal build with only handle I/O with 64 bit integers will be available naturally, just not the default.

I like that! :D

So I guess the default build will just grow with 16 additional symbols (dropping mpg123_position and only adding one variant of mpg123_replace_reader simply as mpg123_reader --- or as mpg123_iosetup, even if there is only I, not O):

list look ok at a first glance (there is a typo in mpg123_feedsee64k)

Would it make sense to rather name the last one mpg123_reader64 (or mpg123_iosetup64, or mpg123_stream_setup64 … opinion about that?) for consistency?

I do not have a strong opinion on reader vs reader64 vs iosetup64. I really dislike calling (potentially) seekable things "stream" though (C++ also messed this one up).

So that all int64_t-based functions have that suffix? Or try to coin new names altogether that don't conflict with the old ones, avoiding the explicit suffix, but deviating from some rather obvious names?

If the previous name was a good fit, I do not think changing the name makes much sense. So, suffixing "64" or "_2" or "_v2" would all work for me. If the previous name was rather awkward ("replace_reader_handle" comes to mind), a completely new name makes sense. I guess making them consistent, by also using the coosen suffix, would be helpful, but again, no strong opinion on that.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Ozkan Sezer - 2023-08-23

svn r5304 took a drastic action :

non-portable off_t / off64_t stuff out of the way: good.

--enable-portable without even any seek-to or tell functionality:
bad, IMO.

Yes MPG123_LARGENAME thing was, and still is, an abomination.
And mpg123_replace_reader also was, and still is, an abomination.
But if you are going to break things, just break things: Why not
just drop the old file i/o stuff and provide two simple methods:
One set with long offsets and one set with int64_t offsets, both
available in both 32 and 64 bit systems.

At the very least, provide the simplicity of libvorbis or opusfile
by adding, for e.g., a new mpg123_opencallbacks (with long offsets)
and a new mpg123_opencallbacks64 (with int64_t offsets) both being
available even with --enable-portable in effect so that mpg123_seek,
tell and friends can still be accessible.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Thomas Orgis - 2023-08-23

I hoped to make everyone happy by both adding the portable API with fixed int64_t and keeping all old API paths working. The --enable-portable switch is only for people who want to do a stripped-down library build. You are free to ignore it. But: Where did you get the impression that there is no seek or tell functionality? There arempg123_seek64() and mpg123_tell64() (and friends). At least they should be.

I took pains not to break anything. A default library build on x86 gives you

$ nm -g src/libmpg123/.libs/libmpg123.so |grep -e mpg123_seek -e mpg123_tell 00038f90 T mpg123_seek 00039d70 T mpg123_seek_32 0003a290 T mpg123_seek_64 0001a770 T mpg123_seek64 000390e0 T mpg123_seek_frame 00039df0 T mpg123_seek_frame_32 0003a320 T mpg123_seek_frame_64 0001a050 T mpg123_seek_frame64 00038e10 T mpg123_tell 00039cb0 T mpg123_tell_32 0003a1d0 T mpg123_tell_64 00019970 T mpg123_tell64 00038e90 T mpg123_tellframe 00039cf0 T mpg123_tellframe_32 0003a210 T mpg123_tellframe_64 00019b40 T mpg123_tellframe64 00038f10 T mpg123_tell_stream 00039d30 T mpg123_tell_stream_32 0003a250 T mpg123_tell_stream_64 00019bc0 T mpg123_tell_stream64

What is missing here? Any build with or without _FILE_OFFSET_BITS should just work and find its functions with 32 bit off_t or 64 bit off_t.

Also, did you miss the addition of mpg123_reader64()? Or what else do you mean with mpg123_opencallbacks64()?

(I just noticed that I messed up the O_LARGEFILE detection … neutered the actual largefile opening on x86 kernels … will fix tomorrow.)
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Ozkan Sezer - 2023-08-23

Only mpg123_seek|tell64 are available in portable build, not mpg123_seek|tell, but OK I guess.

mpg123_reader64: I do seem to have missed that. You do mpg123_new(), then call mpg123_reader64 to set callbacks, yes?

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
- Thomas Orgis - 2023-08-24
  
  The workflow is
  
  mpg123_new();
  mpg123_reader64();
  mpg123_open_handle();
  mpg123_seek64();
  mpg123_tell64();
  mpg123_read();
  mpg123_close();
  
  or you can continue to use the mpg123_open_feed() path, also withmpg123_tell64() and mpg123_feedseek64(), in that case.
  
  But if you don't fancy the 64 suffix, nothing has to change. You just might have trouble representing sample offsets above 2 billion in your app, but libmpg123 will decode the large file.
  
  If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Thomas Orgis - 2023-08-24

Or would a mpg123_open64(handle, readerfunc, seekfunc, cleanupfunc) make sense? I guess it would complicate things … and introducing a mpg123_read64() and mpg123_close64() for consistency is maybe just wasting bits.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
- Ozkan Sezer - 2023-08-24
  
  The workflow is
  
  Yep, got it
  
  Or would a mpg123_open64(handle, readerfunc, seekfunc, cleanupfunc)
  
  Nope, not necessary (not when old stuff is around.)
  
  If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Thomas Orgis - 2023-09-12

So … this is coming towards closure now. I'll prepare the current state of the changed API for release soon. Last chance to speak up.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Thomas Orgis - 2023-09-23

status: open --> closed-fixed
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Thomas Orgis - 2023-09-23

After a long time … 1.32 offers this. Some minor cleanup will follow.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

portability issues and possible libmpg123 v2 API and ABI considerations

Release Series

Searches

Help

#344 portability issues and possible libmpg123 v2 API and ABI considerations

Discussion