[Ffmpeg-devel] [PATCH] remove special nuv wav tags

[Roberto Togni]

On Sun, 05 Nov 2006 21:13:14 +0000
M?ns Rullg?rd <mru at inprovide.com> wrote:

> Reimar D?ffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de> writes:
> 
> > Hello,
> > attached patch does $subj and is tested. Feel free to either apply,
> > ignore or think of something better.
> >
> > Greetings,
> > Reimar D?ffinger
> >
> > Index: libavformat/nuv.c
> > ===================================================================
> > --- libavformat/nuv.c	(revision 6906)
> > +++ libavformat/nuv.c	(working copy)
> > @@ -90,12 +90,18 @@
> >                      url_fskip(pb, 4);
> >
> >                  if (ast) {
> > +                    int lookuptag;
> >                      ast->codec->codec_tag = get_le32(pb);
> >                      ast->codec->sample_rate = get_le32(pb);
> >                      ast->codec->bits_per_sample = get_le32(pb);
> >                      ast->codec->channels = get_le32(pb);
> > +                    lookuptag = ast->codec->codec_tag;
> > +                    switch (lookuptag) {
> > +                      case MKTAG('R', 'A', 'W', 'A'): lookuptag = 1; break;
> > +                      case MKTAG('L', 'A', 'M', 'E'): lookuptag = 0x55; break;
> > +                    }
> >                      ast->codec->codec_id =
> > -                        wav_codec_get_id(ast->codec->codec_tag,
> > +                        wav_codec_get_id(lookuptag,
> >                                           ast->codec->bits_per_sample);
> >                  } else
> >                      url_fskip(pb, 4 * 4);
> > Index: libavformat/riff.c
> > ===================================================================
> > --- libavformat/riff.c	(revision 6906)
> > +++ libavformat/riff.c	(working copy)
> > @@ -200,10 +200,6 @@
> >      { CODEC_ID_TTA, MKTAG('T', 'T', 'A', '1') },
> >      { CODEC_ID_WAVPACK, MKTAG('W', 'V', 'P', 'K') },
> >      { CODEC_ID_SHORTEN, MKTAG('s', 'h', 'r', 'n') },
> > -
> > -    // for NuppelVideo (nuv.c)
> > -    { CODEC_ID_PCM_S16LE, MKTAG('R', 'A', 'W', 'A') },
> > -    { CODEC_ID_MP3, MKTAG('L', 'A', 'M', 'E') },
> >      { 0, 0 },
> >  };
> 
> Are those two the only allowed codecs?  Then something like this seems
> much more appropriate:
> 
>     int tag = get_le32(pb);
>     switch(tag){
>     case MKTAG('R', 'A', 'W', 'A'):
>         ast->codec->codec_id = CODEC_ID_PCM_S16LE;
>         break;
>     case MKTAG('L', 'A', 'M', 'E'):
>         ast->codec->codec_id = CODEC_ID_MP3;
>         break;
>     }
> 
> See, no need to involve RIFF tags at all.  RIFF tags are *not* the
> bloody center of the universe, even if mplayer appears to have been
> written with that assumption.
> 
> Also, setting AVCodecContext.codec_tag in nuv.c seems a little
> questionable, given that most of the code treats it as RIFF codec tag,
> which it is not in this case.
> 

Having to modify a demuxer every time we need to support a new codec
does not seem a better solution to me. With the current code, you just
need to add a pair {codec_id, "tag"} to the tag list.

Since the main argument seems to be "riff is for avi", what about do a
svn move riff.c tags.c ?

Ciao,
 Roberto