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

[Stefano Sabatini]

On date Sunday 2010-07-25 02:02:55 +0200, Michael Niedermayer encoded:
> 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

I don't see why this is necessary, the function was already public,
doing it after or before makes no difference (apart from wasting my
time, which I'd like to avoid).

Also note that most FFmpeg functions return 0 in case of success, <0
in case of failure, changing these functions behavior looks weird.

Regards.
-- 
FFmpeg = Fiendish & Faithless Moronic Proud Evangelical Gadget