[FFmpeg-devel] libavfilter/f_select: response file support

Submitted by Jonathan Gilbert on April 16, 2019, 5:14 p.m.

Details

Message ID CAPSOpYumig9yTEOAGg284UW=1HM_+z313T-F92_UEMNeApVHyA@mail.gmail.com
State New
Headers show

Commit Message

Jonathan Gilbert April 16, 2019, 5:14 p.m.
Hello :-)

I had a project recently where I wanted to externally specify a list
of specific frame numbers to drop. I understand this can be done with
select expressions like "not(eq(n,45)+eq(n,47)+eq(n,75))", but in my
case I wanted to drop nearly 16,000 frames, which would have required
a command-line over 250 KB in size. I chose a different approach: I
have added functionality to f_select.c so that you can specify the
list of frames you want to include/exclude in an external response
file. I have successfully used this code for my project, and thought I
might submit it up to for consideration. :-)

I've never submitted a patch in this format before, I hope I'm doing
this correctly.

Thanks very much,

Jonathan Gilbert

---
 libavfilter/f_select.c | 316 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 310 insertions(+), 6 deletions(-)

     ff_scene_sad_fn sad;            ///< Sum of the absolute
difference function (scene detect only)
@@ -158,6 +183,17 @@ typedef struct SelectContext {
 static const AVOption filt_name##_options[] = {                     \
     { "expr", "set an expression to use for selecting frames",
OFFSET(expr_str), AV_OPT_TYPE_STRING, { .str = "1" }, .flags=FLAGS },
\
     { "e",    "set an expression to use for selecting frames",
OFFSET(expr_str), AV_OPT_TYPE_STRING, { .str = "1" }, .flags=FLAGS },
\
+    { "file",     "set the name of a file from which frame numbers
are read", OFFSET(file_str), AV_OPT_TYPE_STRING, { .str = "" },
.flags=FLAGS }, \
+    { "filemode", "set the interpretation of the file (include or
exclude)", OFFSET(filemode), AV_OPT_TYPE_INT, { .i64 =
SELECT_FILEMODE_INCLUDE }, INT_MIN, INT_MAX, FLAGS, "filemode" }, \
+        { "include", "include all frames listed in the file", 0,
AV_OPT_TYPE_CONST, { .i64 = SELECT_FILEMODE_INCLUDE }, INT_MIN,
INT_MAX, FLAGS, "filemode" }, \
+        { "exclude", "exclude all frames listed in the file", 0,
AV_OPT_TYPE_CONST, { .i64 = SELECT_FILEMODE_EXCLUDE }, INT_MIN,
INT_MAX, FLAGS, "filemode" }, \
+    { "filetype", "set the interpretation of the file (frameno or
pts)", OFFSET(filetype), AV_OPT_TYPE_INT, { .i64 =
SELECT_FILETYPE_FRAMENO }, INT_MIN, INT_MAX, FLAGS, "filetype" }, \
+        { "frameno", "match frames by frame number", 0,
AV_OPT_TYPE_CONST, { .i64 = SELECT_FILETYPE_FRAMENO }, INT_MIN,
INT_MAX, FLAGS, "filetype" }, \
+        { "pts", "match frames by pts", 0, AV_OPT_TYPE_CONST, { .i64
= SELECT_FILETYPE_PTS }, INT_MIN, INT_MAX, FLAGS, "filetype" }, \
+    { "warnunused", "if enabled, warns if any of the frame numbers
from the input file are never matched", OFFSET(file_warn_unused),
AV_OPT_TYPE_INT, { .i64 = SELECT_WARNUNUSED_NO }, INT_MIN, INT_MAX,
FLAGS, "warnunused" }, \
+        { "no", "do not warn about unused frame numbers from the
input file", 0, AV_OPT_TYPE_CONST, { .i64 = SELECT_WARNUNUSED_NO },
INT_MIN, INT_MAX, FLAGS, "warnunused" }, \
+        { "yes", "warn about unused frame numbers from the input
file, but don't show a long list", 0, AV_OPT_TYPE_CONST, { .i64 =
SELECT_WARNUNUSED_YES }, INT_MIN, INT_MAX, FLAGS, "warnunused" }, \
+        { "all", "warn about all unused frame numbers from the input
file, even if there are many", 0, AV_OPT_TYPE_CONST, { .i64 =
SELECT_WARNUNUSED_ALL }, INT_MIN, INT_MAX, FLAGS, "warnunused" }, \
     { "outputs", "set the number of outputs", OFFSET(nb_outputs),
AV_OPT_TYPE_INT, {.i64 = 1}, 1, INT_MAX, .flags=FLAGS }, \
     { "n",       "set the number of outputs", OFFSET(nb_outputs),
AV_OPT_TYPE_INT, {.i64 = 1}, 1, INT_MAX, .flags=FLAGS }, \
     { NULL }                                                            \
@@ -165,17 +201,220 @@ static const AVOption filt_name##_options[] = {
                    \

 static int request_frame(AVFilterLink *outlink);

+static int read_line(FILE *file, char **buffer, int *buffer_length)
+{
+    int ch;
+    int offset = 0;
+
+    while (1) {
+        ch = fgetc(file);
+
+        if (ch == EOF) {
+            int actual_eof = feof(file);
+
+            if (actual_eof) {
+                if (offset == 0)
+                    return 0;
+                else
+                    break;
+            }
+        }
+
+        if (ch == '\n')
+            break;
+
+        if (ch == '\r') {
+            ch = fgetc(file);
+
+            if ((ch != EOF) && (ch != '\n'))
+                ungetc(ch, file);
+
+            break;
+        }
+
+        (*buffer)[offset++] = ch;
+
+        if (offset >= *buffer_length) {
+            char *new_buffer;
+
+            *buffer_length += 2 + *buffer_length / 2;
+            new_buffer = (char *)realloc(*buffer, *buffer_length);
+
+            if (new_buffer == NULL)
+                return AVERROR(ENOMEM);
+
+            *buffer = new_buffer;
+        }
+    }
+
+    (*buffer)[offset] = '\0';
+    return 1;
+}
+
+static void strip_string_whitespace(char *str)
+{
+    char *from = str;
+    char *to = str;
+
+    while (isspace(*from))
+        from++;
+
+    if (!*from)
+        *to = '\0';
+    else if (from == to) {
+        while (*str && !isspace(*str))
+            str++;
+
+        *str = '\0';
+    }
+    else {
+        while (*from && !isspace(*from))
+            *to++ = *from++;
+
+        *to = '\0';
+    }
+}
+
+static int read_frame_list_from_file(AVFilterContext *ctx, char
*filename, int **list_ptr, int *list_length_ptr)
+{
+    FILE *file;
+    int buffer_length;
+    char *buffer;
+    int list_length;
+    int list_capacity;
+    int *list;
+
+    file = fopen(filename, "r");
+
+    if (!file) {
+        int ret = AVERROR(errno);
+        av_log(ctx, AV_LOG_ERROR, "Cannot open file '%s' for reading:
%s\n", filename, av_err2str(ret));
+        return ret;
+    }
+
+    buffer_length = 40;
+    buffer = malloc(buffer_length);
+
+    list_capacity = 20;
+    list = (int *)malloc(list_capacity * sizeof(int));
+
+    if ((buffer == NULL) || (list == NULL)) {
+        int ret = AVERROR(ENOMEM);
+        av_log(ctx, AV_LOG_ERROR, "Error reading from file '%s':
%s\n", filename, av_err2str(ret));
+        return ret;
+    }
+
+    list_length = 0;
+
+    while (1) {
+        char *parse_ptr;
+        int parsed;
+
+        int ret = read_line(file, &buffer, &buffer_length);
+
+        if (ret < 0) {
+            av_log(ctx, AV_LOG_ERROR, "Error reading from file '%s':
%s\n", filename, av_err2str(ret));
+            free(buffer);
+            free(list);
+            return ret;
+        }
+
+        if (ret == 0)
+            break;
+
+        strip_string_whitespace(buffer);
+
+        if (*buffer == '\0')
+            continue;
+
+        parsed = strtol(buffer, &parse_ptr, 10);
+
+        if ((*parse_ptr != '\0') || (parsed < 0)) {
+            av_log(ctx, AV_LOG_ERROR, "Couldn't parse frame number:
'%s'\n", buffer);
+            free(buffer);
+            free(list);
+            return AVERROR(EINVAL);
+        }
+
+        list[list_length] = parsed;
+        list_length++;
+
+        if (list_length == list_capacity) {
+            int *new_list;
+
+            list_capacity += list_capacity / 2;
+            new_list = (int *)realloc(list, list_capacity * sizeof(int));
+
+            if (new_list == NULL) {
+                int ret = AVERROR(ENOMEM);
+                av_log(ctx, AV_LOG_ERROR, "Error reading from file
'%s': %s\n", filename, av_err2str(ret));
+                free(buffer);
+                free(list);
+                return ret;
+            }
+
+            list = new_list;
+        }
+    }
+
+    *list_ptr = list;
+    *list_length_ptr = list_length;
+
+    free(buffer);
+
+    return 0;
+}
+
+static int compare_integers(const void *a_ptr, const void *b_ptr)
+{
+    int a = *(int *)a_ptr;
+    int b = *(int *)b_ptr;
+
+    return a - b;
+}
+
 static av_cold int init(AVFilterContext *ctx)
 {
     SelectContext *select = ctx->priv;
     int i, ret;

-    if ((ret = av_expr_parse(&select->expr, select->expr_str,
-                             var_names, NULL, NULL, NULL, NULL, 0, ctx)) < 0) {
-        av_log(ctx, AV_LOG_ERROR, "Error while parsing expression '%s'\n",
-               select->expr_str);
-        return ret;
+    select->file_frame_list = NULL;
+    select->file_frame_list_used = NULL;
+
+    if (select->file_str && strlen(select->file_str)) {
+        if (select->expr_str && strlen(select->expr_str) &&
strcmp(select->expr_str, "1")) {
+            av_log(ctx, AV_LOG_ERROR, "Cannot specify both a file and
an expression for the select filter\n");
+            return AVERROR(EINVAL);
+        }
+
+        ret = read_frame_list_from_file(ctx, select->file_str,
&select->file_frame_list, &select->file_frame_list_length);
+
+        if (ret < 0)
+            return ret;
+
+        av_log(ctx, AV_LOG_DEBUG, "Read %d frames from frame list
file\n", select->file_frame_list_length);
+
+        qsort(select->file_frame_list,
select->file_frame_list_length, sizeof(select->file_frame_list[0]),
compare_integers);
+
+        for (i = 0; i < select->file_frame_list_length; i++)
+            av_log(ctx, AV_LOG_DEBUG, "[%d] = %d\n", i,
select->file_frame_list[i]);
+
+        if (select->file_warn_unused != SELECT_WARNUNUSED_NO) {
+            av_log(ctx, AV_LOG_DEBUG, "Setting up frame used flags\n");
+
+            select->file_frame_list_used = (char
*)malloc(select->file_frame_list_length * sizeof(char));
+            memset(select->file_frame_list_used, 0,
select->file_frame_list_length * sizeof(char));
+        }
     }
+    else {
+        if ((ret = av_expr_parse(&select->expr, select->expr_str,
+                                 var_names, NULL, NULL, NULL, NULL,
0, ctx)) < 0) {
+            av_log(ctx, AV_LOG_ERROR, "Error while parsing expression '%s'\n",
+                   select->expr_str);
+            return ret;
+        }
+    }
+
     select->do_scene_detect = !!strstr(select->expr_str, "scene");

     for (i = 0; i < select->nb_outputs; i++) {
@@ -298,7 +537,7 @@ static double get_concatdec_select(AVFrame *frame,
int64_t pts)
 #define D2TS(d)  (isnan(d) ? AV_NOPTS_VALUE : (int64_t)(d))
 #define TS2D(ts) ((ts) == AV_NOPTS_VALUE ? NAN : (double)(ts))

-static void select_frame(AVFilterContext *ctx, AVFrame *frame)
+static void select_frame_expr(AVFilterContext *ctx, AVFrame *frame)
 {
     SelectContext *select = ctx->priv;
     AVFilterLink *inlink = ctx->inputs[0];
@@ -382,6 +621,47 @@ static void select_frame(AVFilterContext *ctx,
AVFrame *frame)
     select->var_values[VAR_PREV_T]   = select->var_values[VAR_T];
 }

+static void select_frame_list(AVFilterContext *ctx, AVFrame *frame)
+{
+    SelectContext *select = ctx->priv;
+    AVFilterLink *inlink = ctx->inputs[0];
+    int search_key = -1;
+
+    av_log(inlink->dst, AV_LOG_DEBUG, "checking a frame");
+
+    if (select->filetype == SELECT_FILETYPE_FRAMENO)
+        search_key = inlink->frame_count_out;
+    else if (select->filetype == SELECT_FILETYPE_PTS)
+        search_key = TS2D(frame->pts);
+
+    int *search_result = (int *)bsearch(&search_key,
select->file_frame_list, select->file_frame_list_length,
sizeof(select->file_frame_list[0]), compare_integers);
+
+    if (select->file_frame_list_used != NULL) {
+        int index = search_result - select->file_frame_list;
+
+        if ((index >= 0) && (index < select->file_frame_list_length))
+            select->file_frame_list_used[index] = 1;
+    }
+
+    int include_frame = (search_result != NULL) ^ (select->filemode
== SELECT_FILEMODE_EXCLUDE);
+
+    select->select = include_frame ? 1 : 0;
+    select->select_out = 0;
+    // TODO: consider supporting multiple outputs with response files?
+
+    av_log(inlink->dst, AV_LOG_DEBUG, " -> search_key:%d
select_out:%d\n", search_key, select->select_out);
+}
+
+static void select_frame(AVFilterContext *ctx, AVFrame *frame)
+{
+    SelectContext *select = ctx->priv;
+
+    if (select->file_frame_list == NULL)
+        select_frame_expr(ctx, frame);
+    else
+        select_frame_list(ctx, frame);
+}
+
 static int filter_frame(AVFilterLink *inlink, AVFrame *frame)
 {
     AVFilterContext *ctx = inlink->dst;
@@ -407,6 +687,8 @@ static av_cold void uninit(AVFilterContext *ctx)
     SelectContext *select = ctx->priv;
     int i;

+    av_log(ctx, AV_LOG_DEBUG, "Uninitializing\n");
+
     av_expr_free(select->expr);
     select->expr = NULL;

@@ -416,6 +698,28 @@ static av_cold void uninit(AVFilterContext *ctx)
     if (select->do_scene_detect) {
         av_frame_free(&select->prev_picref);
     }
+
+    if ((select->file_warn_unused != SELECT_WARNUNUSED_NO) &&
(select->file_frame_list_used != NULL)) {
+        int unused_count = 0;
+        int i;
+
+        for (i = 0; i < select->file_frame_list_length; i++)
+            if (!select->file_frame_list_used[i])
+                unused_count++;
+
+        av_log(ctx, AV_LOG_ERROR, "Select filter: %d frames indicated
in response file were not matched\n", unused_count);
+
+        if ((unused_count <= 15) || (select->file_warn_unused ==
SELECT_WARNUNUSED_ALL)) {
+            for (i = 0; i < select->file_frame_list_length; i++)
+                if (!select->file_frame_list_used[i])
+                    av_log(ctx, AV_LOG_ERROR, "=> %d\n",
select->file_frame_list[i]);
+        }
+
+        free(select->file_frame_list_used);
+    }
+
+    if (select->file_frame_list != NULL)
+        free(select->file_frame_list);
 }

 #if CONFIG_ASELECT_FILTER

Comments

Michael Niedermayer April 16, 2019, 10:48 p.m.
On Tue, Apr 16, 2019 at 12:14:13PM -0500, Jonathan Gilbert wrote:
> Hello :-)
> 
> I had a project recently where I wanted to externally specify a list
> of specific frame numbers to drop. I understand this can be done with
> select expressions like "not(eq(n,45)+eq(n,47)+eq(n,75))", but in my
> case I wanted to drop nearly 16,000 frames, which would have required
> a command-line over 250 KB in size. I chose a different approach: I
> have added functionality to f_select.c so that you can specify the
> list of frames you want to include/exclude in an external response
> file. I have successfully used this code for my project, and thought I
> might submit it up to for consideration. :-)
> 
> I've never submitted a patch in this format before, I hope I'm doing
> this correctly.
> 
> Thanks very much,
> 
> Jonathan Gilbert

Applying: libavfilter/f_select: response file support
error: corrupt patch at line 51
error: could not build fake ancestor

please make sure your MUA doesnt wrap long lines or attach the patch

[...]
Carl Eugen Hoyos April 16, 2019, 10:50 p.m.
2019-04-16 19:14 GMT+02:00, Jonathan Gilbert <logic@deltaq.org>:
> Hello :-)
>
> I had a project recently where I wanted to externally specify a list
> of specific frame numbers to drop. I understand this can be done with
> select expressions like "not(eq(n,45)+eq(n,47)+eq(n,75))", but in my
> case I wanted to drop nearly 16,000 frames, which would have required
> a command-line over 250 KB in size. I chose a different approach: I
> have added functionality to f_select.c so that you can specify the
> list of frames you want to include/exclude in an external response
> file. I have successfully used this code for my project, and thought I
> might submit it up to for consideration. :-)
>
> I've never submitted a patch in this format before, I hope I'm doing
> this correctly.

This is definitely not the only issue:
Instead of using malloc() and friends, please see libavutil/mem.h
for the appropriate functions.

Carl Eugen
Jonathan Gilbert April 17, 2019, 9:18 a.m.
I've changed the allocation functions and attached the output of `git
format-patch` to this e-mail.

Thanks,

Jonathan Gilbert

On Tue, Apr 16, 2019 at 5:50 PM Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> 2019-04-16 19:14 GMT+02:00, Jonathan Gilbert <logic@deltaq.org>:
> > Hello :-)
> >
> > I had a project recently where I wanted to externally specify a list
> > of specific frame numbers to drop. I understand this can be done with
> > select expressions like "not(eq(n,45)+eq(n,47)+eq(n,75))", but in my
> > case I wanted to drop nearly 16,000 frames, which would have required
> > a command-line over 250 KB in size. I chose a different approach: I
> > have added functionality to f_select.c so that you can specify the
> > list of frames you want to include/exclude in an external response
> > file. I have successfully used this code for my project, and thought I
> > might submit it up to for consideration. :-)
> >
> > I've never submitted a patch in this format before, I hope I'm doing
> > this correctly.
>
> This is definitely not the only issue:
> Instead of using malloc() and friends, please see libavutil/mem.h
> for the appropriate functions.
>
> Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Jonathan Gilbert April 29, 2019, 8:18 p.m.
Ping! :-)

Thanks,

Jonathan Gilbert

On Wed, Apr 17, 2019 at 4:18 AM Jonathan Gilbert <logic@deltaq.org> wrote:

> I've changed the allocation functions and attached the output of `git
> format-patch` to this e-mail.
>
> Thanks,
>
> Jonathan Gilbert
>
> On Tue, Apr 16, 2019 at 5:50 PM Carl Eugen Hoyos <ceffmpeg@gmail.com>
> wrote:
>
>> 2019-04-16 19:14 GMT+02:00, Jonathan Gilbert <logic@deltaq.org>:
>> > Hello :-)
>> >
>> > I had a project recently where I wanted to externally specify a list
>> > of specific frame numbers to drop. I understand this can be done with
>> > select expressions like "not(eq(n,45)+eq(n,47)+eq(n,75))", but in my
>> > case I wanted to drop nearly 16,000 frames, which would have required
>> > a command-line over 250 KB in size. I chose a different approach: I
>> > have added functionality to f_select.c so that you can specify the
>> > list of frames you want to include/exclude in an external response
>> > file. I have successfully used this code for my project, and thought I
>> > might submit it up to for consideration. :-)
>> >
>> > I've never submitted a patch in this format before, I hope I'm doing
>> > this correctly.
>>
>> This is definitely not the only issue:
>> Instead of using malloc() and friends, please see libavutil/mem.h
>> for the appropriate functions.
>>
>> Carl Eugen
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
>

Patch hide | download patch | download mbox

diff --git a/libavfilter/f_select.c b/libavfilter/f_select.c
index 1132375758..0852fffb44 100644
--- a/libavfilter/f_select.c
+++ b/libavfilter/f_select.c
@@ -22,6 +22,8 @@ 
  * @file
  * filter for selecting which frame passes in the filterchain
  */
+#include <stdio.h>
+#include <ctype.h>

 #include "libavutil/avstring.h"
 #include "libavutil/eval.h"
@@ -139,10 +141,33 @@  enum var_name {
     VAR_VARS_NB
 };

+enum filemode {
+    SELECT_FILEMODE_INCLUDE,
+    SELECT_FILEMODE_EXCLUDE
+};
+
+enum filetype {
+    SELECT_FILETYPE_FRAMENO,
+    SELECT_FILETYPE_PTS
+};
+
+enum warnunused {
+    SELECT_WARNUNUSED_NO,
+    SELECT_WARNUNUSED_YES,
+    SELECT_WARNUNUSED_ALL
+};
+
 typedef struct SelectContext {
     const AVClass *class;
     char *expr_str;
     AVExpr *expr;
+    char *file_str;
+    int *file_frame_list;
+    int file_frame_list_length;
+    enum filemode filemode;
+    enum filetype filetype;
+    char *file_frame_list_used;
+    enum warnunused file_warn_unused;
     double var_values[VAR_VARS_NB];
     int do_scene_detect;            ///< 1 if the expression requires
scene detection variables, 0 otherwise