[MPlayer-dev-eng] Updated nailfps filter patches

[Rich Felker]

On Mon, Nov 12, 2007 at 02:28:03AM -0500, xiphmont at xiph.org wrote:
> +++ libmpcodecs/vf_vo.c	(working copy)
> @@ -77,6 +77,12 @@
>  static int control(struct vf_instance_s* vf, int request, void* data)
>  {
>      switch(request){
> +    case VFCTRL_RESET:
> +    {
> +        if(!video_out) return CONTROL_FALSE; // vo not configured?
> +	return(video_out->control(VOCTRL_RESET, data) 
> +	       == VO_TRUE) ? CONTROL_TRUE : CONTROL_FALSE;
> +    }

Broken indention. Fix.

> +++ libmpcodecs/vf.c	(working copy)
> @@ -99,6 +99,7 @@
>  extern const vf_info_t vf_info_blackframe;
>  extern const vf_info_t vf_info_geq;
>  extern const vf_info_t vf_info_ow;
> +extern vf_info_t vf_info_nail;

Missing const.

> Index: libmpcodecs/vf_nailfps.c

This whole file is mixing tabs and spaces with an odd setting for the
width of tabs. Please convert it to use only spaces based on whatever
your strange idea of tab-width is. I can't do it for you because I
don't know what your tab-width was intended to be...

> Index: libao2/ao_pcm.c
> ===================================================================
> --- libao2/ao_pcm.c	(revision 25005)
> +++ libao2/ao_pcm.c	(working copy)
> @@ -29,6 +29,8 @@
>  static char *ao_outputfilename = NULL;
>  static int ao_pcm_waveheader = 1;
>  static int fast = 0;
> +static int pipe = 0;
> +static int segments = 0;
>  
>  #define WAV_ID_RIFF 0x46464952 /* "RIFF" */
>  #define WAV_ID_WAVE 0x45564157 /* "WAVE" */
> @@ -55,6 +57,7 @@
>  
>  /* init with default values */
>  static struct WaveHeader wavhdr;
> +static uint32_t data_length;
>  
>  static FILE *fp = NULL;
>  
> @@ -71,6 +74,7 @@
>  	  {"waveheader", OPT_ARG_BOOL, &ao_pcm_waveheader, NULL},
>  	  {"file",       OPT_ARG_MSTRZ, &ao_outputfilename, NULL},
>  	  {"fast",       OPT_ARG_BOOL, &fast, NULL},
> +	  {"pipe",       OPT_ARG_BOOL, &pipe, NULL},
>  	  {NULL}
>  	};
>  	// set defaults
> @@ -127,14 +131,23 @@
>  	       (channels > 1) ? "Stereo" : "Mono", af_fmt2str_short(format));
>  	mp_msg(MSGT_AO, MSGL_INFO, MSGTR_AO_PCM_HintInfo);
>  
> +	if(segments){
> +	       if(!fp)fp = fopen(ao_outputfilename, "ab");
> +	       if(fp){
> +		 segments++;

If segments is used purely as a flag you should set it with segments=1
not segments++. In practice I don't think you'll have UINT_MAX files,
but the idiom is not correct.

Also, your code ignores the fact that different -ao options (with
different filenames) could be used for each file on the command line,
e.g.:

mplayer foo.mp3 -ao pcm:file=foo.wav bar.mp3 -ao pcm:file=bar.wav ...

As such, unless you can fix these fundamental issues which result in
feature regression, I think we need to reject the whole ao_pcm.c patch
for now...

> +		 return 1;
> +	       }
> +	}else{
>  	fp = fopen(ao_outputfilename, "wb");
>  	if(fp) {
>  		if(ao_pcm_waveheader){ /* Reserve space for wave header */
>  			fwrite(&wavhdr,sizeof(wavhdr),1,fp);
> -			wavhdr.file_length=wavhdr.data_length=0;
> +		      data_length=0;
>  		}
> +		    segments++;
>  		return 1;
>  	}
> +	}

All kinds of broken indention, masking the intent of the changes.

> +    if(!pipe){
>  	if(ao_pcm_waveheader && fseek(fp, 0, SEEK_SET) == 0){ /* Write wave header */

The if(!pipe) is redundant. fseek will fail on unseekable files. Even
if it were useful, it could be merged into the same if...

> -		wavhdr.file_length = wavhdr.data_length + sizeof(wavhdr) - 8;
> -		wavhdr.file_length = le2me_32(wavhdr.file_length);
> -		wavhdr.data_length = le2me_32(wavhdr.data_length);
> +	        uint32_t file_length = data_length + sizeof(wavhdr) - 8;
> +		wavhdr.file_length = le2me_32(file_length);
> +		wavhdr.data_length = le2me_32(data_length);

This is purely cosmetic. Remove.

>  		fwrite(&wavhdr,sizeof(wavhdr),1,fp);
>  	}
> +    }
> +    if(!pipe){
>  	fclose(fp);
> +	fp=NULL;
> +    }
> +  

And here the exact same conditional if(!pipe) is repeated twice in a
row for no reason at all. It could all be inside the same conditional.

> Index: mplayer.c
> ===================================================================
> --- mplayer.c	(revision 25005)
> +++ mplayer.c	(working copy)
> @@ -1625,7 +1625,8 @@
>  	// XXX Time used in this call is not counted in any performance
>  	// timer now, OSD is not updated correctly for filter-added frames
>  	if (vf_output_queued_frame(sh_video->vfilter))
> -	    break;
> +	return 1;
> +

Broken indention.

> @@ -1643,13 +1644,11 @@
>  	    update_teletext(sh_video, mpctx->demuxer, 0);
>  	    update_osd_msg();
>  	    current_module = "filter video";
> -	    if (filter_video(sh_video, decoded_frame, sh_video->pts))
> -		break;
> +	return(filter_video(sh_video, decoded_frame, sh_video->pts));
>  	}
>  	if (hit_eof)
> -	    return 0;
> +	return -1;
>      }
> -    return 1;

Ditto.

>  }
>  
>  #ifdef HAVE_RTC
> @@ -2047,8 +2046,9 @@
>  						    sh_video->pts));
>      }
>      else {
> -	if (!generate_video_frame(sh_video, mpctx->d_video))
> -	    return -1;
> +        int ret = generate_video_frame(sh_video, mpctx->d_video);
> +	if(ret<1)return ret;
> +

Ditto.

> @@ -2238,10 +2238,12 @@
>  	return -1;
>  
>      if (mpctx->sh_video) {
> +        vf_instance_t *vf = mpctx->sh_video->vfilter;

This one is okay because it only has spaces..

> +++ libaf/control.h	(working copy)
> @@ -234,4 +234,7 @@
>  #define AF_CONTROL_PLAYBACK_SPEED	0x00002500 | AF_CONTROL_FILTER_SPECIFIC
>  #define AF_CONTROL_SCALETEMPO_AMOUNT	0x00002600 | AF_CONTROL_FILTER_SPECIFIC
>  
> +// because you might just want to know.
> +#define AF_CONTROL_RESET                0x00002700 
> +

Uninformative comment...

> Index: libaf/af_nailsync.c

Also convert this to use only spaces...

Rich