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

[Ramiro Polla]

On Sun, Jun 14, 2009 at 7:20 PM, Stefano
Sabatini<stefano.sabatini-lala at poste.it> wrote:
> On date Friday 2009-06-12 00:46:12 +0200, Michael Niedermayer encoded:
>> 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 ;)
>
> That's not intentional, but looks like I'm very talented for it ;-).
>
>> anyway the patch is ok when you split it into cosmetic move
>> and functional changes also minor comment below
>
> Yep.
>
>> [...]
>> > 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
>
> Check attached.

> +            level = log_levels[i].level;
> +            av_log_set_level(level);
> +            return 0;

av_log_set_level(log_levels[i].level);

And I see now that on other parts of ffmpeg.c, it calls the second
parameter of strtol *tail, and not *endptr like I called it. Better
use *tail for consistency.

Ramiro Polla