[FFmpeg-devel] [PATCH 6/9] ffserver_config: handle codec private options

[Reynaldo H. Verdejo Pinochet]

Hi Lukasz

On 11/11/2014 04:31 AM, Lukasz Marek wrote:
> This commit allows to set codec's private option.
> As side effect this commit also improves preset support.

s/this commit also/, it/

> [..]
> +static int ffserver_save_avoption(AVCodecContext *ctx, const char *opt, const char *arg,
> +                                  AVDictionary **dict, int type, FFServerConfig *config, int line_num);
> +static void vreport_config_error(const char *filename, int line_num, int log_level, int *errors, const char *fmt, va_list vl);
> +static void report_config_error(const char *filename, int line_num, int log_level, int *errors, const char *fmt, ...);
> +

These random length line breaks are getting a bit messy, I don't
mind if you don't wana break at 80 but please be consistent.

> [..]
> @@ -293,16 +295,31 @@ static int ffserver_opt_preset(const char *arg,
>               ret = AVERROR(EINVAL);
>               break;
>           }
> -        if (audio_id && !strcmp(tmp, "acodec")) {
> -            *audio_id = opt_codec(tmp2, AVMEDIA_TYPE_AUDIO);
> -        } else if (video_id && !strcmp(tmp, "vcodec")){
> -            *video_id = opt_codec(tmp2, AVMEDIA_TYPE_VIDEO);
> -        } else if(!strcmp(tmp, "scodec")) {
> -            /* opt_subtitle_codec(tmp2); */
> -        } else if (avctx && (ret = ffserver_opt_default(tmp, tmp2, avctx, type)) < 0) {
> -            fprintf(stderr, "%s: Invalid option or argument: '%s', parsed as "
> -                    "'%s' = '%s'\n", filename, line, tmp, tmp2);
> -            break;
> +        if ((!strcmp(tmp, "acodec") && avctx->codec_type == AVMEDIA_TYPE_AUDIO) ||
> +             !strcmp(tmp, "vcodec") && avctx->codec_type == AVMEDIA_TYPE_VIDEO)
> +        {
> +            if (ffserver_set_codec(avctx, tmp2, config, line_num) < 0)
> +                break;

Factor in previous if() condition?

> +        } else if (!strcmp(tmp, "scodec")) {
> +            /* nothing to do */

Not sure why are we leaving this one there? Proly drop and add
an informative comment if needed.

> +        } else {
> +            int type;
> +            AVDictionary **opts;

Add blank line please (Can you do it for other similar occurrences?)

>[..]
> @@ -406,27 +424,67 @@ static int ffserver_set_float_param(float *dest, const char *value, float factor
>       return AVERROR(EINVAL);
>   }
>
> -static int ffserver_save_avoption(const char *opt, const char *arg, AVDictionary **dict,
> +static int ffserver_save_avoption(AVCodecContext *ctx, const char *opt, const char *arg, AVDictionary **dict,
>                                     int type, FFServerConfig *config, int line_num)
>   {
> +    static int hinted = 0;
>       int ret = 0;
>       AVDictionaryEntry *e;
> -    const AVOption *o = av_opt_find(config->dummy_ctx, opt, NULL, type | AV_OPT_FLAG_ENCODING_PARAM, AV_OPT_SEARCH_CHILDREN);
> +    const AVOption *o = NULL;
> +    const char *option = NULL;
> +    const char *codec_name = NULL;
> +
> +    if (strchr(opt, ':')) {
> +        //explicit private option
> +        char buff[1024];

See above (there are others though)

> +        snprintf(buff, sizeof(buff), "%s", opt);

Pointless sizeof but it's OK.

> +        codec_name = buff;
> +        option = strchr(buff, ':');
> +        buff[option - buff] = '\0';

Can't strchr() return NULL and wreak havoc here?

> +        option++;
> +        if ((ret = ffserver_set_codec(ctx, codec_name, config, line_num)) < 0)
> +            return ret;
> +        if (!ctx->codec || !ctx->priv_data)
> +            return -1;
> +    } else {
> +        option = opt;
> +    }
> +
> +    o = av_opt_find(ctx, option, NULL, type | AV_OPT_FLAG_ENCODING_PARAM, AV_OPT_SEARCH_CHILDREN);
>       if (!o) {
>           report_config_error(config->filename, line_num, AV_LOG_ERROR,
> -                &config->errors, "Option not found: %s\n", opt);
> -    } else if ((ret = av_opt_set(config->dummy_ctx, opt, arg, AV_OPT_SEARCH_CHILDREN)) < 0) {
> +                            &config->errors, "Option not found: %s\n", opt);
> +        if (!hinted && ctx->codec_id == AV_CODEC_ID_NONE) {
> +            enum AVCodecID id;
> +            hinted = 1;
> +            switch(ctx->codec_type) {
> +            case AVMEDIA_TYPE_AUDIO:
> +                id = config->guessed_audio_codec_id != AV_CODEC_ID_NONE ? config->guessed_audio_codec_id : AV_CODEC_ID_AAC;
> +                break;
> +            case AVMEDIA_TYPE_VIDEO:
> +                id = config->guessed_video_codec_id != AV_CODEC_ID_NONE ? config->guessed_video_codec_id : AV_CODEC_ID_H264;
> +                break;
> +            default:
> +                break;
> +            }
> +            report_config_error(config->filename, line_num, AV_LOG_ERROR, NULL,
> +                                "If '%s' is codec private option, then prefix it with codec name, "

is a

Rest looks OK at a first glance. Thanks a lot

Bests,


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