Darren Salt 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 ;-)

---

#! /bin/sh /usr/share/dpatch/dpatch-run ## 10_livelock.dpatch by Timothy Baldwin T.E.Baldwin99@members.leeds.ac.uk ## ## All lines beginning with `## DP:' are a description of the patch. ## DP: Fix a livelock problem when jumping between editing marks.

@DPATCH@ diff -urNad vdr-1.3.23/dvbplayer.c /tmp/dpep.odvcge/vdr-1.3.23/dvbplayer.c --- vdr-1.3.23/dvbplayer.c 2005-03-25 23:45:25.789555968 +0000 +++ /tmp/dpep.odvcge/vdr-1.3.23/dvbplayer.c 2005-03-25 23:53:11.273645683 +0000 @@ -376,6 +376,7 @@ cPoller Poller; if (DevicePoll(Poller, 100)) {

•       sched_yield();
        LOCK_THREAD;
  
        // Read the next frame from the file:
  

Under which conditions do these lockups happen?

Which output device is in use (FF DVB card or some software player)?

I don't really see what difference this sched_yield() would make, so I'd like to understand this before simply throwing it in...

Klaus

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:
  

Klaus Schmidinger wrote:

Here's another suggestion (line numbers may be a little off). The only difference to Reinhard's patch is that the sleep is done outside the locked area.

Why not just get rid of that sleep completely?

Index: vdr-1.3.24/dvbplayer.c =================================================================== --- vdr-1.3.24.orig/dvbplayer.c +++ vdr-1.3.24/dvbplayer.c @@ -191,6 +191,7 @@ private: bool running; bool firstPacket; ePlayModes playMode; + cCondVar playMode_condition; ePlayDirs playDir; int trickSpeed; int readIndex, writeIndex; @@ -355,6 +356,7 @@ void cDvbPlayer::Activate(bool On) } else if (active) { running = false; + playMode_condition.Broadcast(); Cancel(3); active = false; } @@ -372,14 +374,9 @@ void cDvbPlayer::Action(void)

nonBlockingFileReader = new cNonBlockingFileReader; int Length = 0; - bool Sleep = false;

running = true; while (running && (NextFile() || readIndex >= 0 || ringBuffer->Available() || !DeviceFlush(100))) { - if (Sleep) { - cCondWait::SleepMs(3); // this keeps the CPU load low - Sleep = false; - } cPoller Poller; if (DevicePoll(Poller, 100)) {

@@ -454,7 +451,7 @@ void cDvbPlayer::Action(void) } } else - Sleep = true; + playMode_condition.Wait(mutex);

// Get the next frame from the buffer:

@@ -528,6 +525,7 @@ void cDvbPlayer::Play(void) DevicePlay(); playMode = pmPlay; playDir = pdForward; + playMode_condition.Broadcast(); } }

Index: vdr-1.3.24/thread.h =================================================================== --- vdr-1.3.24.orig/thread.h +++ vdr-1.3.24/thread.h @@ -77,11 +77,11 @@ class cThread { private: bool running; pthread_t childTid; - cMutex mutex; char *description; static bool emergencyExitRequested; static void *StartThread(cThread *Thread); protected: + cMutex mutex; void Lock(void) { mutex.Lock(); } void Unlock(void) { mutex.Unlock(); } virtual void Action(void) = 0;

cu Ludwig