[FFmpeg-devel] [PATCH] libopenmpt: add subsong support

[Jörn Heusipp]

On 07/18/2016 02:59 PM, Josh de Kock wrote:

>   libavformat/libopenmpt.c | 8 ++++++++
>   1 file changed, 8 insertions(+)


> +    {"subsong",     "set subsong",        OFFSET(subsong),     AV_OPT_TYPE_INT,            {.i64 = 0},                       0,   1000,      A|D},

  1.  -1 is a valid subsong index you can specify to libopenmpt, meaning 
all subsongs consecutively. Later you substract 1 to adjust for that 
which totally confused me here.
  2.  Also, the there is no limit imposed on the number of subsongs, 
although in practice it will of course almost always be rater low.
  3.  As documented at 
https://lib.openmpt.org/doc/group__libopenmpt__c.html#ga523fe9d172709472049941899c9835f2 
, the default subsong (which future libopenmpt versions may select with 
smart heuristics or which may be specified by the module file) may be 
something else than 0 or -1. In case the user wants to use just this 
default subsong, you should not call openmpt_module_get_num_subsongs() 
at all.

Thus, maybe something like:

{"subsong",     "set subsong",        OFFSET(subsong), 
AV_OPT_TYPE_INT,            {.i64 = -2},                       -2, 
INT_MAX,      A|D},

with -2 meaning "let libopenmpt choose".


> +    int subsong = (openmpt_module_get_num_subsongs(openmpt->module) < openmpt->subsong ? 0 : openmpt->subsong);
> +    openmpt_module_select_subsong(openmpt->module, subsong-1);

Does it actually provide any benefit to offset the subsong indices by +1 
compared to what libopenmpt uses? This will confuse users who happen to 
know libopenmpt or openmpt123.


> +    snprintf(str, sizeof(str), "%d", subsong);
> +    av_dict_set(&s->metadata, "track", str, 0);

I'm not sure whether setting "track" metadata always makes sense here 
semantically, but I don't mind if you want to keep it. Maybe it would 
make sense to duplicate the track title as album title in that case.

We also have openmpt_module_get_subsong_name(), although I don't think 
there are that many actual modules in the wild which provide useful text 
here.

Just setting "track" is the most conservative solution, I guess.


In any case, all combined, this maybe should look something like:

if (openmpt->subsong >=
     openmpt_module_get_num_subsongs(openmpt->module))
     openmpt->subsong = -2;
if (openmpt->subsong != -2) {
     if (openmpt->subsong >= 0) {
         snprintf(str, sizeof(str), "%d", openmpt->subsong+1);
         av_dict_set(&s->metadata, "track", str, 0);
     }
     openmpt_module_select_subsong(openmpt->module, subsong);
}

or, if you want want to keep counting subsongs from 1, with 0 meaning 
"all" and -1 meaning "libopenmpt default":

if (openmpt->subsong >
     openmpt_module_get_num_subsongs(openmpt->module))
     openmpt->subsong = -1;
if (openmpt->subsong != -1) {
     if (openmpt->subsong >= 1) {
         snprintf(str, sizeof(str), "%d", openmpt->subsong);
         av_dict_set(&s->metadata, "track", str, 0);
     }
     openmpt_module_select_subsong(openmpt->module, subsong-1);
}


I would prefer the first variant in order to stay consistent with 
libopenmpt. Notice that I would sill offset "track" compared to subsong 
index in this case, as "track" 0 for the first one would be confusing as 
well, compared to common use of "track" number metadata.


Regards,
Jörn