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.
Bye,
Martin
_______________________________________________
Softdevice-devel mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/softdevice-devel
