FF card AV sync problems, possible fix to VDR

  Hi!

The newest firmware for FF cards did not completely fix the AV desync problems 
for me. According to information from Werner the problem happens when small 
video frames fill the decoder buffer with over 2 seconds of data. So I made 
this patch for dvbplayer.c to stop it from uploading more PES frames to decoder 
when STC/PTS difference is more than 2 seconds. This seems to fix the remaining 
problems for me, but I have not tested it much. The PTS/STC-code has been 
mostly taken from the dvb-subtitles plugin. Comments, please

Ville
  

Ville Rannikko wrote:
> Hi!
> 
> The newest firmware for FF cards did not completely fix the AV desync
> problems for me. According to information from Werner the problem
> happens when small video frames fill the decoder buffer with over 2
> seconds of data. So I made this patch for dvbplayer.c to stop it from
> uploading more PES frames to decoder when STC/PTS difference is more
> than 2 seconds. This seems to fix the remaining problems for me, but I
> have not tested it much. The PTS/STC-code has been mostly taken from the
> dvb-subtitles plugin. Comments, please

While this may actually help in your case, I'm not particularly fond
of this. The cDvbPlayer shouldn't have to worry about this. It takes care
that data is sent to the player device fast enough to avoid underruns,
but that's all it should have to care about. It's the device's (driver
and firmware) job to play the data correctly.

Klaus
  

On 1/31/07, Klaus Schmidinger <Klaus.Schmidinger@cadsoft.de> wrote:
>
> Ville Rannikko wrote:
> > Hi!
> >
> > The newest firmware for FF cards did not completely fix the AV desync
> > problems for me. According to information from Werner the problem
> > happens when small video frames fill the decoder buffer with over 2
> > seconds of data. So I made this patch for dvbplayer.c to stop it from
> > uploading more PES frames to decoder when STC/PTS difference is more
> > than 2 seconds. This seems to fix the remaining problems for me, but I
> > have not tested it much. The PTS/STC-code has been mostly taken from the
> > dvb-subtitles plugin. Comments, please
>
> While this may actually help in your case, I'm not particularly fond
> of this. The cDvbPlayer shouldn't have to worry about this. It takes care
> that data is sent to the player device fast enough to avoid underruns,
> but that's all it should have to care about. It's the device's (driver
> and firmware) job to play the data correctly.
>

>From what he's saying, the problem is buffer overrun's, not underrun's.  Too
much data is being sent and the device isn't able to keep up.  If that's the
case then it would make sense for vdr to have a user setting to limit how
many seconds (or milliseconds perhaps?) worth of data is sent to the
buffer.  I can't think of any reason not to add such a feature if it means
better usability for the end-user.

That's my opinion anyways.
  

VDR User wrote:
> On 1/31/07, *Klaus Schmidinger* <Klaus.Schmidinger@cadsoft.de
> <mailto:Klaus.Schmidinger@cadsoft.de>> wrote:
> 
>     Ville Rannikko wrote:
>     > Hi!
>     >
>     > The newest firmware for FF cards did not completely fix the AV desync
>     > problems for me. According to information from Werner the problem
>     > happens when small video frames fill the decoder buffer with over 2
>     > seconds of data. So I made this patch for dvbplayer.c to stop it from
>     > uploading more PES frames to decoder when STC/PTS difference is more
>     > than 2 seconds. This seems to fix the remaining problems for me,
>     but I
>     > have not tested it much. The PTS/STC-code has been mostly taken
>     from the
>     > dvb-subtitles plugin. Comments, please
> 
>     While this may actually help in your case, I'm not particularly fond
>     of this. The cDvbPlayer shouldn't have to worry about this. It takes
>     care
>     that data is sent to the player device fast enough to avoid underruns,
>     but that's all it should have to care about. It's the device's (driver
>     and firmware) job to play the data correctly.
> 
> 
> From what he's saying, the problem is buffer overrun's, not underrun's. 
> Too much data is being sent and the device isn't able to keep up.  If
> that's the case then it would make sense for vdr to have a user setting
> to limit how many seconds (or milliseconds perhaps?) worth of data is
> sent to the buffer.  I can't think of any reason not to add such a
> feature if it means better usability for the end-user.

If the device can't take any more data, it should just refuse to
accept it and return from the write() call without anything written.

Klaus
  

On 1/31/07, Klaus Schmidinger <Klaus.Schmidinger@cadsoft.de> wrote:
>
> VDR User wrote:
> > From what he's saying, the problem is buffer overrun's, not underrun's.
> > Too much data is being sent and the device isn't able to keep up.  If
> > that's the case then it would make sense for vdr to have a user setting
> > to limit how many seconds (or milliseconds perhaps?) worth of data is
> > sent to the buffer.  I can't think of any reason not to add such a
> > feature if it means better usability for the end-user.
>
> If the device can't take any more data, it should just refuse to
> accept it and return from the write() call without anything written.
>

I disagree.  Simply returning from write() implies the data was written and
the device is ready for more.  If this is not true then you risk making the
sync even worse.  I consulted with people familiar with this type of stuff
and it was suggested the problem could (and should) be fixed at the driver
level so I'm following up on that now.  Hopefully we'll find a good (or at
least resonable) solution to this last little bit of a/v sync frustration.
  

VDR User wrote:
> On 1/31/07, *Klaus Schmidinger* <Klaus.Schmidinger@cadsoft.de
> <mailto:Klaus.Schmidinger@cadsoft.de>> wrote:
> 
>     VDR User wrote:
>     > From what he's saying, the problem is buffer overrun's, not
>     underrun's.
>     > Too much data is being sent and the device isn't able to keep up.  If
>     > that's the case then it would make sense for vdr to have a user
>     setting
>     > to limit how many seconds (or milliseconds perhaps?) worth of data is
>     > sent to the buffer.  I can't think of any reason not to add such a
>     > feature if it means better usability for the end-user.
> 
>     If the device can't take any more data, it should just refuse to
>     accept it and return from the write() call without anything written.
> 
> 
> I disagree.  Simply returning from write() implies the data was written
> and the device is ready for more.

The write() function returns the number of bytes actually written,
which is not necessarily the same as the number of bytes the caller
wanted to write. So the device can chose not to accept all of the data.
If the device is currently unable to accept any data (and the file handle
is in O_NONBLOCK mode) it shall return EAGAIN to inform the caller that
it is currently busy and that the caller should try again later.

>  If this is not true then you risk
> making the sync even worse.  I consulted with people familiar with this
> type of stuff and it was suggested the problem could (and should) be
> fixed at the driver level so I'm following up on that now.

Well, that's exactly what I was suggesting. The write() function has
everything it takes to block data from coming into the device if its
buffer is full. No need for the caller to do any funny estimates here.

Klaus
  

On Wed, Jan 31, 2007 at 08:38:29PM +0100, Klaus Schmidinger wrote:
> > From what he's saying, the problem is buffer overrun's, not underrun's. 
> > Too much data is being sent and the device isn't able to keep up.  If
> > that's the case then it would make sense for vdr to have a user setting
> > to limit how many seconds (or milliseconds perhaps?) worth of data is
> > sent to the buffer.  I can't think of any reason not to add such a
> > feature if it means better usability for the end-user.
> 
> If the device can't take any more data, it should just refuse to
> accept it and return from the write() call without anything written.

I gather that in plugin-based playback, cDevice::PlayAudio() and
cDevice::PlayVideo() are overridden and may return 0 to indicate
that nothing was written.

I modified PlayAudio and PlayVideo in softdevice.c so that they return 0
when softdevice is in suspended state.  That is, I replaced
  if (! packetMode)
with
  if (!packetMode && !setupStore.shouldSuspend)
in both methods, so that they would return 0.

This seemed to work for recordings, but I am not sure if it worked for video.
If I suspended the playback for only a short while, it looked like the
playback resumed from where it was stopped, and maybe some frames were
dropped every now and then to reduce the lag.  I'd prefer the live
playback to resume without any lag, that is, to drop all live PES packets
during the time the playback was suspended in the plugin.  However,
the PES packets from recordings should be blocked.

I believe that the easy fix would be to have these methods return 0 only
when playing a recording.  Can the plugin detect this somehow?

	Marko
  

Ville Rannikko wrote:
> Hi!
> 
> The newest firmware for FF cards did not completely fix the AV desync problems 
> for me.

Can you provide a sample recording where A/V sync fails with the current
firmware?

> According to information from Werner the problem happens when small  
> video frames fill the decoder buffer with over 2 seconds of data. So I made  
> this patch for dvbplayer.c to stop it from uploading more PES frames to decoder 
> when STC/PTS difference is more than 2 seconds. This seems to fix the remaining 
> problems for me, but I have not tested it much. The PTS/STC-code has been 
> mostly taken from the dvb-subtitles plugin. Comments, please

The driver accepts as much data as the driver buffers can hold:
- max. 192 KByte video
- max.  64 KByte audio

The firmware requests data from the driver. The driver does not push
data to the firmware without request from the firmware.

Until now I do not understand how there could be any over/underflows
during normal operation.

> --- dvbplayer.c.orig	2007-01-31 18:21:42.000000000 +0200
> +++ dvbplayer.c	2007-01-31 18:35:36.000000000 +0200
> @@ -495,6 +495,51 @@
>                        }
>                     }
>                 if (p) {
> +                 if(playMode == pmPlay && pc > 13)
> +                   {
> +                     int64_t stc = -1;
> +                     int64_t pts = -1;
> +
> +                     if ( p[0] == 0x00 && p[1] == 0x00 && p[2] == 0x01)
> +                       {
> +                         if(p[7] & 0x80)
> +                           {
> +                             switch( p[3] )
> +                               {
> +                               case 0xE0 ... 0xEF: // video
> +                               case 0xC0 ... 0xDF: // audio
> +                                 pts  = (int64_t) (p[ 9] & 0x0E) << 29 ;
> +                                 pts |= (int64_t)  p[ 10]         << 22 ;
> +                                 pts |= (int64_t) (p[ 11] & 0xFE) << 14 ;
> +                                 pts |= (int64_t)  p[ 12]         <<  7 ;
> +                                 pts |= (int64_t) (p[ 13] & 0xFE) >>  1 ;
> +                               }
> +                           }
> +                       }
> +                     if(pts != -1)
> +                       {
> +                         cDevice *pd = cDevice::PrimaryDevice();
> +                         if(pd)
> +                           {
> +                             stc = pd->GetSTC();
> +                             if(stc != 0)
> +                               {
> +                                 if(pts & (int64_t)1<<32)
> +                                   {
> +                                     stc |= (int64_t)1<<32;
> +                                   }
> +                                 int64_t timeDiff = (pts-stc);
> +                                 if (pts<stc)
> +                                   {
> +                                     timeDiff += (int64_t)1<<33;
> +                                   }
> +                                 timeDiff /= 90;
> +                                 if(timeDiff > 2000)
> +				   cCondWait::SleepMs(timeDiff - 2000);
> +                               }
> +			   }
> +		       }
> +		   }
>                     int w = PlayPes(p, pc, playMode != pmPlay);
>                     if (w > 0) {
>                        p += w;

Hm, that patch does not look like a real bug fix to me. ;-)
It is a workaround which limits the total buffer capacity to max 2s.
I guess this works because it somehow triggers a resync in the firmware.

Anyway, fixes must be applied to the firmware or driver, not to VDR.

But let's analyse the problem first. We need a sample recording...

Oliver
  

Klaus Schmidinger wrote:
> VDR User wrote:
> > On 1/31/07, *Klaus Schmidinger* <Klaus.Schmidinger@cadsoft.de
> > <mailto:Klaus.Schmidinger@cadsoft.de>> wrote:
> > 
> >     VDR User wrote:
> >     > From what he's saying, the problem is buffer overrun's, not
> >     underrun's.

Afaics this is just a guess. We need a sample recording and track down
what happens in the driver/firmware.

> >     > Too much data is being sent and the device isn't able to keep up.  If
> >     > that's the case then it would make sense for vdr to have a user
> >     setting
> >     > to limit how many seconds (or milliseconds perhaps?) worth of data is
> >     > sent to the buffer.  I can't think of any reason not to add such a
> >     > feature if it means better usability for the end-user.
> > 
> >     If the device can't take any more data, it should just refuse to
> >     accept it and return from the write() call without anything written.

Code from the driver:
|        ...
|        if (nonblock && !FREE_COND)
|                return -EWOULDBLOCK;
|
|        while (todo > 0) {
|                if (!FREE_COND) {
|                        if (nonblock)
|                                return count - todo;
|                        if (wait_event_interruptible(av7110->avout.queue,
|                                                     FREE_COND))
|                                return count - todo;
|                }
|         ...

Looks ok to me. The driver does not accept more data than it can handle.

> > I disagree.  Simply returning from write() implies the data was written
> > and the device is ready for more.
> 
> The write() function returns the number of bytes actually written,
> which is not necessarily the same as the number of bytes the caller
> wanted to write. So the device can chose not to accept all of the data.
> If the device is currently unable to accept any data (and the file handle
> is in O_NONBLOCK mode) it shall return EAGAIN to inform the caller that
> it is currently busy and that the caller should try again later.
> 
> >  If this is not true then you risk
> > making the sync even worse.  I consulted with people familiar with this
> > type of stuff and it was suggested the problem could (and should) be
> > fixed at the driver level so I'm following up on that now.
> 
> Well, that's exactly what I was suggesting. The write() function has
> everything it takes to block data from coming into the device if its
> buffer is full. No need for the caller to do any funny estimates here.

Fullack. There is no reason to add workarounds like this to VDR.

@all:
Don't panic. Provide samples. :-)

Oliver
  

Hi,

Marko Mäkelä wrote:

> I believe that the easy fix would be to have these methods return 0 only
> when playing a recording.  Can the plugin detect this somehow?

For live TV, VDR runs a transfer thread and the replaying device may use
cDevice::Transferring() to detect this (works at least for vdr-xine in
cXineDevice::SetPlayMode() for PlayMode being != pmNone).

Bye.
  

On Thu, Feb 01, 2007 at 07:57:20PM +0100, Reinhard Nissl wrote:
> Hi,
> 
> Marko Mäkelä wrote:
> 
> > I believe that the easy fix would be to have these methods return 0 only
> > when playing a recording.  Can the plugin detect this somehow?
> 
> For live TV, VDR runs a transfer thread and the replaying device may use
> cDevice::Transferring() to detect this (works at least for vdr-xine in
> cXineDevice::SetPlayMode() for PlayMode being != pmNone).

Thanks, I knew there had to be some way.  Softdevice used to blank
the screen after a few seconds of missing video stream (tuned to an
audio channel).  A fix was introduced so that it wouldn't blank the
screen when a recording was paused.

	Marko