Hang when moving between editing marks

  Hi,

Klaus Schmidinger wrote:

>>>> The attached patch should fix a hang (livelock) which can occur when
>>>> moving between editing marks. Blame Timothy Baldwin if it doesn't 
>>>> work -
>>>> he suggested it ;-)
>>
>>> Under which conditions do these lockups happen?
>>> Which output device is in use (FF DVB card or some software player)?
>>
>> It's happened here: budget card, xine-lib.
> 
> Ah, as I suspected ;-)
> 
> Could it be that the xine player doesn't implement
> cDevice::Poll() the way it is supposed to?
> My guess would be that in case of a still picture the xine player's
> Poll() function returns immediately instead of waiting for the
> given timeout. That would explain why everything appears to lock up.

We've already discussed vdr-xine's Poll() implementation in this thread 
(~ 26.11.2004):

vdr-1.3.15: high CPU load when showing still picture while editing cut marks

At that time, you've decided to put Sleep() in the loop back again. But 
you've put it at a different location than it was in the previous versions.

This can still lead to high cpu load when someone pauses a recording 
almost at the end. At that time, VDR has already read and sent the last 
packet to vdr-xine and is now just waiting for vdr-xine to report (via 
DeviceFlush()) that xine has played the last frame of the given data.

The attached patch fixes this issue by exchanging two "if" blocks, which 
also moves the Sleep() almost to the position where is was located in 
earlier VDR versions.

This patch is also part of my other dvbplayer-2/3 patch, but I've 
extracted it to get it included earlier than the other things of the 
original patches.

But maybe it would be even better to move this Sleep() out of the locked 
area.

> Could you please verify this?

Well, as written in the above mentioned thread, after sending the still 
image data to xine, the fifo gets empty and therefore Poll() returns 
immediately, as data can be transfered to xine.

But I do not see any special behaviour for FF cards in this code:

   bool cDvbDevice::Poll(cPoller &Poller, int TimeoutMs)
   {
     Poller.Add((playMode == pmAudioOnly
       || playMode == pmAudioOnlyBlack) ? fd_audio : fd_video, true);
     return Poller.Poll(TimeoutMs);
   }

Is it that FF cards behave like that when a still image is shown?
But as a Clear() is happening before sending the still image, I'd expect 
even a FF card to immediately return here in Poll().

>>> I don't really see what difference this sched_yield() would make, so I'd
>>> like to understand this before simply throwing it in...
>>
>> Quoting from the mail which described the patch:
>>
>> | It's livelock!
>>
>> | The thread which executes cDvbPlayer::Action(void) (in dvbplayer.c) 
>> locks
>> | the cDvbPlayer object most of the time when an editing mark is first 
>> jumped
>> | to. The symptoms are cured by adding a call to sched_yield() before
>> | LOCK_THREAD in cDvbPlayer::Action(void).

Well, I do not have this sched_yield() in my code version, but maybe I 
do not see any effects of missig it as my development PC has an 
HyperThreading processor. When the unlock happens, the other waiting 
thread might immediately be able to enter the critical section.

Bye.
  

Reinhard Nissl wrote:
> Hi,
> 
> Klaus Schmidinger wrote:
> 
>>>>> The attached patch should fix a hang (livelock) which can occur when
>>>>> moving between editing marks. Blame Timothy Baldwin if it doesn't
>>>>> work -
>>>>> he suggested it ;-)
>>>
>>>
>>>> Under which conditions do these lockups happen?
>>>> Which output device is in use (FF DVB card or some software player)?
>>>
>>>
>>> It's happened here: budget card, xine-lib.
>>
>>
>> Ah, as I suspected ;-)
>>
>> Could it be that the xine player doesn't implement
>> cDevice::Poll() the way it is supposed to?
>> My guess would be that in case of a still picture the xine player's
>> Poll() function returns immediately instead of waiting for the
>> given timeout. That would explain why everything appears to lock up.
> 
> 
> We've already discussed vdr-xine's Poll() implementation in this thread
> (~ 26.11.2004):
> 
> vdr-1.3.15: high CPU load when showing still picture while editing cut
> marks
> 
> At that time, you've decided to put Sleep() in the loop back again. But
> you've put it at a different location than it was in the previous versions.
> 
> This can still lead to high cpu load when someone pauses a recording
> almost at the end. At that time, VDR has already read and sent the last
> packet to vdr-xine and is now just waiting for vdr-xine to report (via
> DeviceFlush()) that xine has played the last frame of the given data.
> 
> The attached patch fixes this issue by exchanging two "if" blocks, which
> also moves the Sleep() almost to the position where is was located in
> earlier VDR versions.
> 
> This patch is also part of my other dvbplayer-2/3 patch, but I've
> extracted it to get it included earlier than the other things of the
> original patches.
> 
> But maybe it would be even better to move this Sleep() out of the locked
> area.
> 
>> Could you please verify this?
> 
> 
> Well, as written in the above mentioned thread, after sending the still
> image data to xine, the fifo gets empty and therefore Poll() returns
> immediately, as data can be transfered to xine.
> 
> But I do not see any special behaviour for FF cards in this code:
> 
>   bool cDvbDevice::Poll(cPoller &Poller, int TimeoutMs)
>   {
>     Poller.Add((playMode == pmAudioOnly
>       || playMode == pmAudioOnlyBlack) ? fd_audio : fd_video, true);
>     return Poller.Poll(TimeoutMs);
>   }
> 
> Is it that FF cards behave like that when a still image is shown?
> But as a Clear() is happening before sending the still image, I'd expect
> even a FF card to immediately return here in Poll().
> 
>>>> I don't really see what difference this sched_yield() would make, so
>>>> I'd
>>>> like to understand this before simply throwing it in...
>>>
>>>
>>> Quoting from the mail which described the patch:
>>>
>>> | It's livelock!
>>>
>>> | The thread which executes cDvbPlayer::Action(void) (in dvbplayer.c)
>>> locks
>>> | the cDvbPlayer object most of the time when an editing mark is
>>> first jumped
>>> | to. The symptoms are cured by adding a call to sched_yield() before
>>> | LOCK_THREAD in cDvbPlayer::Action(void).
> 
> 
> Well, I do not have this sched_yield() in my code version, but maybe I
> do not see any effects of missig it as my development PC has an
> HyperThreading processor. When the unlock happens, the other waiting
> thread might immediately be able to enter the critical section.
> 

After comparing rev.3 and rev.4 of this patch, I noticed that you removed all
the fixIFrame functions.  Fixing the IFrames is no longer an issue?

Thanks for all your great patches..

Best Regards,
C.Y.M.
  

Reinhard Nissl wrote:
> Hi,
> 
> Klaus Schmidinger wrote:
> 
>>>>> The attached patch should fix a hang (livelock) which can occur when
>>>>> moving between editing marks. Blame Timothy Baldwin if it doesn't 
>>>>> work -
>>>>> he suggested it ;-)
>>>
>>>
>>>> Under which conditions do these lockups happen?
>>>> Which output device is in use (FF DVB card or some software player)?
>>>
>>>
>>> It's happened here: budget card, xine-lib.
>>
>>
>> Ah, as I suspected ;-)
>>
>> Could it be that the xine player doesn't implement
>> cDevice::Poll() the way it is supposed to?
>> My guess would be that in case of a still picture the xine player's
>> Poll() function returns immediately instead of waiting for the
>> given timeout. That would explain why everything appears to lock up.
> 
> 
> We've already discussed vdr-xine's Poll() implementation in this thread 
> (~ 26.11.2004):
> 
> vdr-1.3.15: high CPU load when showing still picture while editing cut 
> marks
> 
> At that time, you've decided to put Sleep() in the loop back again. But 
> you've put it at a different location than it was in the previous versions.
> 
> This can still lead to high cpu load when someone pauses a recording 
> almost at the end. At that time, VDR has already read and sent the last 
> packet to vdr-xine and is now just waiting for vdr-xine to report (via 
> DeviceFlush()) that xine has played the last frame of the given data.
> 
> The attached patch fixes this issue by exchanging two "if" blocks, which 
> also moves the Sleep() almost to the position where is was located in 
> earlier VDR versions.
> 
> This patch is also part of my other dvbplayer-2/3 patch, but I've 
> extracted it to get it included earlier than the other things of the 
> original patches.
> 
> But maybe it would be even better to move this Sleep() out of the locked 
> area.
> 
>> Could you please verify this?
> 
> 
> Well, as written in the above mentioned thread, after sending the still 
> image data to xine, the fifo gets empty and therefore Poll() returns 
> immediately, as data can be transfered to xine.
> 
> But I do not see any special behaviour for FF cards in this code:
> 
>   bool cDvbDevice::Poll(cPoller &Poller, int TimeoutMs)
>   {
>     Poller.Add((playMode == pmAudioOnly
>       || playMode == pmAudioOnlyBlack) ? fd_audio : fd_video, true);
>     return Poller.Poll(TimeoutMs);
>   }
> 
> Is it that FF cards behave like that when a still image is shown?
> But as a Clear() is happening before sending the still image, I'd expect 
> even a FF card to immediately return here in Poll().

Well, actually I thought that the FF cards would wait in the Poll()
function if they are in still of pause mode, but apparently they don't
(I checked).

>>>> I don't really see what difference this sched_yield() would make, so 
>>>> I'd
>>>> like to understand this before simply throwing it in...
>>>
>>>
>>> Quoting from the mail which described the patch:
>>>
>>> | It's livelock!
>>>
>>> | The thread which executes cDvbPlayer::Action(void) (in dvbplayer.c) 
>>> locks
>>> | the cDvbPlayer object most of the time when an editing mark is 
>>> first jumped
>>> | to. The symptoms are cured by adding a call to sched_yield() before
>>> | LOCK_THREAD in cDvbPlayer::Action(void).
> 
> 
> Well, I do not have this sched_yield() in my code version, but maybe I 
> do not see any effects of missig it as my development PC has an 
> HyperThreading processor. When the unlock happens, the other waiting 
> thread might immediately be able to enter the critical section.

It works here on my system without the sched_yield(), too. But maybe that's
because I'm still using kernel 2.4.20 and the old DVB driver.

Anyway, I don't see anything wrong with your patch and as far as I have seen
everything still runs ok on my system. So if others (especially those who
wrote that the sched_yield() line helped) could please test this patch
(note: _without_ the sched_yield()!) and let me know whether this also
works for them, I could release VDR 1.3.24 shortly...

Klaus

> ------------------------------------------------------------------------
> 
> --- ../vdr-1.3.23-orig/dvbplayer.c	2005-01-14 15:00:56.000000000 +0100
> +++ dvbplayer.c	2005-04-03 10:51:15.000000000 +0200
> @@ -380,8 +468,8 @@ void cDvbPlayer::Action(void)
>  
>             // Read the next frame from the file:
>  
> -           if (!readFrame && (replayFile >= 0 || readIndex >= 0)) {
> -              if (playMode != pmStill) {
> +           if (playMode != pmStill && playMode != pmPause) {
> +              if (!readFrame && (replayFile >= 0 || readIndex >= 0)) {
>                   if (!nonBlockingFileReader->Reading()) {
>                      if (playMode == pmFast || (playMode == pmSlow && playDir == pdBackward)) {
>                         uchar FileNumber;
> @@ -438,16 +535,16 @@ void cDvbPlayer::Action(void)
>                      break;
>                      }
>                   }
> -              else
> -                 cCondWait::SleepMs(3); // this keeps the CPU load low
> -              }
>  
> -           // Store the frame in the buffer:
> +              // Store the frame in the buffer:
>  
> -           if (readFrame) {
> -              if (ringBuffer->Put(readFrame))
> -                 readFrame = NULL;
> +              if (readFrame) {
> +                 if (ringBuffer->Put(readFrame))
> +                    readFrame = NULL;
> +                 }
>                }
> +           else
> +              cCondWait::SleepMs(3); // this keeps the CPU load low
>  
>             // Get the next frame from the buffer:
  

I applied the patch, but it doesn't work as good as the sched_yield "patch".

Now it takes between 3 and 8 seconds when moving or jumping between cutmarks, 
with the sched_yield this was possible instantly.

S.
  

Sebastian Frei wrote:
> I applied the patch, but it doesn't work as good as the sched_yield "patch".
> 
> Now it takes between 3 and 8 seconds when moving or jumping between cutmarks, 
> with the sched_yield this was possible instantly.

What happens if you put a

   cCondWait::SleepMs(1);

instead of the sched_yield()?
Maybe Reinhards remark about putting the sleep() outside of the locked area
is the key...

Klaus
  

Am Sonntag 08 Mai 2005 16:37 schrieb Klaus Schmidinger:
> Sebastian Frei wrote:
> > I applied the patch, but it doesn't work as good as the sched_yield
> > "patch".
> >
> > Now it takes between 3 and 8 seconds when moving or jumping between
> > cutmarks, with the sched_yield this was possible instantly.
>
> What happens if you put a
>
>    cCondWait::SleepMs(1);
>
> instead of the sched_yield()?
> Maybe Reinhards remark about putting the sleep() outside of the locked area
> is the key...
>
> Klaus
OK, I undid the previous patch and replaced the sched_yield... with 
cCondWait....

It works perfectly again.

Now I'll try your 2. suggestion.

S.