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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
