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

[Ulion]

2007/11/8, Ulion <ulion2002 at gmail.com>:
> 2007/11/8, Ulion <ulion2002 at gmail.com>:
> > 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.
> >
>
> It works fine. If no object, I will apply this patch after two days.
>

Applied.

-- 
Ulion