[MPlayer-dev-eng] [PATCH] A/V sync improvement for TV streams

[Michael Niedermayer]

On Mon, Oct 20, 2008 at 07:23:12PM -0400, Laurent wrote:
> On 10/17/08, Uoti Urpala <uoti.urpala at pp1.inet.fi> wrote:
[...]

partial review below, if someone more knowledgeable in the affected code
wants to review this, then this surely is welcome!

[...]
> Index: libmpcodecs/ad_pcm.c
> ===================================================================
> --- libmpcodecs/ad_pcm.c	(revision 27776)
> +++ libmpcodecs/ad_pcm.c	(working copy)
> @@ -7,7 +7,7 @@
>  #include "libaf/af_format.h"
>  #include "libaf/reorder_ch.h"
>  
> -static ad_info_t info = 
> +static const ad_info_t info =
>  {
>  	"Uncompressed PCM audio decoder",
>  	"pcm",

this hunk is ok and can be commited i think


[...]
> @@ -121,13 +128,39 @@
>  static int decode_audio(sh_audio_t *sh_audio,unsigned char *buf,int minlen,int maxlen)
>  {
>    unsigned len = sh_audio->channels*sh_audio->samplesize;
> -  len = (minlen + len - 1) / len * len;
> -  if (len > maxlen)
> +  minlen = (minlen + len - 1) / len * len;
> +  if (minlen > maxlen)
>        // if someone needs hundreds of channels adjust audio_out_minsize
>        // based on channels in preinit()
>        return -1;
> -  len=demux_read_data(sh_audio->ds,buf,len);
> -  if (len > 0 && sh_audio->channels >= 5) {
> +
> +  maxlen = maxlen / len * len;

> +  len = 0;
> +  struct ad_pcm_context *ctx = sh_audio->context;

this breaks gcc 2.95 support, declaration and statements should not be mixed
there are a few more such cases that need to be fixed


[...]
> Index: mplayer.c
> ===================================================================
> --- mplayer.c	(revision 27776)
> +++ mplayer.c	(working copy)
> @@ -2085,12 +2085,18 @@
>  static int sleep_until_update(float *time_frame, float *aq_sleep_time)
>  {
>      int frame_time_remaining = 0;
> +    float delay = 0.0;
>      current_module="calc_sleep_time";
>  
>      *time_frame -= GetRelativeTime(); // reset timer
>  
> -    if (mpctx->sh_audio && !mpctx->d_audio->eof) {
> -	float delay = mpctx->audio_out->get_delay();
> +    if (mpctx->sh_audio) {
> +	// If there still are audio in the buffers,
> +	// we should continue to sync, especially as
> +	// the audio demuxer may be EOF temporarily (TV acquisition).
> +	delay = mpctx->audio_out->get_delay();
> +    }
> +    if (delay > 0.0) {
>  	mp_dbg(MSGT_AVSYNC, MSGL_DBG2, "delay=%f\n", delay);
>  
>  	if (autosync) {

I do not know the related code well enough but from my point of view this
looks odd. EOF supposedly means end of "file" and that really should not be
true unless it really is the end, i mean there cannot be 2 ends.
If this variable is really intended to mean some kind of "empty buffer please
wait" then it should be renamed to a more appropriate name in a seperate
patch i think.

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In a rich man's house there is no place to spit but his face.
-- Diogenes of Sinope
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20081029/ecffd367/attachment.pgp>