diff mbox

[FFmpeg-devel,PATCHv3] add signature filter for MPEG7 video signature

Message ID 4126592.Myufu8ebLk@gump
State Superseded
Headers show

Commit Message

Gerion Entrup Jan. 2, 2017, 10:52 p.m. UTC
Hi,

I made a new thread because of the delay between this mail and my last one. Attached is the next iteration of the patch (rebased to current master).

Main change is, that I've moved the writing and lookup code from uninit to request_frame (thank you for the suggestion). Please comment.

Kind regards,
Gerion

Comments

Moritz Barsnick Jan. 3, 2017, 10:33 a.m. UTC | #1
On Mon, Jan 02, 2017 at 23:52:58 +0100, Gerion Entrup wrote:
> +Calculates the MPEG-7 Video Signature. The filter could handle more than one
> +input. In this case the matching between the inputs could be calculated. The
> +filter passthrough the first input. The signature of each stream could be written
> +into a file.

"Could" means "k├Ânnte" in German (I assume that's you first language),
and that's probably not you intent here. I don't even understand what
you man. "Can" (and "could") implies optional. What does it really do?

You also need to fix some verbs. Language-wise, this could be correct,
but it still does not tell what really happens and what is optional:

  Calculates the MPEG-7 Video Signature. The filter can handle more
  than one input. In this case the matching between the inputs is
  calculated. The filter passes through the first input. The signature
  of each stream can be written into a file.

> +Calculate the mathing for the whole video and output whether the whole video
                 ^matching
> +Calculate as long as a matching is found or the video ends. Should be faster in
> +some cases.

"as long as" or "only until"? It sounds like you mean the latter.

> +@item filename
> +Set the path to which the output is written. If there is more than one input,
> +the path must be a prototype, i.e. must contain %d or %0nd (where n is a positive
> +integer), that will be replaced with the input number. If no filename is
> +specified, no output will be written. This is the default.

Question: Is this path technically (in ffmpeg terms) a URL/URI? If so,
that might need to be mentioned.

> +To detect whether two videos matches and store the signatures in XML format in
                                ^ match

> +typedef struct CourseSignature{

The English opposite of "fine" is "coarse", not "course". :)

> +typedef struct MatchingInfo{

Bracket style.

> +    int i,j,tmp_i,tmp_j,count;

Add some spaces, please.

> +    { "format",     "set output format, possible values: binary (default), xml",
                                           ^ possible values and default are
                                             automatically documented for you,
                                             see "ffmpeg -h filter=filtername".

> +    { "th_d",       "set threshold to detect one word as similar",
> +    { "th_dc",      "set threshold to detect all words as similar",
> +    { "th_xh",      "set threshold to detect frames as similar",

You can probably omit the word "set" here, that's what options are for
anyway.

> +    int x0,y0,x1,y1;

Spaces.

> +    }else if(x0-1 >= 0){
> +    }else if(y0-1 >= 0){
> +    }else{

Bracket style (spaces).

> +        /* return if unexspected error occurs in input stream */
                        ^ unexpected
> +        if (ret == AVERROR_EOF){

Bracket style.

Moritz
diff mbox

Patch

diff --git a/libavfilter/signature.h b/libavfilter/signature.h
index ed1c32e..195c94c 100644
--- a/libavfilter/signature.h
+++ b/libavfilter/signature.h
@@ -120,6 +120,8 @@  typedef struct {
     int coursecount; /* counter from 0 to 89 */
     int midcourse;   /* whether it is a coursesignature beginning from 45 + i * 90 */
     uint32_t lastindex; /* helper to store amount of frames */
+
+    int exported; /* boolean whether stream already exported */
 } StreamContext;
 
 typedef struct {
diff --git a/libavfilter/vf_signature.c b/libavfilter/vf_signature.c
index 2b51724..e1fff9b 100644
--- a/libavfilter/vf_signature.c
+++ b/libavfilter/vf_signature.c
@@ -71,6 +71,7 @@  AVFILTER_DEFINE_CLASS(signature);
 
 static int query_formats(AVFilterContext *ctx)
 {
+    /* all formats with a seperate gray value */
     static const enum AVPixelFormat pix_fmts[] = {
         AV_PIX_FMT_GRAY8,
         AV_PIX_FMT_YUV410P, AV_PIX_FMT_YUV411P,
@@ -586,21 +587,56 @@  static int export(AVFilterContext *ctx, StreamContext *sc, int input)
 static int request_frame(AVFilterLink *outlink)
 {
     AVFilterContext *ctx = outlink->src;
-    SignatureContext *sc = ctx->priv;
-    int i, ret;
+    SignatureContext *sic = ctx->priv;
+    StreamContext *sc, *sc2;
+    MatchingInfo match;
+    int i, j, ret;
+    int lookup = 1; /* indicates wheather EOF of all files is reached */
+
+    /* process all inputs */
+    for (i = 0; i < sic->nb_inputs; i++){
+        sc = &(sic->streamcontexts[i]);
 
-    for (i = 0; i < sc->nb_inputs; i++){
         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
+
+        /* return if unexspected error occurs in input stream */
         if (ret < 0 && ret != AVERROR_EOF)
             return ret;
+
+        /* export signature at EOF */
+        if (ret == AVERROR_EOF){
+            /* export if wanted */
+            if (strlen(sic->filename) > 0) {
+                if (export(ctx, sc, i) < 0)
+                    return ret;
+            }
+            sc->exported = 1;
+        }
+        lookup &= sc->exported;
+    }
+
+    /* signature lookup */
+    if (lookup && sic->mode != MODE_OFF) {
+        /* iterate over every pair */
+        for (i = 0; i < sic->nb_inputs; i++) {
+            sc = &(sic->streamcontexts[i]);
+            for (j = i+1; j < sic->nb_inputs; j++) {
+                sc2 = &(sic->streamcontexts[j]);
+                match = lookup_signatures(ctx, sic, sc, sc2, sic->mode);
+                if (match.score != 0) {
+                    av_log(ctx, AV_LOG_INFO, "matching of video %d at %f and %d at %f, %d frames matching\n",
+                            i, ((double) match.first->pts * sc->time_base.num) / sc->time_base.den,
+                            j, ((double) match.second->pts * sc2->time_base.num) / sc2->time_base.den,
+                            match.matchframes);
+                    if (match.whole)
+                        av_log(ctx, AV_LOG_INFO, "whole video matching\n");
+                } else {
+                    av_log(ctx, AV_LOG_INFO, "no matching of video %d and %d\n", i, j);
+                }
+            }
+        }
     }
+
     return ret;
 }
 
@@ -663,52 +699,14 @@  static av_cold int init(AVFilterContext *ctx)
 static av_cold void uninit(AVFilterContext *ctx)
 {
     SignatureContext *sic = ctx->priv;
-    StreamContext *sc, *sc2;
+    StreamContext *sc;
     void* tmp;
     FineSignature* finsig;
     CourseSignature* cousig;
-    MatchingInfo match;
-    int i,j;
-
-    //TODO export and especially lookup_signature can have a return value to show some error etc.
-    //How could this be handled in this function?
+    int i;
 
-    /* signature export */
-    if (strlen(sic->filename) > 0) {
-        for (i = 0; i < sic->nb_inputs; i++) {
-            /* if filter frame was called at least once */
-            if (&(sic->streamcontexts[i]) && (&(sic->streamcontexts[i]))->curfinesig)
-                export(ctx, &(sic->streamcontexts[i]), i);
-        }
-    }
-
-    /* signature lookup */
-    if (sic->mode != MODE_OFF) {
-        /* iterate over every pair */
-        for (i = 0; i < sic->nb_inputs; i++) {
-            sc = &(sic->streamcontexts[i]);
-            if (sc && sc->curfinesig) {
-                for (j = i+1; j < sic->nb_inputs; j++) {
-                    sc2 = &(sic->streamcontexts[j]);
-                    if (sc2 && sc2->curfinesig) {
-                        match = lookup_signatures(ctx, sic, sc, sc2, sic->mode);
-                        if (match.score != 0) {
-                            av_log(ctx, AV_LOG_INFO, "matching of video %d at %f and %d at %f, %d frames matching\n",
-                                    i, ((double) match.first->pts * sc->time_base.num) / sc->time_base.den,
-                                    j, ((double) match.second->pts * sc2->time_base.num) / sc2->time_base.den,
-                                    match.matchframes);
-                            if (match.whole)
-                                av_log(ctx, AV_LOG_INFO, "whole video matching\n");
-                        } else {
-                            av_log(ctx, AV_LOG_INFO, "no matching of video %d and %d\n", i, j);
-                        }
-                    }
-                }
-            }
-        }
-    }
 
-    /* cleanup */
+    /* free the lists */
     if (sic->streamcontexts != NULL) {
         for (i = 0; i < sic->nb_inputs; i++) {
             sc = &(sic->streamcontexts[i]);