On Montag 25 September 2006 19:37, Martin Wache wrote:
> Stefan Lucke schrieb:
> > 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 ?
> >
> On non-linux systems, yes. As soon as you are using strict posix mutexes
> trying to lock a mutex from the thread which already holds the mutex
> always results in a deadlock.
>
> But there may be other problems as well: if you lock a mutex, and then
> call a method which locks the same mutex again, this second lock will
> fail with -EAGAIN (in case of a mutex PTHREAD_MUTEX_ERRORCHECK_NP), but
> not block. If this method now unlocks the mutex, the mutex will be
> unlocked, even though the calling method still thinks it holds the mutex.
> I did not try this out, maybe I'm wrong here (it depends on the
> implementation of the library... there are also recursive locks which
> are allowed to double lock)
> So I would like to avoid double locking...
>
> Thinking again about all this, I don't think that one actually needs to
> call DrawStill() from other than cVideo, it is only used to redraw a
> frame when the OSD changes and no video currently is displayed.
> I think the name is misleading, even still pictures should be displayed
> with DrawVideo(), since on OSD change one still wants to have the last
> still picture, and only DrawVideo() updates oldPicture. So I would
> suggest to remove the lock from DrawStill(), make it private and put a
> comment on it, that it only should be called with oldPictureMutex held.
DrawStill() is called from outside of video.c -> ShmClient.c.
So we should have a non-locking private one, and a locking public.
--
Stefan Lucke
_______________________________________________
Softdevice-devel mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/softdevice-devel
