[Ffmpeg-devel] [PATCH] Add kibi, mibi and gibi support

[Michael Niedermayer]

Hi

On Sat, Sep 16, 2006 at 11:50:39AM +0200, Panagiotis Issaris wrote:
> Hi,
> 
> On za, 2006-09-16 at 11:21 +0200, Panagiotis Issaris wrote:
> > On za, 2006-09-16 at 10:13 +0200, Panagiotis Issaris wrote:
> > > On za, 2006-09-16 at 09:56 +0200, Panagiotis Issaris wrote:
> > >[...]
> > > > The attached patch adds support for the "ki", "Mi" and "Gi" postfixes to
> > > > the AVOption option parsing code. I do know that this in fact is
> > > > incorrect, as the official SI standard uses "Ki" instead of "ki" "for
> > > > consistency reasons" [1]. 
> > > > 
> > > > I was wondering how to solve this? Should I just make the kilo case
> > > > insensitive thus allow both 'k' and 'K' for both? Or make all of them
> > > > case insensitive? What about 'B' then? I had the initial intention to
> > > > have 'B' stand for "byte" and 'b' for "bit" with "" being a shorthand
> > > > for 'b'. If case sensitivity would be removed, both "b" and "B" would
> > > > stand for "byte" (in the current implementation).
> > Regression tests succeed with this new corrected version of the same
> > patch.
> There was another bug in the av_strtod() function. The attached third
> version of this patch fixes this.
> 
> The problem was that av_strtod() was not correctly interpreting strings
> which contained no number. If the first call to strtod() indicated that
> there was no number, it should have returned immediately. But it didn't
> which caused it to interpret the first 'i' of "-dct int" as some kind of
> number. This was also due to the previous bug, which allowed using "i"
> without an earlier "k", "M" or "G" which makes no sense at all. Only
> fixing this behavior would ofcourse be wrong, as any other flag value,
> starting with a B would cause yet another option interpretation problem.
> 
>  opt.c |   24 +++++++++++++++++-------
>  1 files changed, 17 insertions(+), 7 deletions(-)
> 
> As this is already the _third_ version of this patch, I will do another
> thorough review of the related code, but I'm sending it to the ml
> anyways in case anyone sees other bugs.
> 
> Regression tests succeed.
> 
> With friendly regards,
> Takis

> Index: libavcodec/opt.c
> ===================================================================
> --- libavcodec/opt.c	(revision 6279)
> +++ libavcodec/opt.c	(working copy)
> @@ -27,26 +27,36 @@
>  #include "avcodec.h"
>  #include "opt.h"
>  
> -/**
> - * strtod() function extended with 'k', 'M' and 'B' postfixes.
> - * This allows using kB, MB, k, M and B as a postfix. This function
> - * assumes that the unit of numbers is bits not bytes.
> +/** strtod() function extended with 'k', 'M', 'G', 'ki', 'Mi', 'Gi' and 'B'
> + * postfixes.  This allows using f.e. kB, MiB, G and B as a postfix. This
> + * function assumes that the unit of numbers is bits not bytes.
>   */
>  static double av_strtod(const char *name, char **tail) {
>      double d;
> +    int p = 0;
>      d= strtod(name, tail);
> +    if (*tail<=name)
> +        return d;
> +
>      if(*tail>name && (**tail=='k')) {
> -        d*=1000;
> +        p = 1;
>          (*tail)++;
>      }
>      else if(*tail && (**tail=='M')) {

why the difference between *tail>name and *tail in the 2 if() ?
it alsi seems that with the if (*tail<=name) return d; the *tail checks
are unneeded? or other way around the *tail if changed to *tail>name
would make the if (*tail<=name) return d unneeded ?


> -        d*=1000000;
> +        p = 2;
>          (*tail)++;
>      }
>      else if(*tail && (**tail=='G')) {
> -        d*=1000000000;
> +        p = 3;
>          (*tail)++;
>      }
> +    if (p>0)

the (*tail)++ can be moved here, that would remove 2 of them

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

In the past you could go to a library and read, borrow or copy any book
Today you'd get arrested for mere telling someone where the library is