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