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

[Reimar Döffinger]

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?

> > > 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?
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...

[...]
> +            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.

Greetings,
Reimar Döffinger