[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, 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.

-- 
Ulion