I'm not exactly sure what kind of information you need; The mono/stereo decoding functions should be obvious, and the quad decoding function renders front-left/front-right/rear-left/rear-right audio.
In case this was part of your confusion, get_num_channels has nothing to do with this as it merely returns the number of pattern channels. libopenmpt does not give you a separate output stream for each pattern channel, you just get the final mono/stereo/quad mix containing all rendered channels.

Note that most modules do not make use of quad surround, so the rear channels will remain silent on them, and the absolute majority of people who use "surround" effects in modules use them with a stereo setup in mind (it's a bit complicated to explain but I can elaborate if you want), so my recommendation is to always render in stereo by default, and maybe provide an option to the user to switch to quad surround, but certainly don't make it the default. It will mess up many modules.

I would also recommend sticking with stereo channel layout unless the library user explicitly demands mono or quad. Even when the sink at the end is a 5.1 setup, you almost certainly don't want to negotiate quad surround at the libopenmpt level (I have no idea how format negotiation works in ffmpeg internally, so this chain of reasoning might not actually be applicable there at all). Even mono should probably only be used when explicitly requested, as the panning law used by some (all?) module formats is not an equal power panning law, which means relative volume of panned voices will differ in mono output relative to stereo output.

Ah right. Thanks! There was some confusion but this seems to clear everything up.

This can probably be closed

No problem. Feel free to report back if you have more questions.

Illya: One more thing... I found the ffmpeg mailing list thread where this was discussed, so here's some additional information: I think the old libmodplug demuxer reported the channel count as some kind metadata, and I think so should the new libopenmpt demuxer. I.e. don't use openmpt_module_get_num_channels for decoding but provide the information to the user as metadata (maybe together with get_num_samples, get_num_instruments, get_num_patterns, etc).

Also, probing has been brought up in the thread. Please don't implement your own magic byte search but rather use openmpt_could_open_propability with an effort of 0.25. This will (as far as applicable) only verify the module header for validity.

I've since gotten the probing working instantly with 1.0 effort by letting libopenmpt access the file directly, I'll look into adding channels as metadata as well (and how modplug did it). I will also be switching both the probe, and the demuxer over to a proper stream-based architecture in the future.

Also, on deprecating modplug, are you still for this?

<blockquote>I've since gotten the probing working instantly with 1.0 effort by letting libopenmpt access the file directly</blockquote>
If you want the probing to be (much) faster but allow for a (mostly very low) chace of getting false positives, I'd definitely go with an effort of 0.25. I guess it's a design decision that is up to you.

<blockquote>Also, on deprecating modplug, are you still for this?</blockquote>
Absolutely. libmodplug is inferior to libopenmpt in pretty much any way: Not actively maintained, less platforms supported, less module formats supported, it supports them less well and has many other drawbacks like not being entirely threadsafe (loading/playing multiple modules in several threads).

Quoting http://thread.gmane.org/gmane.comp.video.ffmpeg.devel/215367/focus=215710 and http://thread.gmane.org/gmane.comp.video.ffmpeg.devel/215367/focus=215722 here:

<blockquote>Can you extend this list of extensions? libmodplug has the following monster:
669,abc,amf,ams,dbm,dmf,dsm,far,it,mdl,med,mid,mod,mt2,mtm,okt,psm,ptm,s3m,stm,ult,umx,xm,itgz,itr,itz,mdgz,mdr,mdz,s3gz,s3r,s3z,xmgz,xmr,xmz
Does libopenmpt documentation (or source) provide such a list?</blockquote>
libopenmpt actually does provide such a list, albeit not at compile-time because the list is subject to change as new formats get added to libopenmpt (which will never change the public API). See https://buildbot.openmpt.org/builds/latest-unpacked/libopenmpt-docs/docs/group__libopenmpt__c.html#ga6fff8e3cf426e01e6434d084233c43c5 (<pre>openmpt_get_supported_extensions()</pre>). If ffmpeg does require a compile-time known list (I did not investigate that), your v3 patch is of course fine. Otherwise, using openmpt_get_supported_extensions() should be preferred.

<blockquote>

• double ret = openmpt_could_open_propability(openmpt_stream_get_file_callbacks(), file, 1.0, openmpt_logfunc, NULL);
• fclose(file);
• return ret ? ret AVPROBE_SCORE_MAX : 0;
  So apparently this breaks FATE as it detects some random mp3 and others as
  supported. You probably need to lower it down a lot, something like ret
  AVPROBE_SCORE_EXTENSION/2 or something.
  </blockquote>
  I cannot judge what the target level of that score should be for ffmpeg as I am not that familiar with the probe scoring it is using. However the libopenmpt part here seems suboptimal:
  openmpt_could_open_propability() with effort=1.0 will completely load the module file, which is exactly the same as would happen if you used openmpt_module_create(), i.e. rather slow and unsuitable for file type probing. As far as I understand ffmpeg probing, which I think is expected to only just have a quick look at the file, you probably should be using effort 0.25. Also, the exact return value of openmpt_could_open_propability() remains unspecified. I would suggest the following algorithm:
  <pre>
  double ret;
  ret = openmpt_could_open_propability(openmpt_stream_get_file_callbacks(), file, 0.25, openmpt_logfunc, NULL);
  fclose(file);
  if (ret >= 0.5)
  ret = 0.5;
  ret = ret 2.0;
  return ret WHATEVER_FFMPEG_DEVELOPERS_CONSIDER_A_USEFUL_MAX_SCORE_FOR_LIBOPENMPT.
  </pre>
  Note that I suggest clamping the returned probability at 0.5 and scaling back to 1.0 by multiplying with 2.0. Effort 0.25 will currently return 0.6 at max, but that is an implementation detail and subject to change when we improve format detection logic. Using 0.5 as the threshold here seems sane to me.
  Clamping and scaling is useful as libopenmpt will NEVER return 1.0 as a result here unless you specify effort=1.0, which would be slow as outlined above.
  This is no ultimate suggestion as probing module formats is rather difficult (some have no or only very bad file magic numbers). Ultimately, this probably needs to be tuned relative to the probing score of other ffmpeg demuxers by your demuxer.

In addition, it would be nice to see which random MP3 files are detected (most likely as Ultimate SoundTracker mods), since if course we'd always like to improve our heuristics.

<blockquote>If ffmpeg does require a compile-time known list (I did not investigate that), your v3 patch is of course fine. Otherwise, using openmpt_get_supported_extensions() should be preferred,</blockquote>
And when updating libopenmpt, please regularly check back on our website or changelog as new file formats may be added every now and then.

<blockquote>In addition, it would be nice to see which random MP3 files are detected (most likely as Ultimate SoundTracker mods), since if course we'd always like to improve our heuristics.</blockquote>
I have added a few heuristics to format detection since the last official libopenmpt release to reject more random binary files (including SNES SPC and MP3s with ID3v2), so by updating to the latest version from SVN, these problems may already be fixed.

Also, besides the channel/pattern/etc. count, you may extract more interesting metadata by using openmpt_module_get_metadata, openmpt_module_get_subsong_name, openmpt_module_get_instrument_name, openmpt_module_get_sample_name and related functions.

Also, looking at the patch, it looks like you are using 44100/int16 as the output format. Can you please consider using 48000/float32 as the default instead (unless ffmpeg has some policy demanding 44100/int16 as default)?
I just added documentation stating this recommendation at https://buildbot.openmpt.org/builds/latest-unpacked/libopenmpt-docs/docs/libopenmpt_c_overview.html#libopenmpt_c_outputformat .

Digging further into the proposed patch...

if (ret < 1 || (openmpt->duration < openmpt_module_get_position_seconds(openmpt->module)))

This is a very bad idea. You should really just check for "ret < 1", because:
1) The pre-calculated duration is just an estimate. In the best case it is off by a few milliseconds, in the worst case it can be off by a big factor, simply because the duration of some modules cannot be estimated correctly without getting stuck in an infinite loop.
2) In case the module does not exploit an infinite pattern loop (most modules don't), ret is always going to be 0 at some point anyway.

In addition, I think you would be dropping the last rendered block because you check the duration after rendering, not before, so I assume that the just rendered block would not be played.

Unless FFmpeg demuxers must guarantee that the decoded stream must stop at some point (but I cannot imagine that they do, otherwise internet streams would not work by definition), I would propose to simply check for "ret < 1" and nothing else.

FFmpeg recommends that there is a low cut-off filter to set low scores to 0 for the probe function as well as a lot of demuxers return low scores, and this mucks up probing overall.

https://my.mixtape.moe/riddjz.aea (fate-samples/atrac1/chirp_tone_10-16000.aea)
Is one of the files which is tripping up the probe function, in that it is returning an absurdly high score (I haven't been able to get the exact score yet but I know it's between 0.6 and 0.7) and I havent been able to reliably filter it out.

This is ideally what I'd like to use (or something like this) for probing

    ret = openmpt_could_open_propability(openmpt_stream_get_file_callbacks(), file, 0.25, openmpt_logfunc, NULL);
    fclose(file);

    if (ret < 0.25)
        ret = 0;
    if (ret > 0.5)
        ret = 0.5;
    ret *= 2.0;

    return llrint(ret * AVPROBE_SCORE_EXTENSION);

What do you guys think? I'm speaking with people at FFmpeg to see if I can improve the atrac probing function as well as a sort of "meet-in-the-middle" fix for this.

You can get the latest iteration of the FFmpeg patch here (if you'd like to test it yourselves): http://sprunge.us/cKUG

<blockquote>FFmpeg recommends that there is a low cut-off filter to set low scores to 0 for the probe function as well as a lot of demuxers return low scores, and this mucks up probing overall.
This is ideally what I'd like to use (or something like this) for probing

    ret = openmpt_could_open_propability(openmpt_stream_get_file_callbacks(), file, 0.25, openmpt_logfunc, NULL);
    fclose(file);

    if (ret < 0.25)
        ret = 0;
    if (ret > 0.5)
        ret = 0.5;
    ret *= 2.0;

    return llrint(ret * AVPROBE_SCORE_EXTENSION);

</blockquote>

That in general would also look sane to me. Note however, that it will (currently) be equivalent to what I suggested as libopenmpt (currently) does not differentiate different scores for probing. The API was deliberately designed to allow for such an improvement later on, but it remains unimplemented as of now. This however is an implementation detail/defect in libopenmpt which will, I guess, not change anytime soon. Rejecting low scores right out is, I guess, a way to make your ffmpeg demuxer kind of future-proof in case some libopenmpt in the future actually weightens the probing scores internally. Currently it would not hurt, but also not gain anything for you. 0.25 appears fine as a threshold to use in this case.

<blockquote>https://my.mixtape.moe/riddjz.aea Is one of the files which is tripping up the probe function, in that it is returning an absurdly high score (I haven't been able to get the exact score yet but I know it's between 0.6 and 0.7) and I havent been able to reliably filter it out.</blockquote>
libopenmpt as of r6531 will already reject that file, so you probably just need to update your libopenmpt copy. If ffmpeg maintainers want to see you relying on released libopenmpt versions only, We could even make a libopenmpt release with that fix included (I consider ffmpeg support important enough for libopenmpt to warrant an extra bugfix-release in that case).
However, keep in mind (you, and ffmpeg in general) that, by relying on libopenmpt for probing may always cause probing problems or conflicts in potentially future versions. There is not much that can be done there as far as I can see. Reporting false-positive probing here in the bug tracker is certainly a way to tackle this in the long run, but it is never an instant fix on the ffmpeg side. If ffmpeg has some probing score threshold above which ffmpeg core demuxers should be reasonably certain that the file is actually supported by them, the maximum libopenmpt score should probably be just right below that threshold as ffmpeg has no direct control of the probing used by libopenmpt internally.
Seeing

#define AVPROBE_SCORE_EXTENSION  50 /// score for file extension
#define AVPROBE_SCORE_MIME       75 /// score for file mime type
#define AVPROBE_SCORE_MAX       100 /// maximum score

in avformat.h I am guessing that such a threshold could be between in [76..99]. libopenmpt probing is certainly better than looking at just file extension. At least it is supposed to be ;-). If that mime-type in question here is coming from some HTTP server, I can also see arguments for scoring libopenmpt somewhere in [51..74]. This is for ffmpeg developers to judge.

(I did not look at your updated patch yet.)

+    st->codecpar->codec_type  = AVMEDIA_TYPE_AUDIO;
+    st->codecpar->codec_id    = AV_NE(AV_CODEC_ID_PCM_F32BE, AV_CODEC_ID_PCM_F32LE);
+    st->codecpar->channels    = openmpt->channels;
+    st->codecpar->sample_rate = openmpt->sample_rate;
+
[...]
+
+#define AUDIO_PKT_SIZE 2048
+
+static int read_packet_openmpt(AVFormatContext *s, AVPacket *pkt)
+{
+    OpenMPTContext *openmpt = s->priv_data;
+    int n_samples = AUDIO_PKT_SIZE / (openmpt->channels ? openmpt->channels*4 : 4);
+    int ret;
+
+    if ((ret = av_new_packet(pkt, AUDIO_PKT_SIZE)) < 0)
+        return ret;
+
+    switch (openmpt->channels) {
+    case 1:
+        ret = openmpt_module_read_mono(openmpt->module, openmpt->sample_rate,
+                                       n_samples, (short *)pkt->data);
+        break;
+    case 2:
+        ret = openmpt_module_read_interleaved_stereo(openmpt->module, openmpt->sample_rate,
+                                                    n_samples, (short *)pkt->data);
+        break;
+    case 4:
+        ret = openmpt_module_read_interleaved_quad(openmpt->module, openmpt->sample_rate,
+                                                   n_samples, (short *)pkt->data);
+        break;
+    default:
+        av_log(s, AV_LOG_ERROR, "Unsupported number of channels: %d", openmpt->channels);
+        return AVERROR(EINVAL);
+    }
+
+    if (ret < 1)
+        return AVERROR_EOF;
+
+    pkt->size = AUDIO_PKT_SIZE;
+
+    return 0;
+}

That does not look quite right to me.

<ol>

<li>You are specifying the ffmpeg sample format as float32 (AV_NE(AV_CODEC_ID_PCM_F32BE, AV_CODEC_ID_PCM_F32LE)), but are using the int16 libopenmpt rendering functions:
I think, that should be ret = openmpt_module_read_interleaved_float_stereo(openmpt->module, openmpt->sample_rate, n_samples, (float*)pkt->data); instead of ret = openmpt_module_read_interleaved_stereo(openmpt->module, openmpt->sample_rate, n_samples, (short *)pkt->data); . Mono and quad accordingly.
Size calculation is correct for float.
</li>

<li>The end of stream case still will not work correctly:
The last call to openmpt_module_read_FOO before end of stream may/will return a length shorter than what you provided as buffer length:
I.e.:

if (ret < 1) {
 pkt->size = 0;
 return AVERROR_EOF;
}
pkt->size = ret * (openmpt->channels * 4);

instead of

+    if (ret < 1)
+        return AVERROR_EOF;
+
+    pkt->size = AUDIO_PKT_SIZE;

</li>

</ol>

Illya: Thanks for the updated patch. In addition to what manx noticed, are you planning to add sub song support? From a quick look at the GME demuxer I noticed that FFmpeg appears to have support for more than one song per file, so hopefully this would be easy to add to this plugin as well. Look into openmpt_module_get_num_subsongs and openmpt_module_select_subsong for more information.
A file for testing this feature can be found e.g. at http://files.exotica.org.uk/modland/?file=pub/modules/Impulsetracker/Siren/deus%20ex%20-%20hong%20kong%20streets.it

Manx:
Thanks not sure how I missed that. (int->float stuff)

If ffmpeg maintainers want to see you relying on released libopenmpt versions only, We could
even make a libopenmpt release with that fix included (I consider ffmpeg support important >
enough for libopenmpt to warrant an extra bugfix-release in that case).
This would probably be best, this would mean that we could set a minimum libopenmpt version to avoid people using versions which are before the fix, and then complaining :)

However, keep in mind (you, and ffmpeg in general) that, by relying on libopenmpt for probing
may always cause probing problems or conflicts in potentially future versions.

I'm not sure how else we could do this, I can obviously change the probing logic on the ffmpeg side if stuff changes in the future, but I dont think anyone (or me) in FFmpeg wants to particularly implement a probe function for that many files at this point in time.

Saga Musix:

are you planning to add sub song support?

Currently not for the first version of the decoder, I just want to get this done, but within a week or so of the first version getting merged (which is dependent on if the probing doesn't probe any files accidentally in the test suite) I'll definitely try to put this in.

p.s. where can I find the text formatting for this bugtracker?

<blockquote>p.s. where can I find the text formatting for this bugtracker?</blockquote>
Mantis supports a subset of HTML tags, in our case the list of supported tags is p, li, ul, ol, br, pre, i, b, u, em, strong, blockquote, code, del, s.

<blockquote>
I'm not sure how else we could do this, I can obviously change the probing logic on the ffmpeg side if stuff changes in the future, but I dont think anyone (or me) in FFmpeg wants to particularly implement a probe function for that many files at this point in time.
</blockquote>
I also think it is best to let libopenmpt handle the probing. Duplicating functionality is of no use to anyone here. I just wanted to point out that probing conflicts are inherently to be expected when mixing 2 different probing infrastructures that are not (and cannot for all cases) be adjusted/tuned to one another.

(bugtracker formatting is just simple html, valid tags: p, li, ul, ol, br, pre, i, b, u, em, strong, blockquote, code, del, s)

I'm testing with the latest snapshot (version 0.2.6556) and it seems there's just one more test which is failing due to the probe, the sample can be found at https://my.mixtape.moe/xxsveq.cin or in the fate sample directory (fate-samples/idcin/idlog-2MB.cin).

Thanks for the sample, I'll fix it ASAP.

Fixed in r6557.

Awesome! checking out svn now (autotools src autobuild doesnt seem to be up yet), will get back to you in a few. If it does indeed pass all the tests then a release would be much appreciated

We're currently planning a combined OpenMPT/libopenmpt update for next weekend (not this one).

Note: libopenmpt beta18 has been released today.

Did you miss my reply on ffmpeg-devel regarding memory leaks and error codes (see https://ffmpeg.org/pipermail/ffmpeg-devel/2016-July/196461.html )?
https://ffmpeg.org/pipermail/ffmpeg-devel/2016-July/196496.html still contains those problems.

manx: I did actually miss that email, I've fixed it now, and the first version of the demuxer has been merged.

Removal of the libmodplug demuxer depends on libopenmpt's availability in distros, and will be blocked until most major distros have libopenmpt in their repos.

I'm currently working on adding support for subsongs

Thanks for the effort! I'll close this issue as the basic functionality is now there. manx will discuss some remaining issues over the ffmpeg mailing list, and if there turn up any new issues on our side, feel free to open a new issue.