[FFmpeg-devel] [PATCH v2 4/4] avformat/libopenmpt: Probe file format from file data if possible

[Jörn Heusipp]

On 01/10/2018 12:55 PM, Carl Eugen Hoyos wrote:
> 2018-01-10 12:18 GMT+01:00 Jörn Heusipp <osmanx at problemloesungsmaschine.de>:

>> When building with libopenmpt 0.3, use the libopenmpt file header
>> probing functions for probing. libopenmpt probing functions are
>> allocation-free and designed to be as fast as possible.
>>
>> For libopenmpt 0.2, or when libopenmpt 0.3 file header probing cannot
>> probe successfully due to too small probe buffer, test the filename
>> against the file extensions supported by the libopenmpt library that
>> is actually linked, instead of relying on a hard-coded file extension
>> list. File extension testing is also allocation-free and designed to
>> be fast in libopenmpt. Avoiding a hard-coded file extension list is
>> useful because later libopenmpt versions will likely add support for
>> more module file formats.
>>
>> libopenmpt file header probing is tested regularly against the FATE
>> suite and other diverse file collections by libopenmpt upstream in
>> order to avoid false positives.
>>
>> FATE passes with './configure --enable-libopenmpt' as well as with
>> './configure --enable-libopenmpt --enable-libmodplug'.
>>
>> libopenmpt probing adds about 5%..10% cpu time (depending on precise
>> usage pattern and host CPU and compiler version used for libopenmpt)
>> compared to all current internal FFmpeg probing functions combined in
>> tools/probetest for all of its module formats combined (currently 41
>> modules formats in libopenmpt 0.3.4 and 234 file formats in FFmpeg).
>>
>> Signed-off-by: Jörn Heusipp <osmanx at problemloesungsmaschine.de>
>> ---
>>   libavformat/libopenmpt.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 57 insertions(+)
>>
>> diff --git a/libavformat/libopenmpt.c b/libavformat/libopenmpt.c
>> index 5efbdc4..42b78fb 100644
>> --- a/libavformat/libopenmpt.c
>> +++ b/libavformat/libopenmpt.c
>> @@ -218,6 +218,62 @@ static int read_seek_openmpt(AVFormatContext *s, int stream_idx, int64_t ts, int
>>       return 0;
>>   }
>>
>> +static int probe_openmpt_extension(AVProbeData *p)
>> +{
>> +    const char *ext;
>> +    if (p->filename) {
>> +        ext = strrchr(p->filename, '.');
>> +        if (ext && strlen(ext + 1) > 0) {
>> +            ext++;  /* skip '.' */
>> +            if (openmpt_is_extension_supported(ext) == 1)
>> +                return AVPROBE_SCORE_EXTENSION;
>> +        }
>> +    }
>> +    return 0;
>> +}
>> +
>> +static int read_probe_openmpt(AVProbeData *p)
>> +{
>> +#if OPENMPT_API_VERSION_AT_LEAST(0,3,0)
>> +    int probe_result;
>> +    if (p->buf && p->buf_size > 0) {
>> +        probe_result = openmpt_probe_file_header_without_filesize(
>> +                           OPENMPT_PROBE_FILE_HEADER_FLAGS_DEFAULT,
>> +                           p->buf, p->buf_size,
>> +                           &openmpt_logfunc, NULL, NULL, NULL, NULL, NULL);
>> +        if (probe_result == OPENMPT_PROBE_FILE_HEADER_RESULT_SUCCESS) {
>> +            /* As probing here relies on code external to FFmpeg, do not return
>> +             * AVPROBE_SCORE_MAX in order to reduce the impact in the rare
>> +             * cases of false positives.
>> +             */
>> +            return AVPROBE_SCORE_MIME + 1;
>> +        } else if (probe_result == OPENMPT_PROBE_FILE_HEADER_RESULT_WANTMOREDATA) {
> 
>> +            if (probe_openmpt_extension(p) > 0) {
>> +                return AVPROBE_SCORE_EXTENSION;
> 
> I don't think this is a good idea because it means that a high score is
> reported for WANTMOREDATA.

That is what the first patch did also, however much more convoluted and 
not easy to spot.
In any case, I agree, that does not appear like that good of an idea. 
Will change to AVPROBE_SCORE_RETRY with the next iteration of the patch.


Regards,
Jörn