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

[Carl Eugen Hoyos]

2018-01-06 11:07 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.

You could also test tools/probetest

> FATE passes with './configure --enable-libopenmpt' as well as with
> './configure --enable-libopenmpt --enable-libmodplug'.
>
> As expected, I did not see any measurable performance difference
> caused by libopenmpt file header probing when compared to the previous
> pure file extension based format probing (using the following
> synthetic test program (which tries to do nothing but exercise file
> probing) on the complete FATE suite).
>
>     // find ../fate/ -type f | xargs --no-run-if-empty ./probetest
>     #include <stdio.h>
>     #include "libavformat/avformat.h"
>     #define BUFSIZE 2048
>     static char buf[BUFSIZE + AVPROBE_PADDING_SIZE];
>     int main(int argc, const char * * argv) {
>         av_log_set_level(AV_LOG_WARNING);
>         av_register_all();
>         for (int i = 1; i < argc; ++i) {
>             AVProbeData pd;
>             FILE * f;
>             size_t size;
>             memset(&pd, 0, sizeof(AVProbeData));
>             pd.filename = argv[i];
>             memset(buf, 0, sizeof(buf));
>             f = fopen(pd.filename, "rb");
>             size = fread(buf, 1, BUFSIZE, f);
>             fclose(f);
>             pd.buf_size = size;
>             av_probe_input_format(&pd, 1);
>         }
>         return 0;
>     }
>
> Signed-off-by: Jörn Heusipp <osmanx at problemloesungsmaschine.de>
> ---
>  libavformat/libopenmpt.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
>
> diff --git a/libavformat/libopenmpt.c b/libavformat/libopenmpt.c
> index 5efbdc4..a663b2e 100644
> --- a/libavformat/libopenmpt.c
> +++ b/libavformat/libopenmpt.c
> @@ -218,6 +218,64 @@ static int read_seek_openmpt(AVFormatContext *s, int stream_idx, int64_t ts, int
>      return 0;
>  }
>
> +static int read_probe_openmpt(AVProbeData * p)
> +{
> +    const int score_data      = AVPROBE_SCORE_MIME + 1;   /* 76 */
> +    const int score_ext       = AVPROBE_SCORE_EXTENSION;  /* 50 */
> +    const int score_ext_retry = AVPROBE_SCORE_RETRY;      /* 25 */
> +    const int score_retry     = AVPROBE_SCORE_RETRY / 2;  /* 12 */
> +    const int score_fail      = 0;                        /*  0 */
> +
> +    const char *ext;
> +    int probe_result;
> +    int score = score_fail;
> +
> +    if (p->filename) {
> +        ext = strrchr(p->filename, '.');
> +        if (ext && strlen(ext + 1) > 0) {
> +            ext++;  /* skip '.' */
> +            if (openmpt_is_extension_supported(ext) == 1)
> +                score = FFMAX(score, score_ext);
> +        }
> +    }
> +
> +#if OPENMPT_API_VERSION_AT_LEAST(0,3,0)
> +    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_FAILURE) {
> +            score = score_fail;

What's wrong with return 0;?

> +        } else if (probe_result == OPENMPT_PROBE_FILE_HEADER_RESULT_SUCCESS) {
> +            score = FFMAX(score, score_data);

What does OPENMPT_PROBE_FILE_HEADER_RESULT_SUCCESS mean?
Why not return MAX?

> +        } else if (probe_result == OPENMPT_PROBE_FILE_HEADER_RESULT_WANTMOREDATA) {

I believe this should return 0 but maybe you found that this is bad?

> +            if (score > score_fail) {
> +                /* known file extension */
> +                score = FFMAX(score, score_ext_retry);
> +            } else {
> +                /* unknown file extension */
> +                if (p->buf_size >= openmpt_probe_file_header_get_recommended_size()) {
> +                    /* We have already received the recommended amount of data
> +                     * and still cannot decide. Return a rather low score.
> +                     */
> +                    score = FFMAX(score, score_retry);
> +                } else {
> +                    /* The file extension is unknown and we have very few data
> +                     * bytes available. libopenmpt cannot decide anything here,
> +                     * and returning any score > 0 would result in successfull
> +                     * probing of random data.
> +                     */
> +                    score = score_fail;

This patch indicates that it may be a good idea to require libopenmpt 0.3,
when was it released, which distributions do not include it?

Carl Eugen