On Sonntag 24 September 2006 22:56, Martin Wache wrote:
> Stefan Lucke schrieb:
> > On Freitag 22 September 2006 06:37, Stefan Lucke wrote:
> >> On Donnerstag 21 September 2006 12:53, Martin Wache wrote:
> >>> Marko Mäkelä schrieb:
> >>>> On Wed, Sep 20, 2006 at 10:20:49AM +0200, Martin Wache wrote:
> >>>>> I would suggest to check what osdMutex is actually protecting, in some
> >>>>> places I really don't know that. So for protecting what is the osdMutex
> >>>>> needed?
> >> Added some comment on what osdMutex is protecting.
> // -----------------------------------------------------------------------
> // State changes of OSD like on / off transitions, must be proteced by
> // osdMutex. This are changes of variable OSDpresent, as the output method
> // may need to take some longer actions e.g clearing background for one or
> // mutiple output buffers (double, triple buffering).
> //
>
> I don't think that for this we actually need a mutex. I saw that you
> sent a patch which removes osdMutex from DirectFB. So is this still what
> you are thinking? Do we need osdMutex?
I looked too close at directfb and that was my actual impression.
Now I think different :-). The roots of osdMutex are in vdr-1.2.x:
OpenOSD() , CloseOSD()
In CloseOSD there is some code duplicated video.c:
void cVideoOut::CloseOSD()
{
osdMutex.Lock();
OSDpresent=false;
for (int i = 0; i < MAXNUMWINDOWS; i++)
{
if (layer[i])
{
delete(layer[i]);
layer[i]=0;
}
}
osdMutex.Unlock();
}
and now in video-xv.c:
/* ---------------------------------------------------------------------------
*/
void cXvVideoOut::CloseOSD()
{
cVideoOut::CloseOSD();
osdMutex.Lock();
#if VDRVERSNUM < 10307
for (int i = 0; i < MAXNUMWINDOWS; i++)
{
if (layer[i])
{
delete(layer[i]);
layer[i]=0;
}
}
#endif
[snip]
osdMutex.Unlock();
}
>
> >> For Action() of cVideoOut, osdMutex is the wrong one, as the existance
> >> and current contend of previous decoded frame has to be protected.
> >> Introduced a separate mutex for that purpose.
> >>
> >> That should fix the hang situation.
> >>
> >> Martin, what is areaMutex for ?
>
> Funny that you are asking me that question, you introduced areaMutex ;-)
> Please check video.c revision 1.30 and the corresponding log message.
Ok, then I only removed one of my own dark sides.
>
> >> From reading the the code, it is just for serializing calls of YUV() .
> >> So it could be moved to the private section, right ?
> >
> Sure, it could be moved. But if you want to do this to avoid deadlocks,
> that won't work, since YUV() is still called with areaMutex, or now
> oldPictureMutex held. So it is still possible to create deadlocks in the
> video-out classes which inherit from cVideo.
>
> > Dropped areaMutex, as there was the possibility of loosing a current
> > frame. This is now handled by locking with oldPictureMutex too.
> >
> Hmm, it would have been simpler just to keep areaMutex and to use
> areaMutex instead of osdMutex in cVideo::Action()... Locking an mutex,
> which is already held by the thread is not a good coding style (Action()
> calls DrawStill())...
Beside coding style, do you still see a deadlock possibility ?
> And you should update the comments on oldPictureMutex, if it takes over
> the responsibilities of areaMutex.
Will do that.
--
Stefan Lucke
_______________________________________________
Softdevice-devel mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/softdevice-devel
