[MPlayer-dev-eng] [PATCH] Fix dead lock in ao_macosx when switch stream format.

[Ulion]

2007/11/8, Reimar Döffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de>:
> Hello,
> [...]
> > > > If it ever doesn't print "reached timeout" your compiler is very stupid.
> > > > IOW this is completely broken and you could just remove the whole
> > > > callback stuff instead.
> > >
> > > I didn't figure what you mean. On my machine, the output is like this when init:
> > >
> > > AO: [macosx] setting stream format: 48000.0Hz 16bit
> > > [cac3][94][6144][1536][0][2] int BE S packed aligned
> > > AO: [macosx] actual format in use: 48000.0Hz 16bit
> > > [cac3][94][6144][1536][0][2] int BE S packed aligned
> > >
> > > and this when uninit:
> > > AO: [macosx] setting stream format: 44100.0Hz 16bit
> > > [lpcm][30][4][1][4][2] int BE S packed aligned
> > > AO: [macosx] actual format in use: 44100.0Hz 16bit
> > > [lpcm][30][4][1][4][2] int BE S packed aligned
> > >
> > > No timeout printed, please check my patch again.
>
> Are you sure you are not compiling with -O0 or -O1?

It's -O4, this result from PPC, I will do Intel double core run check.

>
> > > > This is exactly why writing lockless code comes after fully
> > > > understanding how to use locks correctly, fully understanding how the
> > > > CPU you work with works and realizing that it's pure luck if it works at
> > > > all without any asm code.
> > >
> > > asm code? I missed something?
> >
> > I add 'volatile' declaration to the flag int, should this be safe?
>
> I practice since this is Intel/PPC only anyway yes. In theory volatile
> is not enough, it only stops the compiler from optimizing the check
> completely away but it does not insert any memory barriers possibly
> necessary to make both CPUs see the same data (e.g. if there is no cache
> coherency).
> Which is why just moving those unlock and destroy calls a bit further up
> still seems like the simpler solution to me, but I'm not going to
> discuss long because of Mac stuff I don't use anyway.
>
> [...]
> > +    static volatile int stream_format_changed;
> > +    stream_format_changed = 0;
>
> Why static?

In case the remove call failed the callback still has a valid address
to assign. Although it won't fail normally.

> Which reminds me, if as you say AudioStreamRemovePropertyListener waits
> for the callback to finish anyway, what is the point of having a timeout
> at all? There is a comment trying to explain it but it makes no sense to
> me...

It only wait the callback when the callback is calling, if the
callback is still not called, it won't wait, just remove the callback.
The timeout is for in case the callback is called too late or not get
called, and we can't block on this change format call too long or
forever.

>
> [...]
> > +            if (NULL != inClientData)
> > +                *(volatile int *)inClientData = 1;
>
> A simple
> > if (inClientData)
> seems preferable to me, though the check seems unnecessary anyway since
> we only call it with argument != NULL.
>

OK.

Then I will test this on Intel double core machine for bunch of times
to see whether it timed out after the callback get called.

-- 
Ulion