[Fwd: Re: winemp3.acm: Fixes the Mac-specific code]

[Kristofer Henriksson]

I think the reason why your change works is that it seems the
AudioConverter doesn't care if it has invalid data at the beginning of
its stream. There will be however a little creeping error in
computation of frame lengths over time, which is that there may or may
not be one extra byte of padding on any given frame, and you need to
look at the frame header to know if it's there. Ignoring the padding
is essentially equivalent to ignoring the remainder in the division
you do to get the frame size.

I'm still perplexed why the very first call to the conversion function
will not start the source data with an ID3 tag or a frame header. The
data shouldn't start in the middle of a frame, and there should be
nothing else in an MP3 file than frames and ID3.

At any rate, I can fix my code to properly sync to MP3 frames, and
then the padding should be properly picked up as well.

Regards,
Kristofer

On Thu, May 13, 2010 at 12:46 PM, Aric Stewart <aric at codeweavers.com> wrote:
> There is a bug in the code i just posted,  It should clearly be
> adsi->pwfxSrc->nSamplesPerSec not Dst.
>
> Still works great,  probibly because my tests had the same nSamplesPerSec
> for both Src and Dst.
>
> -aric
>
> Aric Stewart wrote:
>>
>> Hi,
>>
>>  I will admit that my understanding of both ACM drivers as well as the os
>> x audio libraries are not perfect and mostly come from work on this code
>> itself. So i will fully admit it is most likely full of areas needing
>> improvement.
>>
>> Kristofer Henriksson wrote:
>>>
>>> Aric,
>>>
>>> I may have been operating under some faulty assumptions. I thought
>>> that all mp3 files would "begin at the beginning" and also keep track
>>> of how much of the source data the convert function said that it used.
>>> I am very careful to set the value of *nsrc to agree with how much
>>> source data was successfully used and assume that the calling function
>>> with resend the data that is not used. If this is done, it ensures a
>>> valid frame at the start of each invocation.
>>
>> Yes, a proper application should do just that. It should be tracking the
>> value in nsrc and resend unused data at the beginning of the next buffer.
>>
>> You get a lot of the expected source stream format from MPEG3_StreamOpen.
>>  With that i wonder if the presence of an ID3 tag becomes optional.  My
>> familiarity with mp3 format makes me unsure if a packet header would also be
>> required.
>>
>>>
>>> As far as using AudioFileStream, I tried for a bit to get it to work
>>> before I wrote my own custom mp3 parser. I'm running Snow Leopard
>>> (10.6.3) and never got the background music to work in Oblivion. One
>>> of the problems is that Mp3AudioConverterComplexInputDataProc needs to
>>> return an error if it will have more data in the future, rather than
>>> returning zero. However, even changing this, AudioFileStream seemed to
>>> be skipping a lot of mp3 frames.
>>>
>>
>> The lack of an error in that return is probably an oversight on my part.
>>  All the mp3 clips in the game i was working with where sent in one piece so
>> there was only one pass through the loop that consumed all of the data. I am
>> unsure why skipping mp3 frames would occure. My guess is that I am feeding
>> the data in incorrectly and on those boundries between feeds frames are
>> being ignored/corrupted or the like.
>>
>>> If you tell me what are more appropriate assumptions I should be
>>> making in my parser, I can fix it to work the way it should. I suppose
>>> I am also assuming that it will usually receive at least one full
>>> packet, and if this is unjustified I can fix this as well. If I should
>>> expect calling functions to ignore the value of *nsrc, then I can just
>>> implement an internal single-packet buffer, and this should take care
>>> of the problem.
>>>
>>
>> I think the assumptions we need to investigate is the one about there
>> being a packet header at the beginning.  Maybe for non variable bit rate
>> mp3s we just get the data given to us when the stream is opened.
>>
>> In fact, going on a hunch i just got your code to work great with my game!
>>
>> What i did was this:
>>
>> I added PACMDRVSTREAMINSTANCE adsi to Mp3GetPacketLength  and then when
>> you would return -1 if you could not determin the header I instead returned
>> (192000 * 144) / adsi->pwfxDst->nSamplesPerSec.
>>
>> I picked that because it shows up in other areas of the code where we need
>> to get a length.
>>
>> This is exciting also because there where a few mp3s that whre returning
>>  'Fmt?' errors from the stream code and would not play. But with your patch
>> and this change those clips played as well.
>>
>> Does that give you something to work with?  You clearly understand mp3
>> format better than I.
>>
>> -aric
>>
>