On Wed, Oct 12, 2011 at 01:35:19PM +0100, [email protected] wrote:
>
> --- a/avconv.c
> +++ b/avconv.c
> @@ -2966,15 +2968,62 @@ static void parse_forced_key_frames(char *kf,
> OutputStream *ost,
>
> + while ((c = avio_r8(s)) && (c != '\n'))
The second set of parentheses is unnecessary.
> +static int get_preset_file_2(const char *preset_name, const char
> *codec_name, AVIOContext **s)
nit: This long line can easily be shortened.
Why the _2 suffix in the function name?
> + int i, ret = 1;
> + char filename[1000];
> + const char *base[3] = { getenv("AVCONV_DATADIR"),
> + getenv("HOME"),
> + AVCONV_DATADIR,
> + };
nit:
AVCONV_DATADIR, };
> + for (i = 0; i < FF_ARRAY_ELEMS(base) && ret; i++) {
> + if (!base[i])
> + continue;
> + if (codec_name) {
> + snprintf(filename, sizeof(filename), "%s%s/%s-%s.ffpreset",
> base[i],
> + i != 1 ? "" : "/.avconv", codec_name, preset_name);
> + ret = avio_open(s, filename, AVIO_FLAG_READ);
> + }
> + if (ret) {
> + snprintf(filename, sizeof(filename), "%s%s/%s.ffpreset",
> base[i],
> + i != 1 ? "" : "/.avconv", preset_name);
> + ret = avio_open(s, filename, AVIO_FLAG_READ);
> + }
> + }
I think this is not quite what we want. The directory name we search
should only be a hidden directory with a leading period in the home
directory. For the system directories it is customary to use normal
directory names, i.e.
/home/foo/.avconv/
/usr/share/avconv/
> @@ -2996,6 +3045,30 @@ static OutputStream *new_output_stream(OptionsContext
> *o, AVFormatContext *oc, e
> + if (ret) {
> + av_log(NULL, AV_LOG_FATAL, " Preset %s specified for stream %d:%d,"
> + "but could not be opened.\n", preset, ost->file_index,
> ost->index);
IMO more readable:
av_log(NULL, AV_LOG_FATAL,
"Preset %s specified for stream %d:%d, but could not be opened.\n",
preset, ost->file_index, ost->index);
This also fixes a missing space after comma.
> @@ -3942,6 +4015,7 @@ static const OptionDef options[] = {
> { "codec", HAS_ARG | OPT_STRING | OPT_SPEC, {.off =
> OFFSET(codec_names)}, "codec name", "codec" },
> + { "pre", HAS_ARG | OPT_STRING | OPT_SPEC, {.off = OFFSET(presets)},
> "preset name", "preset" },
> { "map", HAS_ARG | OPT_EXPERT | OPT_FUNC2, {(void*)opt_map}, "set input
> stream mapping", "file.stream[:syncfile.syncstream]" },
I suggest "preset" instead, which is much more intuitive.
> --- a/doc/avconv.texi
> +++ b/doc/avconv.texi
> @@ -770,6 +772,21 @@ quality).
>
> +@section Preset file
> +
> +A preset file contains a sequence of @var{option}=@var{value} pairs, one for
@var{option=value} should do.
> +each line, specifying a sequence of options which would be awkward to
> specify on
> +the command line.
I don't like the word "awkward" here. Whether or not the options are
awkward to use is irrelevant. What's more important is that the options
in the config file are the same as the command line ones.
> Check the @file{ffpresets} directory in the Libav source tree for examples.
I wonder why we keep using "ff" as prefix, I don't see that making sense
anymore.
> +Preset files are specified with the @code{pre} option, this option takes a
> +preset name as input. Avconv searches for a file named @var{arg}.ffpreset in
"arg" has no meaning in this context, you never defined it.
> +the directories @file{$AVCONV_DATADIR} (if set), and @file{$HOME/.avconv},
> and in
> +the datadir defined at configuration time (usually
> @file{$PREFIX/share/avconv})
data directory
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
