[MPlayer-dev-eng] [PATCH] Profiles support for MEncoder

[The Wanderer]

Danny - liste wrote:

> Hello to all.
> 
> With this patch MEncoder support a sort of profiles: profiles are a
> set of encoding options listed in some 'profiles file', modificable
> by the user.

Judging by your provided documentation, it looks to me like an extension
to the config system - and, as Guillaume said, a good idea if well
implemented.

> The use is something like
> 
>    mencoder file.mpg -profiles mpg4_hq,mp3
> 
> or
> 
>    mencoder video.avi -profiles dvd_pal,ac3
> 
> The profile are stored in files, where [name] start the profile,
> valid until the end of file or the beginning of a new one. Take a
> look at the file mencoder.pro included to see a sample.

I would recommend '~/.mplayer/profiles' and
'$global_mplayer_conf_dir/profiles' rather than 'mencoder.pro'. At the
very least, use "mencoder-profiles' instead.

> This file is also copied in the configuration directory by the
> install (using a cp, I don't know if is the rigth way to do it ...).
> 
> The documentation part is really brute (sorry) but is a start.
> 
> In the future this can be a place where put some control about the
> encoding (for examples the maximum keyframe for a dvd encoding) or,
> using a simple parser, using parametrized profiles, to have something
>  simple for setting the bitrate like:
> 
>    mencoder file.mpg -profiles mpeg4_hq(1200),mp3(160)
> 
> It's far for complete but is usable and I think is a nice thing to
> have ;)
> 
> What's missing is:
> 
>    . MP_MSG handing: I put some printf (commented by //@) that may be
> use the standard handling.

They're not all commented out - I count seven that would still be
executed. (See below.) Use of stdout for text printing is not
acceptable; if you won't take the time and effort to at least fake it
with mp_msg, it would be better (although still ugly) to use
fprintf(stderr, ...).

>    . A better documentation ;)

Except for grammar/spelling and the like it doesn't look too bad; I'll
look it over when I'm more awake than I am now. (This is sort of a
preliminary run, since I'm here and so I don't forget about it later.)

>    . Some good profile to put in the 'standard' file
> 
>    . profiles for MPlayer ?

I'm not entirely sure what good this would do...

>    . preload the profiles in memory (this can be a must if we have a
> lots of profile, for examples for the various top-box that we have)

I don't understand your example, but I agree that this is probably a
good idea.

> A last note: even if this kind of work can be done with include files
> I prefer to have a single part (the mencoder.pro file) where you have
> some good setup.
> 
> You always have the opportunity to change the default profiles, using
> a user file (that can be put in the ~.mplayer/mencoder
> configuration).

The side of me that a) doesn't like hard-coded values and b) tends to
produce feature bloat wants to make the name/location of the profile
file configurable, via an entry in that same file (and, thus, a
command-line option if need be)... but that might be overkill.


> diff -Naur orig/mencoder.c main/mencoder.c
> --- orig/mencoder.c	2006-01-08 21:32:25.956598488 +0100
> +++ main/mencoder.c	2006-01-08 18:26:25.458252128 +0100

> @@ -286,7 +289,7 @@
>  
>  #include "libao2/audio_out.h"
>  /* FIXME */
> -static void mencoder_exit(int level, char *how)
> +void mencoder_exit(int level, char *how)

Why this change? What is there about the ways you've used mencoder_exit
which would forbid its being static?

> --- orig/mencoder.pro	1970-01-01 01:00:00.000000000 +0100
> +++ main/mencoder.pro	2006-01-08 20:32:43.000000000 +0100
> @@ -0,0 +1,60 @@
> +# MEncoder profile file
> +#
> +[ default ]
> +        profile-desc    en, "Default profile"
> +        profile-desc    it, "Profilo di default"

Is there any use in making the choice of which language to use at
runtime, rather than at compile time? The latter is the way it's done in
every other part of MPlayer at the moment, and I'd rather maintain
consistency unless there's good reason not to.

> +        # default opt
> +        oac             copy=yes
> +        ovc             copy=yes
> +        of              avi=yes
> +        o               encoded.avi
> +        mc              0
> +
> +[ mpeg ]
> +        profile-desc    en, "Default profile for mpeg2 (DVD)"

"MPEG-2"

There are other bits like this (use of incorrect forms) in the rest of
the file. I'll get them in detail when I go over the documentation.

> +static int profile_find_single(m_option_t *config, char *file, char *prof, int no_file_err)

> +            rt = 0;
> +            if (prof == NULL) {
> +                printf("  Profile file:%s\n", file);

Uncommented printf.


> +                t = prof_get_opt(line, &opt, &par);
> +                if (t < 0) {
> +                    // Error -> exit from loop
> +                    printf("Profile parse error: file %s, line:%d\n", file, line_num);

Another one.

> +                    if (prof == NULL) {
> +                        // dump -> print last option
> +                        if (*c_prof != '\0') {
> +                            printf("    %-16s %s\n", c_prof, c_desc);

Again.

> +                                //@ printf("opt:[%s], par=[%s], %d\n",
> +                                //@         opt, par, t);
> +                                if (t < 0) {
> +                                    printf("Profile parse error: file %s, line:%d\n", file, line_num);

Again.


> +
> +            }
> +            if (*c_prof != '\0') {
> +                printf("    %-16s %s\n", c_prof, c_desc);

Again.

> +            }
> +            if (prof == NULL) {
> +                printf("\n");

And again, immediately afterward.

> +static int profile_find_list(m_option_t *config, char *prof)
> +{

> +        if (rt <= 0) {
> +            if (rt == 0) {
> +                printf("Profile not found: [%s]\n", cp);

And one more.

-- 
       The Wanderer

Warning: Simply because I argue an issue does not mean I agree with any
side of it.

Secrecy is the beginning of tyranny.