[FFmpeg-devel] [PATCH] ffmpeg should look up AVCodec by name

[Aurelien Jacobs]

Michael Niedermayer wrote:

> On Mon, Oct 06, 2008 at 01:22:09AM +0200, Aurelien Jacobs wrote:
> > Hi,
> > 
> > Curently, when specifying a codec with -[asv]codec to ffmpeg, it is
> > first converted to a CODEC_ID_* and then the coresponding AVCodec
> > is picked up. Problem arise when we have several AVCodec handling the
> > same CODEC_ID_*. eg: -acodec libvorbis select the internal vorbis
> > encoder instead of the libvorbis one...
> > Attached patch ensure we pick up AVCodec using the codec name, when
> > available. This allows to actually select the libvorbis encoder for
> > example. This works for both input and output codecs. This also
> > correctly display the selected codec.
> > 
> > Aurel
> 
> > Index: ffmpeg.c
> > ===================================================================
> > --- ffmpeg.c	(revision 15552)
> > +++ ffmpeg.c	(working copy)
> > @@ -1915,7 +1915,9 @@ static int av_encode(AVFormatContext **output_file
> >      for(i=0;i<nb_ostreams;i++) {
> >          ost = ost_table[i];
> >          if (ost->encoding_needed) {
> > -            AVCodec *codec;
> > +            AVCodec *codec = ost->st->codec->codec;
> 
> > +            ost->st->codec->codec = NULL;
> 
> why?

Because avcodec_open() fails if it's not NULL.
I guess the reason is to prevent calling avcodec_open() twice for the
same AVCodecContext.
Maybe avcodec_open() could be modified to not check this...

> besides the solution looks hackish

In a way, it is, because it misuse ost->st->codec->codec to convey
the AVcodec selection inside ffmpeg.c.
But I haven't found a better way to convey this simply. At the time
the audio_codec_name is parsed, the corresponding AVInputStream is
still not allocated/initialized, so it can't be used. And when
AVInputStream is allocated, audio_codec_name is not available
anymore. But still this information must be conveyed on a per stream
basis.

> and i think ive seen a similar patch already from someone else ...

I don't remember about this.

> [...]
> 
> > Index: libavcodec/utils.c
> > ===================================================================
> > --- libavcodec/utils.c	(revision 15552)
> > +++ libavcodec/utils.c	(working copy)
> > @@ -1033,6 +1033,8 @@ AVCodec *avcodec_find_decoder(enum CodecID id)
> >  AVCodec *avcodec_find_decoder_by_name(const char *name)
> >  {
> >      AVCodec *p;
> > +    if (!name)
> > +        return NULL;
> >      p = first_avcodec;
> >      while (p) {
> >          if (p->decode != NULL && strcmp(name,p->name) == 0)
> 
> where and why is null passed?

In opt_input_file(), I call this function without checking if name is
NULL. I could do the check in opt_input_file() (3 times), but it felt
cleaner this way.

> and if approved this should be a seperate commit

OK.

> > @@ -1051,7 +1053,9 @@ void avcodec_string(char *buf, int buf_size, AVCod
> >      int bitrate;
> >      AVRational display_aspect_ratio;
> >  
> > -    if (encode)
> > +    if (enc->codec)
> > +        p = enc->codec;
> > +    else if (encode)
> >          p = avcodec_find_encoder(enc->codec_id);
> >      else
> >          p = avcodec_find_decoder(enc->codec_id);
> 
> iam not sure if this always prints what is expected, i mean
> the input should be printed as vorbis or vorbis decoded by libvorbis
> not just libvorbis ...

I personally would expect the actual encoder/decoder name printed here
instead of the codec name, but maybe I'm wrong. Anyway, this is minor
issue. I can remove this from the patch. This can be fixed latter.

Aurel