On 10.02.2016 10:01, Paul B Mahol wrote:
On 2/10/16, Tobias Rapp <t.r...@noa-archive.com> wrote:
On 06.02.2016 23:30, Paul B Mahol wrote:
On 2/6/16, Paul B Mahol <one...@gmail.com> wrote:
Hi,

patch attached.


Improved version attached.

[...]
+
+static int string(const char *value1, const char *value2, size_t length)
+{
+    return !strncmp(value1, value2, length);
+}

If I understand correctly this function is used to compare if the start
of value2 matches value1. Maybe this function should be called
"starts_with" (also in the "function" option enum)?

+
+static int equal(const char *value1, const char *value2, size_t length)
+{
+    float f1, f2;
+
+    if (sscanf(value1, "%f", &f1) + sscanf(value2, "%f", &f2) != 2)
+        return 0;
+
+    return f1 != f2;
+}
+
+static int less(const char *value1, const char *value2, size_t length)
+{
+    float f1, f2;
+
+    if (sscanf(value1, "%f", &f1) + sscanf(value2, "%f", &f2) != 2)
+        return 0;
+
+    return f1 > f2;
+}
+
+static int greater(const char *value1, const char *value2, size_t length)
+{
+    float f1, f2;
+
+    if (sscanf(value1, "%f", &f1) + sscanf(value2, "%f", &f2) != 2)
+        return 0;
+
+    return f1 < f2;
+}
+
[...]

I think it would be better to not compare float values directly with
"==", "<" or ">". Instead use some code like "fabsf(f1 - f2) <= epsilon".

BTW: Is the return value of "equal", "less" and "greater" inverse on
purpose?

Not for equal, but for other it is.

OK, now I got it that the argument order is flipped but matches the documentation. In that case it is a bit counter-intuitive that one has to do something like

mode=print:key=mykey:value=1.0:function=less

to print all metadata values of "mykey" which are *bigger* than 1.0.


Another sidenote: I have seen that some filters use a common expression
language (e.g. aeval, crop, scale). Not sure if this expression language
supports string operators or if it is limited to numbers but in my
opinion it would be great to re-use it here.

Added and applied. Filter is easily extendable.

Thanks for adding expression support.

I know my response has come late and I'm only an occasional FFmpeg developer so my thoughts might not have big weight. Still I think it would have been more polite to first respond on the list to all my comments (see the "starts_with" suggestion above) before applying the patch.

Regards,
Tobias

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Reply via email to