[MPlayer-dev-eng] [PATCH] Bug fix in af_add/del/switch/clr

[Jehan Pagès]

HI,

On Mon, Jan 24, 2011 at 4:17 AM, Reimar Döffinger
<Reimar.Doeffinger at gmx.de> wrote:
> On Wed, Jan 19, 2011 at 02:29:03PM +0900, Jehan Pagès wrote:
>> Though it seems unlikely to load filters every few milliseconds to
>> notice this, the user who reported the bug used an audio equalizer
>> filter reloading. I guess such an equalizer could be very often
>> reloaded by a mplayer GUI while a user is trying to find the right
>> values to apply to each frequency band (I guess this person was indeed
>> developping some GUI to mplayer). And he wants to do this without
>> music speeding or video desyncing. And anyway that's a bug, so it is
>> good to fix. :-)
>
> There's a command to change the audio filter parameters, so for the
> equalizer case at least you don't have to do that.

I see. Anyway that's still a bug, and that could be relevant for other
filters (I don't know all the filters out there). :-)

>> Basically to fix, I removed calls to reinit_audio_chain(); after such
>> a dynamic load/unload of a filter. This was causing the issue and this
>> was an unecessary call actually as any sound reinitialization was
>> already dealed with in the functions which actually loaded/unloaded
>> audio filters.
>
> I would have to check again, but I think you are wrong: Only partial
> reinitialization, namely only of filters after the modifed one is done.
> IIRC this means that things like the "expansion ratio" might get wrong
> - or maybe not, but this needs to be checks carefully since unfortunately
> it is security-relevant.

I see. For the simple listening test at least, it looked all ok
(filter seemed added or removed with the right effect when the command
was issued, no more sound glitches and no more apparent sound frame
lost so that with many filter load/unload a second, there was no more
speed or desync issue). But I admit I have not really checked the code
very deeply. I got lazy.
Your analysis in this code will be much appreciated.

>> Attached is the small patch file.
>> You can try the bug reproduction before and after, and confirms it has
>> been fixed (I confirmed it on my machine and the user who reported me
>> the bug also did for him).
>
> Of course an interesting question is why does it fix it.
> What is reinit_audio_chain doing that causes it?
> Because probably reinit_audio_chain could and should be fixed,
> from which other places that use it might benefit as well.

Yeah for this too, at first I thought of and began to checking this
part of the code, but after a while, I also got lazy.
I'll see if I can find something anyway, unless someone finds before me. :-)
Thanks.

Jehan