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

[Lukasz Marek]

On 25.10.2014 19:53, Lukasz Marek wrote:
> On 24.10.2014 00:18, Reynaldo H. Verdejo Pinochet wrote:
>> Hi Lukasz
>>> +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.
>
> I changed macros to functions. I think inline is not required in such
> code. Small comment, there is plenty of atoi in parsing code.
> It returns 0 in case parsed string is not a number and ignores random
> suffixed. Probably more problems can be pointed here. I prepared these
> functions to replace all atoi with them.
>
> Since these functions allows to check ranges, I split back all options
> to separate if's, so every option can have its own range.
>
>>> [..]
>>> @@ -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.
>
> I forgot about that. Checks added.
>
>
>>> [..]
>>> -
>>> +    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".
>
> Dropped.

Patch OK'ed by Reynaldo in private email. Pushed.