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

[Michael Niedermayer]

On Mon, Oct 06, 2008 at 02:19:57AM +0200, Aurelien Jacobs wrote:
> 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...

I do like to keep double call detection ...


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

why dont you use a static array?


> 
> > and i think ive seen a similar patch already from someone else ...
> 
> I don't remember about this.

somewhere in the thread with subject
"[FFmpeg-devel] [PATCH] Codec lookup: do not use codec_id"
i think


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

iam ok with this hunk


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

ok, then lets postpone this hunk ...

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

If a bugfix only changes things apparently unrelated to the bug with no
further explanation, that is a good sign that the bugfix is wrong.
-------------- 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/ffmpeg-devel/attachments/20081006/8884b44b/attachment.pgp>