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

[Gerion Entrup]

On Dienstag, 19. April 2016 13:25:53 CEST Moritz Barsnick wrote:
> 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.
The whole sentence is crap (was meaningful with former revisions). Have forgotten
to delete it.


> 
> > +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.
I think with map it should be faster because the framework does not decode
the audio and the subtitles. Don't know abount :v, have updated it.

> 
> 
> > + * 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?
I have added it (taken from the psnr filter mainly).

> 
> > +    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?
Yes.

> 
> > +                        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.
Thank you for the typos and the whitespace errors. They should be fixed
now.

Gerion

-------------- next part --------------
A non-text attachment was scrubbed...
Name: diff-to-last-revision.diff
Type: text/x-patch
Size: 40047 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160419/892b31f1/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-add-signature-filter-for-MPEG7-video-signature.patch
Type: text/x-patch
Size: 82249 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20160419/892b31f1/attachment-0001.bin>