[FFmpeg-devel] [PATCH] Codec lookup: do not use codec_id

[Benoit Fouet]

Hi Nicolas,

Nicolas George wrote:
> Hi.
>
> L'octidi 18 thermidor, an CCXV, Michael Niedermayer a ?crit :
>   
>> i think setting video_stream_copy and video_codec_name here is clearer than
>> a call to opt_video_codec() also its a more minimalistic change relative to
>> the current code
>>     
>
> I agree. I went through the opt_*_codec functions because of the av_free,
> but the av_free is no longer there.
>
> I also complied with the other stylistic remarks, although I feel this
> coding style quite unnatural.
>
> Here the updated version of the patch. Again, it passes the same series of
> tests as the previous versions.
>
>   

find some remarks below

> Suggested log message:
> Use the codec name from the command line options instead of the codec id.
>
> PS: A small question for later patches: would an attached file be preferred?
>
>   

yes, definitely


>Index: ffmpeg.c
>===================================================================
>--- ffmpeg.c    (revision 9947)
>+++ ffmpeg.c    (working copy)
>-static void opt_codec(int *pstream_copy, int *pcodec_id,
>+static void opt_codec(int *pstream_copy, char **pcodec_name,
>                       int codec_type, const char *arg)
> {
>-    AVCodec *p;
>-
>     if (!strcmp(arg, "copy")) {
>         *pstream_copy = 1;
>     } else {
>-        p = first_avcodec;
>-        while (p) {
>-            if (!strcmp(p->name, arg) && p->type == codec_type)
>-                break;
>-            p = p->next;
>-        }
>-        if (p == NULL) {
>-            fprintf(stderr, "Unknown codec '%s'\n", arg);
>-            exit(1);
>-        } else {
>-            *pcodec_id = p->id;
>-        }
>+        *pcodec_name = av_strdup(arg);

where is it freed ? what if several codecs are used in command line ?

>     }
> }
>

[snip]

>@@ -2848,7 +2855,7 @@
>
>     /* reset some key parameters */
>     video_disable = 0;
>-    video_codec_id = CODEC_ID_NONE;
>+    video_codec_name = NULL;

av_free sounds better to me, no ?

>     video_stream_copy = 0;
> }
>
>@@ -2895,8 +2902,8 @@
>                 av_set_double(audio_enc, opt_names[i], d);
>         }
>
>-        if (audio_codec_id != CODEC_ID_NONE)
>-            codec_id = audio_codec_id;
>+        if (audio_codec_name)
>+            codec_id = find_codec_or_die(audio_codec_name,
CODEC_TYPE_AUDIO, 1);
>         audio_enc->codec_id = codec_id;
>
>         if (audio_qscale > QSCALE_NONE) {
>@@ -2916,7 +2923,7 @@
>
>     /* reset some key parameters */
>     audio_disable = 0;
>-    audio_codec_id = CODEC_ID_NONE;
>+    audio_codec_name = NULL;

ditto

>     audio_stream_copy = 0;
> }
>
>@@ -2944,7 +2951,7 @@
>              if(d==d && (opt->flags&AV_OPT_FLAG_SUBTITLE_PARAM) &&
(opt->flags&AV_OPT_FLAG_ENCODING_PARAM))
>                  av_set_double(subtitle_enc, opt_names[i], d);
>         }
>-        subtitle_enc->codec_id = subtitle_codec_id;
>+        subtitle_enc->codec_id =
find_codec_or_die(subtitle_codec_name, CODEC_TYPE_SUBTITLE, 1);
>     }
>
>     if (subtitle_language) {
>@@ -2954,7 +2961,7 @@
>     }
>
>     subtitle_disable = 0;
>-    subtitle_codec_id = CODEC_ID_NONE;
>+    subtitle_codec_name = NULL;

ditto

>     subtitle_stream_copy = 0;
> }
>

-- 
Ben
Purple Labs S.A.
www.purplelabs.com