[FFmpeg-devel] [PATCH] Set loglevel with strings in ffmpeg.c.

[Michael Niedermayer]

On Fri, Jun 12, 2009 at 12:27:18AM +0200, Stefano Sabatini wrote:
> On date Thursday 2009-06-11 18:48:59 -0300, Ramiro Polla encoded:
> > On Thu, Jun 11, 2009 at 6:30 PM, Michael Niedermayer<michaelni at gmx.at> wrote:
> > > On Thu, Jun 11, 2009 at 02:08:41PM -0300, Ramiro Polla wrote:
> > >> $subj makes it more user-friendly, i.e.:
> > >> ffmpeg -loglevel info -i input output
> > >>
> > >> Ramiro Polla
> > >
> > >> ?ffmpeg.c | ? 34 ++++++++++++++++++++++++++++++++--
> > >> ?1 file changed, 32 insertions(+), 2 deletions(-)
> > >> b69c3b8cd2469ff83024c20d90ce7b5da1fe5117 ?loglevel.diff
> > >
> > > iam not sure if this is worth it but i am sure ffmpeg.c is the wrong place
> > > ffplay & ffserver should also support that
> > 
> > libavutil/options.c maybe?
> 
> In order to set an AVOption, that option needs to correspond to a
> field of a context with an AVClass, which is not the case, so
> cmdutils.[hc] is the place.

exactly


> 
> > I don't want to spend much more time on this patch, but maybe Stefano
> > wants to pick it up =)
> 
> I always enjoy to improve the interface and move code from apps to
> utils code ;-), check attached.

i sometimes feel that you enjoy driving me crazy ;)

anyway the patch is ok when you split it into cosmetic move
and functional changes also minor comment below


[...]
> Index: cmdutils.c
> ===================================================================
> --- cmdutils.c	(revision 19156)
> +++ cmdutils.c	(working copy)
> @@ -213,6 +213,42 @@
>      return 0;
>  }
>  
> +int opt_loglevel(const char *opt, const char *arg)
> +{
> +    const struct { const char *name; int level; } const log_levels[] = {
> +        { "quiet"  , AV_LOG_QUIET   },
> +        { "panic"  , AV_LOG_PANIC   },
> +        { "fatal"  , AV_LOG_FATAL   },
> +        { "error"  , AV_LOG_ERROR   },
> +        { "warning", AV_LOG_WARNING },
> +        { "info"   , AV_LOG_INFO    },
> +        { "verbose", AV_LOG_VERBOSE },
> +        { "debug"  , AV_LOG_DEBUG   },
> +    };
> +    char *endptr;
> +    int i, level;
> +
> +    for (i = 0; i < FF_ARRAY_ELEMS(log_levels); i++) {
> +        if (!strcmp(log_levels[i].name, arg)) {
> +            level = log_levels[i].level;
> +            goto set_log_level;
> +        }
> +    }
> +
> +    level = strtol(arg, &endptr, 10);
> +    if (*endptr) {
> +        fprintf(stderr, "Invalid loglevel \"%s\". "
> +                "Possible levels are numbers or:\n", arg);
> +        for (i = 0; i < FF_ARRAY_ELEMS(log_levels); i++)
> +            fprintf(stderr, "\"%s\"\n", log_levels[i].name);
> +        exit(1);
> +    }
> +
> +set_log_level:
> +    av_log_set_level(level);
> +    return 0;
> +}

as much as i like gotos, this one can be done by just replacing the
goto with the 2 lines without increasing the number of lines at all


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

Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090612/d6dabdba/attachment.pgp>