[FFmpeg-devel] [PATCH] Move av_parse_frame_size() and av_parse_frame_rate() from libavcodec to libavcore

[Michael Niedermayer]

On Sat, Jul 24, 2010 at 01:36:28PM +0200, Stefano Sabatini wrote:
> On date Saturday 2010-07-24 04:18:22 +0200, Michael Niedermayer encoded:
> > On Fri, Jul 23, 2010 at 12:57:15PM +0200, Stefano Sabatini wrote:
> > [...]
> > > +
> > > +int av_parse_video_frame_rate(AVRational *frame_rate, const char *arg)
> > > +{
> > > +    int i;
> > > +    int n = FF_ARRAY_ELEMS(video_frame_rate_abbrs);
> > > +    char* cp;
> > > +
> > > +    /* First, we check our abbreviation table */
> > > +    for (i = 0; i < n; ++i)
> > > +         if (!strcmp(video_frame_rate_abbrs[i].abbr, arg)) {
> > 
> > > +             frame_rate->num = video_frame_rate_abbrs[i].rate_num;
> > > +             frame_rate->den = video_frame_rate_abbrs[i].rate_den;
> > 
> > video_frame_rate_abbrs should contain AVRationals
> > and i wonder if it would be simpler if both functions accepted AVRational
> 
> A size is not a rational, also I'm not sure the following is legal:

its not rational, no i just thought it would allow the string searching
code to be reused


> { "pal", (AVRational) {1, 25} },
>   
> > > +             return 0;
> > > +         }
> > > +
> > > +    /* Then, we try to parse it as fraction */
> > > +    cp = strchr(arg, '/');
> > > +    if (!cp)
> > > +        cp = strchr(arg, ':');
> > > +    if (cp) {
> > > +        char* cpp;
> > > +        frame_rate->num = strtol(arg, &cpp, 10);
> > > +        if (cpp != arg || cpp == cp)
> > > +            frame_rate->den = strtol(cp+1, &cpp, 10);
> > > +        else
> > > +           frame_rate->num = 0;
> > 
> > indention
> > {}
> > yes they are seperate from moving it of course ...
> > still i think you could try patcheck and fix the things
> > 
> > 
> > > +    }
> > [...]
> > > +
> > > +/**
> > > + * Parse str and put in width_ptr and height_ptr the detected values.
> > > + *
> > > + * @return 0 in case of a successful parsing, a negative value otherwise
> > 
> > this is unflexible for public abi
> > because it just says we will never return a value >0
> > doing so would break abi
> > if you say <0 is error >= 0 is no error then additional information could be
> > returned in the future without breaking abi/api
> 
> These comments are all unrelated to the proposed change, I'll fix them
> with other patches but after this patch is applied.

for being strictly correct, you have to fix the api/abi before making it
available from another lib

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 190 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100725/571bace6/attachment.pgp>