[FFmpeg-devel] [PATCHv2] add signature filter for MPEG7 video signature

[Moritz Barsnick]

Just some random observations from me.

> BTW:
> ./ffmpeg -i ~/test2.mkv -vf signature=xml:filename=signature.xml -f null -
> fails because xml is not a valid parameter.
> ./ffmpeg -i ~/test2.mkv -vf signature=xml:filename=signature.xml:detectmode=full -f null -
> fails not, but write binary to the output file. Why?

I think "format" is not the first parameter the filter accepts, and
you're passing "xml" as an unnamed parameter. (Not sure why it's
accepted in the second case though.) signature=format=xml:...

> + at item detectmode
> +Enable or disable the matching process.
> +or 1 (to enable). Optionally you can set the mode to 2. Then the detection ends,

There's a line or sentence missing. Furthermore, it's unclear which
symbolic values (described below) those numbers relate to.

> +Set the number of inputs. The option value must be a non negative interger.
                                                                     ^^^^^^^^ integer 

> +Set the path to witch the output is written. If there is more than one input,
                   ^^^^^ which

> +To calculate the signature of an input video and store it in signature.bin:
> + at example
> +ffmpeg -i input.mkv -vf signature=filename=signature.bin -map 0:v -c rawvideo -f null -
> + at end example
> +
> + at item
> +To detect whether two videos matches and store the signatures in XML format in
> +signature0.xml and signature1.xml:
> + at example
> +ffmpeg -i input1.mkv -i input2.mkv -filter_complex "[0:0][1:0] signature=nb_inputs=2:detectmode=full:format=xml:filename=signature%d.xml" -map 0:v -map 1:v -c rawvideo -f null -

When muxing to null, you shouldn't need to specify the rawvideo codec.
It doesn't affect your filter anyway, and ffmpeg recently defaults to
wrapped_avframe, which is faster. I think you don't even need to "-map
0:v -map 1:v", the complex filter gets its "[0:0][1:0]" inputs anyway.
Any if you need both, you should conistently stick to ":0" or ":v"
stream specifiers, because you mean the exact same one in both cases.


> + * calculates the jaccard distance and evaluate a pair of course signatures as good
                                          ^^^^^^^^ evaluates

> + * compares framesignatures and sort out signatures with a l1 distance over a given threshold.
                                   ^^^^ sorts                             ^^^^ above


> +    /* The following calculate a summed area table (intpic) and brings the numbers
                                ^calculates
> +     * in intpic to to the same denuminator.
                    ^^^^^ to       ^^^^^^^^^^^ denominator
> +     * So you only have to handle the numinator in the following sections.
                                         ^^^^^^^^^ numerator
> +    dh1 = inlink->h/32;
> +    if (inlink->h%32)

Shouldn't there be whitespace around the operators?

> +    if (!f) {
> +        av_log(ctx, AV_LOG_ERROR, "cannot open xml file %s\n", filename);

How about strerror(errno) additionally?

> +    if (!f) {
> +        av_log(ctx, AV_LOG_ERROR, "cannot open file %s\n", filename);

Ditto.

> +        ret = ff_request_frame(ctx->inputs[i]);
> +        // TODO handle this in a better way?
> +        // Problem is the following:
> +        // Assuming two inputs, inputA with 50 frames, inputB with 100 frames
> +        // simply returning ret when < 0 would result in not filtering inputB
> +        // after 50 frames anymore, not wanted
> +        // only returning ret at the end would result in only respecting the error
> +        // values of the last input, please comment

Dies this still need to be resolved?

> +                        if(match.score != 0){
Whitespace style nit:
                           if (match.score != 0) {
Same everywhere else. You have quite inconsistent use of whitespace for
"if"s, "while"s and '{'s.


Cheers,
Moritz