[MPlayer-dev-eng] Updated nailfps filter patches

[xiphmont at xiph.org]

On Nov 12, 2007 3:33 AM, Rich Felker <dalias at aerifal.cx> wrote:

> 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 ...

In fact, I was not aware that was valid syntax.  My code ignores this
possibility.  I will fix it.

> 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...

The above is a fundamental issue.  I've not seen any others yet....

> > +    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...

...and if you want the pipe behavior on non pipe files?  I'm
implementing what my doc said I'm implementing.  Could we stick to
bugs that are actually bugs please.

> > -             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.

Look at it again and think 'bigendian' and 'reused'.

>
> >               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.

Yes.  Also not a bug.

> > +// because you might just want to know.
> > +#define AF_CONTROL_RESET                0x00002700
> > +
>
> Uninformative comment...

fair enough.

Monty