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

[Stefano Sabatini]

On date Friday 2007-06-01 12:40:05 +0200, Benoit Fouet encoded:
> 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

Look at the attached patch if it looks better now.
 
> > @@ -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...

OK, putted each documentation string on a dedicated line.
 
> > 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

Fixed.

> > + * 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

Fixed.
 
> > + * 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

You're right, in every case I changed the code to use only the @param
syntax, since the @param[in] notation isn't used anywhere in the
header.
 
> > + * 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)

Fixed.

> [...]
> > +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
[...]

Fixed, now the functions return 0 or -1.

Cheers.
-- 
Stefano Sabatini
Linux user number 337176 (see http://counter.li.org)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: list-video-size-rate-abvs-01.patch
Type: text/x-diff
Size: 8242 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070601/1e104edd/attachment.patch>