[FFmpeg-devel] [PATCH] List supported video size and rate abbreviations

[Benoit Fouet]

Hi,

Stefano Sabatini wrote:

[snip]

> Index: ffmpeg.c
> ===================================================================
> --- ffmpeg.c	(revision 9165)
> +++ ffmpeg.c	(working copy)
> @@ -2065,14 +2065,29 @@
>      av_log_level = atoi(arg);
>  }
>  
> -static void opt_frame_rate(const char *arg)
> +static void list_video_rate_abvs(void)
>  {
> -    if (parse_frame_rate(&frame_rate, &frame_rate_base, arg) < 0) {
> -        fprintf(stderr, "Incorrect frame rate\n");
> -        exit(1);
>   

cosmetics should belong to a separate patch

> +    int i;
> +    char video_rate_abv_str[128];
> +    for (i=-1; i < video_size_rate_abv_nb; i++) {
> +        if (avcodec_video_rate_abv_string (video_rate_abv_str, sizeof(video_rate_abv_str), i))
> +            fprintf(stdout, "%s\n", video_rate_abv_str);
>      }
>  }
>  
>   

[snip]

>  static void opt_frame_size(const char *arg)
>  {
> -    if (parse_image_size(&frame_width, &frame_height, arg) < 0) {
> -        fprintf(stderr, "Incorrect frame size\n");
> -        exit(1);
>   

and here too

> +    if (strcmp (arg, "list") == 0) {
> +        list_video_size_abvs();
> +        exit(0);
> +    } else {
> +        if (parse_image_size(&frame_width, &frame_height, arg) < 0) {
> +            fprintf(stderr, "Incorrect frame size\n");
> +            exit(1);
> +        }
> +        if ((frame_width % 2) != 0 || (frame_height % 2) != 0) {
> +            fprintf(stderr, "Frame size must be a multiple of 2\n");
> +            exit(1);
> +        }
>      }
> -    if ((frame_width % 2) != 0 || (frame_height % 2) != 0) {
> -        fprintf(stderr, "Frame size must be a multiple of 2\n");
> -        exit(1);
> -    }
>  }
>   

ditto

>  
>  
> @@ -3607,8 +3637,8 @@
>      /* video options */
>      { "vframes", OPT_INT | HAS_ARG | OPT_VIDEO, {(void*)&max_frames[CODEC_TYPE_VIDEO]}, "set the number of video frames to record", "number" },
>      { "dframes", OPT_INT | HAS_ARG, {(void*)&max_frames[CODEC_TYPE_DATA]}, "set the number of data frames to record", "number" },
> -    { "r", HAS_ARG | OPT_VIDEO, {(void*)opt_frame_rate}, "set frame rate (Hz value, fraction or abbreviation)", "rate" },
> -    { "s", HAS_ARG | OPT_VIDEO, {(void*)opt_frame_size}, "set frame size (WxH or abbreviation)", "size" },
> +    { "r", HAS_ARG | OPT_VIDEO, {(void*)opt_frame_rate}, "set frame rate (Hz value, fraction or abbreviation), 'list' as argument shows all the video rate abbreviations supported", "rate" },
> +    { "s", HAS_ARG | OPT_VIDEO, {(void*)opt_frame_size}, "set frame size (WxH or abbreviation), 'list' as argument shows all the video size abbreviations supported", "size" },
>   

i personnaly prefer breaking long lines, but i don't know the exact
policy about it here...

> Index: libavformat/avformat.h
> ===================================================================
> --- libavformat/avformat.h	(revision 9165)
> +++ libavformat/avformat.h	(working copy)
> @@ -883,6 +883,52 @@
>  
>  int match_ext(const char *filename, const char *extensions);
>  
> +
> +/**
> + * Number of supported video size and rate abbreviations supported.
> + */
> +extern int const video_size_rate_abv_nb;
> +
> +/**
> + * Print in buf the string corresponding to the video size
>   

Prints

> + * abbreviation with number video_size_abv.  FFmpeg supports
> + * video_size_rate_abv_nb different video size abbreviations, so the
> + * argument may vary between 0 and video_size_rate_abv_nb -1. If
> + * video_size_abv is negative it will print an header, if
>   

a header

> + * video_size_abv is greater or equal to video_size_rate_abv_nb it
> + * will do nothing and return NULL.
> + *
> + * @return the pointer to the filled buffer, or NULL if video_size_abv is meaningless.
> + * @param buf[in] the buffer where to write the string
> + * @param buf_size[in] the size of buf
> + * @param video_size_abv[in] the number of the video size abbreviation whose
>   

IIRC, the syntax is @param[in] param_name

> + * information you want to get. Meaningful values vary from 0 to video_size_rate_abv_nb -1,
> + * with a negative number print an header.
>   

redundant
(the same applies for the second doxy comment)

> + */
> +char *avcodec_video_size_abv_string (char *buf, int buf_size, int video_size_abv);
> +
> +/**
> + * Print in buf the string corresponding to the video rate
> + * abbreviation with number video_rate_abv.  FFmpeg supports up to
> + * video_size_rate_abv_nb different video rate abbreviations, so the
> + * argument may vary between 0 and video_size_rate_abv_nb -1, but not
> + * every one of these numbers will correspond to a video framerate. If
> + * video_rate_abv is negative it will print an header, if
> + * video_rate_abv is greater or equal to video_size_rate_abv_nb it
> + * will do nothing and return NULL.
> + *
> + * @return the pointer to the filled buffer, or NULL if video_rate_abv is meaningless
> + * or the corresponding entry doesn't match a valid video framerate.
> + * @param buf[in] the buffer where to write the string
> + * @param buf_size[in] the size of buf
> + * @param video_rate_abv[in] the number of the video rate abbreviation
> + * whose information you want to get. Meaningful values vary from 0 to
> + * video_size_rate_abv_nb -1, but not every one of these will
> + * correspond to a video rate abbreviation (so the function may
> + * return NULL), with a negative number print an header,
> + */
> +char *avcodec_video_rate_fmt_string (char *buf, int buf_size, int video_size_fmt);
> +
>  #endif /* HAVE_AV_CONFIG_H */
>  
>  #endif /* AVFORMAT_H */
> Index: libavformat/utils.c
> ===================================================================
> --- libavformat/utils.c	(revision 9165)
> +++ libavformat/utils.c	(working copy)
> @@ -2516,6 +2516,52 @@
>      { "hd1080",   1920,1080,     0,    0 },
>  };
>  
> +int const video_size_rate_abv_nb= sizeof (frame_abvs) / sizeof (AbvEntry);
> +
> +char *avcodec_video_size_abv_string (char *buf, int buf_size, int video_size_abv)
> +{
>   

now that i've read how they work, i think this function should return
error code instead of the string

> +    AbvEntry info= frame_abvs[video_size_abv];
> +
> +    /* print header */
> +    if (video_size_abv < 0) {
> +        snprintf (buf, buf_size,
> +                  "name      " " width" " height"
> +            );
> +        return buf;
> +    } else if (video_size_abv < video_size_rate_abv_nb) {
> +        snprintf (buf, buf_size,
> +                  "%-10s" " %4d " "  %4d ",
> +                  info.abv,
> +                  info.width,
> +                  info.height
> +            );
> +        return buf;
> +    } else
> +        return NULL;
> +}
>   

something like:
if (video_size_abv < 0){
    snprintf(...);
else if (video_size_abv < video_size_rate_abv_nb) {
    snprintf(..);
else
    return -1;
return 0;

same for the next one

> +
> +char *avcodec_video_rate_abv_string (char *buf, int buf_size, int video_rate_abv)
> +{
> +    AbvEntry info= frame_abvs[video_rate_abv];
> +
> +    /* print header */
> +    if (video_rate_abv < 0) {
> +        snprintf (buf, buf_size,
> +                  "name      "  " frame_rate" " frame_rate_base"
> +            );
> +        return buf;
> +    } else if (video_rate_abv < video_size_rate_abv_nb && info.frame_rate != 0) {
> +        snprintf (buf, buf_size,
> +                  "%-10s" "    %5d   " "      %4d      ",
> +                  info.abv,
> +                  info.frame_rate,
> +                  info.frame_rate_base
> +            );
> +        return buf;
> +    } else
> +        return NULL;
> +}
> +
>   

as always, i'm not maintainer of anything,so this is just my 2 cents ;)

Ben
-- 
Purple Labs S.A.
www.purplelabs.com