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

[Lukasz Marek]

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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-ffserver_config-postpone-codec-context-creation.patch
Type: text/x-patch
Size: 29026 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20141025/5323a891/attachment.bin>