[FFmpeg-devel] [PATCH 4/4] ffserver_config: postpone codec context creation

[Reynaldo H. Verdejo Pinochet]

Hi Lukasz

On 10/22/2014 06:44 PM, Lukasz Marek wrote:
> [..]
> To save your time:
> 1. I updated ffserver_apply_stream_config function,
> 2. added comments in FFServerConfig struct in header (according to
> Stefano's remark)
> 3. in ffserver_parse_config_stream, at the end added:
>         av_freep(&config->video_preset);
>         av_freep(&config->audio_preset);
>    and checking result of ffserver_apply_stream_config calls

Thanks for the sum up.

> [..]
> @@ -502,6 +503,95 @@ static int ffserver_parse_config_feed(FFServerConfig *config, const char *cmd, c
>      return 0;
>  }
>  
> +static int ffserver_apply_stream_config(AVCodecContext *enc, const AVDictionary *conf, AVDictionary **opts)
> +{
> +    static const char *error_message = "Cannot parse '%s' as number for %s parameter.\n";
> +    AVDictionaryEntry *e;
> +    char *tailp;
> +    int ret = 0;
> +
> +#define SET_INT_PARAM(factor, param, key)                   \
> +    if ((e = av_dict_get(conf, #key, NULL, 0))) {           \
> +        if (!e->value[0]) {                                 \
> +            av_log(NULL, AV_LOG_ERROR, error_message, e->value, #key); \
> +            ret = AVERROR(EINVAL);                          \
> +        }                                                   \
> +        enc->param = strtol(e->value, &tailp, 0);           \
> +        if (factor) enc->param *= (factor);                 \
> +        if (tailp[0] || errno) {                            \
> +            av_log(NULL, AV_LOG_ERROR, error_message, e->value, #key); \
> +            ret = AVERROR(errno);                           \
> +        }                                                   \
> +    }
> +#define SET_DOUBLE_PARAM(factor, param, key)                \
> +    if ((e = av_dict_get(conf, #key, NULL, 0))) {           \
> +        if (!e->value[0]) {                                 \
> +            av_log(NULL, AV_LOG_ERROR, error_message, e->value, #key); \
> +            ret = AVERROR(EINVAL);                          \
> +        }                                                   \
> +        enc->param = strtod(e->value, &tailp);              \
> +        if (factor) enc->param *= (factor);                 \
> +        if (tailp[0] || errno) {                            \
> +            av_log(NULL, AV_LOG_ERROR, error_message, e->value, #key); \
> +            ret = AVERROR(errno);                           \
> +        }                                                   \
> +    }

Can you turn those two macros into static inline funcs?. Also,
looks like it shouldn't be too hard to factor those two into
just 1 func. This is mostly to aid debugging. Nothing vital.

> [..]
> @@ -624,136 +712,104 @@ static int ffserver_parse_config_stream(FFServerConfig *config, const char *cmd,
>          stream->max_time = atof(arg) * 1000;
>      } else if (!av_strcasecmp(cmd, "AudioBitRate")) {
>          ffserver_get_arg(arg, sizeof(arg), p);
> -        config->audio_enc.bit_rate = lrintf(atof(arg) * 1000);
> -    } else if (!av_strcasecmp(cmd, "AudioChannels")) {
> +        av_dict_set_int(&config->audio_conf, cmd, lrintf(atof(arg) * 1000), 0);
> +    } else if (!av_strcasecmp(cmd, "AudioChannels") ||
> +               !av_strcasecmp(cmd, "AudioSampleRate")) {
>          ffserver_get_arg(arg, sizeof(arg), p);
> -        config->audio_enc.channels = atoi(arg);
> -    } else if (!av_strcasecmp(cmd, "AudioSampleRate")) {
> -        ffserver_get_arg(arg, sizeof(arg), p);
> -        config->audio_enc.sample_rate = atoi(arg);
> +        av_dict_set(&config->audio_conf, cmd, arg, 0);

Here and on every occurrence:

Any particular reason why not to check av_dict_set*()'s return
for < 0? Only reason I ask is because the config code already
has failed allocation checks elsewhere. Also, should help avoiding
some coverity scan noise.

> [..]
>      } else if (!av_strcasecmp(cmd, "NoAudio")) {
> @@ -783,16 +839,32 @@ static int ffserver_parse_config_stream(FFServerConfig *config, const char *cmd,
>      } else if (!av_strcasecmp(cmd, "</Stream>")) {
>          if (stream->feed && stream->fmt && strcmp(stream->fmt->name, "ffm") != 0) {
>              if (config->audio_id != AV_CODEC_ID_NONE) {
> -                config->audio_enc.codec_type = AVMEDIA_TYPE_AUDIO;
> -                config->audio_enc.codec_id = config->audio_id;
> -                add_codec(stream, &config->audio_enc);
> +                AVCodecContext *audio_enc = avcodec_alloc_context3(avcodec_find_encoder(config->audio_id));
> +                if (config->audio_preset &&
> +                    ffserver_opt_preset(arg, audio_enc, AV_OPT_FLAG_AUDIO_PARAM|AV_OPT_FLAG_ENCODING_PARAM,
> +                                        NULL, NULL) < 0)
> +                    ERROR("Could not apply preset '%s'\n", arg);
> +                if (ffserver_apply_stream_config(audio_enc, config->audio_conf, &config->audio_opts) < 0)
> +                    config->errors++;
> +                add_codec(stream, audio_enc);
>              }
>              if (config->video_id != AV_CODEC_ID_NONE) {
> -                config->video_enc.codec_type = AVMEDIA_TYPE_VIDEO;
> -                config->video_enc.codec_id = config->video_id;
> -                add_codec(stream, &config->video_enc);
> +                AVCodecContext *video_enc = avcodec_alloc_context3(avcodec_find_encoder(config->video_id));
> +                if (config->video_preset &&
> +                    ffserver_opt_preset(arg, video_enc, AV_OPT_FLAG_VIDEO_PARAM|AV_OPT_FLAG_ENCODING_PARAM,
> +                                        NULL, NULL) < 0)
> +                    ERROR("Could not apply preset '%s'\n", arg);

I usually add braces for single statement ifs when the condition itself
has multiple lines. But I'm OK either way.

> [..]
> -
> +    AVDictionary *video_opts;     /* Contains AVOptions for video encoder */
> +    AVDictionary *video_conf;     /* Contains values stored in video AVCodecContext.fields */
> +    AVDictionary *audio_opts;     /* Contains AVOptions for audio encoder */
> +    AVDictionary *audio_conf;     /* Contains values stored in audio AVCodecContext.fields */

Would drop the repeated "Contains".

Everything looks OK otherwise. Thanks a lot.

Bests,


-- 
Reynaldo H. Verdejo Pinochet
Open Source Group
Samsung Research America / Silicon Valley