[FFmpeg-devel] [PATCHv4] lavf: add libopenmpt demuxer

[Jörn Heusipp]

Hello!

On 06/30/2016 02:49 AM, Josh de Kock wrote:
> Fixes ticket #5623
>
> TODO: bump lavf minor

(I am one of the libopenmpt maintainers)
We have been following the ffmpeg libopenmpt demuxer patches and we 
already gave feedback at https://bugs.openmpt.org/view.php?id=817 (for 
reference).

I'm addressing various concerns here. (As I am not replying to all these 
mails individually, I am CC-ing anyone who had commented on the matter.)

Regarding the original probing used in the v1 patch:
That would only check for (a subset of) MOD files, which themselves are 
only a very small subset of the module formats libopenmpt supports. That 
was unsuitable as probing for all libopenmpt supported module formats.

Regarding AVProbeData:
Looking at AVProbeData, I can see no (optional) field describing the 
file size:
typedef struct AVProbeData {
     const char *filename;
     unsigned char *buf; /**< Buffer must have AVPROBE_PADDING_SIZE of 
extra allocated bytes filled with zero. */
     int buf_size;       /**< Size of buf except extra allocated bytes */
     const char *mime_type; /**< mime_type, when known. */
} AVProbeData;
Sadly, that makes it rather useless for probing module formats: There 
are module formats which have no magic bytes at all, or very bad ones 
which require verifying other simple parts of the header in order to 
determine any meaningful probing result, some even require seeking 
through the file and verifying other later parts, some even have a 
footer that may need to be verified.
Because of this situation, the libopenmpt I/O layer absolutely needs to 
know the file size in order to do anything useful. In our adaption layer 
for streams (like stdin or HTTP without length information), we lazily 
pre-cache the whole file until hitting EOF as soon as the size is 
required or the code wants to seek to the end. As any kind of streaming 
does not apply to module formats, this was (and is) a sane design choice 
for libopenmpt.

Regarding probing performance:
Probing via the openmpt_could_open_propability (yeah, I know the typo in 
the API) uses file I/O callbacks with seeking functionality (if 
provided, otherwise it will pre-cache lazily when required, as described 
above) and will only actually issue I/O requests for those precise parts 
of the file which it needs (i.e. it performs the minimal required I/O). 
openmpt_could_open_propability does multiple memory allocations 
internally (if that is of concern with respect to performance).
As I may obviously be biased, I refrain from providing any performance 
numbers here. The only thing I can ensure is, that libopenmpt probing 
will obviously be slower than just comparing some few magic bytes inside 
AVProbeData.
In case the library user (ffmpeg demuxer in this case) cannot or does 
not want to provide the whole file data to the probing function, it 
absolutely MUST pretend that it would do so as documented here: 
https://buildbot.openmpt.org/builds/latest-unpacked/libopenmpt-docs/docs/group__libopenmpt__c.html#ga6b636df734a1caa154886ae0ceb33ad7 
(openmpt_could_open_propability()). However, as far as I can see, the 
implementation outlined there is not possible with the information 
AVProbeData provides (file size is missing).

Regarding separate opening of the file and only working with local files:
I understand the concerns and I do acknowledge that this is in fact a 
sub-optimal (if not outright bad) implementation for ffmpeg, as ffmpeg 
can also work with HTTP URLs and other file data sources. Duplicating a 
(stripped down) version of the libopenmpt module probing logic that 
works only with the prefix data provided by AVProbeData in the ffmpeg 
libopenmpt demuxer appears like a waste of effort and duplication of 
code to me (module format probing is rather complicated, the formats are 
all underdocumented and there are various strange corner cases one needs 
to consider (all of that is handled by libopenmpt)).

As I cannot see a feasible way to implement proper probing for ffmpeg 
with openmpt_could_open_propability(), and implementing probing in 
ffmpeg libopenmpt demuxer itself for all module formats is also not 
worth the effort, I think the best solution would (sadly) be using 
openmpt_get_supported_extensions() and verifying the file extension 
against that, and maybe providing better probing for local files only 
(if that is acceptable by ffmpeg policy) using the implementation that 
is in the latest patch. Only verifying the file extension would 
obviously be less reliable than actual data probing, but it will be 
better than nothing and will not slow down probing in any problematic way.

Regards,
Jörn