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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
