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

[Danny - liste]

The Wanderer wrote:
> 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.
>

    yes, consider it a config system with steroid ...


>> 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.
>

    Sorry, old DOS 8.3 file rules ....

>> 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, ...).
>

    I see what you mean. The uncommented printf are used or to print 
errors or to list profiles, I'll fix them



>>    . 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...
>

    You can have 2 or 3 setup, for examples one for interlace content 
and one for progressive, or one with some postprocessing enabled (for 
low bitrate avi) and one with no postprocessing (for DVD).

    Obvusly the number of profile for MPlayer will be a lot less than 
for MEncoder


>>    . 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.
>

    If we put profiles for various model of home divx/mpeg4/xvid player, 
each one with different capabilities we may have a lot of profiles, each 
one different for the maximum rate or the buffer size or the keyframe 
distance.


>> 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.
>
>

    You can obtain what you want with the -profile-file option, and you 
can use this in your config file.

    "-profile-file my_file," use my_file and, if the profile is not 
found, the standard one, if you use simply "-profil-file my_file" it 
looks only in my_file.

>> 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?
>
    The only reasonably way I found to exit form the program is to call 
the mencoder_exit function so I have to access to it from an external 
module (and that's why the profile is not avaible for MPlayer, there is 
everithing to support it in my patch,)

>> --- 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.

    Is to have the same file for all the user (and all languages). 
Otherwise instead of copying the profile file at install we need to 
process it and translate the description.

    In this way is, in my opinion, simple to handle .

>
>> +        # 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.
>

    Yes. These file is only to show how it work and to test the patch. 
The real one will be a little bit bigger and a little bit better.

>> +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.
>

    I'll Fix all.

>
>> +                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.
>

    Thanks.

       Daniele Forghieri