Re: [mpg123-devel] [PATCH] new function to return samples per frame

[Thomas O. <tho...@or...>]

Am Fri, 25 Sep 2009 15:25:44 +0200
schrieb Patrick Dehne <pa...@my...>: 

> Vincent asked me to post this reply for him because of technical
> problems regarding mail servers:

So, I'm talking to TheVinn here. Just noting the obvious;-)

> Recently there was some discussion about returning the number of samples
> per frame. Someone suggested an extensible, flexible interface for
> retriving data:

... because that someone is not sure if we really tackle all available data with the second attempt at exporting "everything important about a frame". If the new struct really contains everything we ever want, then it's fine to add one specific function to retrieve that one.
Incidentally... the samples per frame are missing from your draft... because they are not stored.
We could change that, of course... it's storing more values verbatim against (simple) calculation on occasion.

> I need to voice my strong opposition to this idea. While I agree that
> this information needs to be available, this is not the way to do
> it. More function calls? More macros / constants? I believe this is
> counter-productive.

Well, both approaches add one function.

> These are the important goals:
> 
> 1. Avoid storing duplicate values
> 2. Add a minimum of new functions and constants
> 3. Export maximum information about the stream and relevant decoder state
> 4. Provide everything we can right before the actual decode, on every frame
> 5. Provide everything else right after each actual decode
> 6. Impose no additional performance overhead from the existing code path
> 7. Adds the smallest possible increase in the size of the compiled library

These goals sound fine so far.

> struct mpg123_handle_struct
> {
>   /* non-public fields */
>   struct mpg123_state state;
> };
> 
> The desired public fields would be moved from their current location in
> the mpg123_handle_struct, to the mpg123_state struct. The names would
> not change. As you can see, this is merely a renaming and does not
> affect performance at all.

Better structuring of the mpg123 handle is always an option, independent of the question what needs to be available externally. Sure... but well, we should discuss what members of that struct _not_ to put into the public part. 

> mpg123_state* mpg123_get_state( mpg123_handle* mh )
> {
>   return &mh->state;
> } 
> Adding new fields to the mpg123_state in the future is as easy as simplyc
> appending to the structure. Perfect binary compatibility is
> preserved.

Nope. Assume binary built with a newer version of the mpg123 header... using an older version of the library. It could try to access some new struct members and grab into void storage. At least you'd have to provide a second parameter with sizeof(mpg123_state) to enable sanity checking... still I do not really like extending structs of an established ABI... that's one reason the mpg123 handle is an opaque type.
Hm, and I'm getting a bit more vary by thinking about the ABI implications of using structs at all... compilers can pack / align the members differently.
But well, there seems to be some conventional standard and people messing with the compiler's binary layout options should know what they are doing.

> Since changing existing fields may break compatibility, we
> should discuss and carefully choose exactly how new information is
> exported. For example, consider the current implementation of
> mpg123_info():

We indeed can revisit the various fields and think about updating some of them. Have careful look at the current state of the code and decide what form is best.

>   if(mh->error_protection) mi->flags |= MPG123_CRC;
>   if(mh->copyright)        mi->flags |= MPG123_COPYRIGHT;

Like this... many fields can go into one integer as boolean flags. I don't feel like wasting 32bits for each of them.

> As you can see from this code snippet, mh->mpeg25 and mh->lsf are being
> transformed into a version enumeration. Since we do not want to make a
> duplicate of this data, we will have to choose one representation and
> stick with it.

One has to check how often we need what kind of distinction.

> Either the mpg123_state will have an enumeration, and
> libmpg123 code will be modified to use 'version' instead of mh->mpeg25
> and mh->lsf, or we will put mpeg25 and lsf in the mpg123_state.

Oh, and here lies a link to the initial question of samples per frame ... that info is a rather simple function of the mpeg version and layers ... the information is there, but should we help out with the knowledge or not? The spf() function (macro) is not explicitly used in the decoder core... I added it for the mid level to compute sample offsets... could be useful to have it stored -- when we freed some bytes by combining flags into one integer;-)
 
> The framesize is apparently different between the mh and the
> frameinfo. My opinion is that the '+4' is better left as part of the
> documentation instead of doing it in the code. Callers will simply need
> to understand that there are 4 bytes belonging to the header that are
> not reported. Alternatively, we can change libmpg123 to count the 4
> bytes when it computes the value of mh->framesize.

Well, there are arguments for both variants. It needs to be documented in any case.

> While an extensible architecture for returning decoder state values
> seems like a good idea initially, when you consider that the set of all
> possible relevant decoder information is quite finite and time-invariant
> it seems that this flexible approach is over-engineered.

Then make sure we got all possible information settled... still need to be careful with ABI compatibility.


Alrighty then,

Thomas.