diff mbox

[FFmpeg-devel,18/20] mpeg12: Move finding the best frame rate to common code

Message ID 20170912234410.14093-19-sw@jkqxz.net
State New
Headers show

Commit Message

Mark Thompson Sept. 12, 2017, 11:44 p.m. UTC
Previously in the mpeg2_metadata filter.  Also adds a test.

(cherry picked from commit b5859e0b04bdbe12c97cb12ac10a45d51d2d73c9)
---
 libavcodec/Makefile                |  1 +
 libavcodec/mpeg12.h                |  4 ++
 libavcodec/mpeg12framerate.c       | 64 ++++++++++++++++++++++++++++
 libavcodec/mpeg2_metadata_bsf.c    | 58 +++----------------------
 libavcodec/tests/mpeg12framerate.c | 87 ++++++++++++++++++++++++++++++++++++++
 tests/fate/libavcodec.mak          |  5 +++
 6 files changed, 166 insertions(+), 53 deletions(-)
 create mode 100644 libavcodec/tests/mpeg12framerate.c

Comments

Michael Niedermayer Sept. 14, 2017, 12:48 a.m. UTC | #1
On Wed, Sep 13, 2017 at 12:44:08AM +0100, Mark Thompson wrote:
> Previously in the mpeg2_metadata filter.  Also adds a test.
[...]
> +    for (c = 1; c <= max_code; c++) {
> +        for (n = 1; n <= (mpeg2 ? 4 : 1); n++) {
> +            for (d = 1; d <= (mpeg2 ? 32 : 1); d++) {
> +                AVRational test, error;
> +                int cmp;
> +
> +                test = av_mul_q(ff_mpeg12_frame_rate_tab[c],
> +                                (AVRational) { n, d });
> +
> +                cmp = av_cmp_q(test, frame_rate);
> +                if (cmp == 0) {
> +                    best_c = c;
> +                    best_n = n;
> +                    best_d = d;
> +                    goto found;
> +                }
> +
> +                if (cmp < 0)
> +                    error = av_div_q(frame_rate, test);
> +                else
> +                    error = av_div_q(test, frame_rate);
> +
> +                cmp = av_cmp_q(error, best_error);
> +                if (cmp < 0 || (cmp == 0 && n == 1 && d == 1)) {
> +                    best_c = c;
> +                    best_n = n;
> +                    best_d = d;
> +                    best_error = error;
> +                }
> +            }
> +        }
> +    }
[...]
> -        for (i = 1; i < FF_ARRAY_ELEMS(frame_rate_table); i++) {
> -            AVRational fr, error;
> -            int n, d, cmp;
> -            for (n = 1; n <= 4; n++) {
> -                for (d = 1; d <= 32; d++) {
> -                    fr = av_mul_q(frame_rate_table[i],
> -                                  (AVRational) { n, d });
> -                    cmp = av_cmp_q(fr, ctx->frame_rate);
> -                    if (cmp == 0) {
> -                        code  = i;
> -                        ext_n = n;
> -                        ext_d = d;
> -                        goto found_frame_rate;
> -                    }
> -                    if (cmp < 0)
> -                        error = av_div_q(ctx->frame_rate, fr);
> -                    else
> -                        error = av_div_q(fr, ctx->frame_rate);
> -                    cmp = av_cmp_q(error, best_error);
> -                    if (cmp < 0 || (cmp == 0 && n == 1 && d == 1)) {
> -                        code  = i;
> -                        ext_n = n;
> -                        ext_d = d;
> -                        best_error = error;
> -                    }
> -                }
> -            }
> -        }

This doesnt just move the code, it also changes it
(this makes it harder to determine if the chnages are just cosmetic or
 not)

[...]

--
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you drop bombs on a foreign country and kill a hundred thousand
innocent people, expect your government to call the consequence
"unprovoked inhuman terrorist attacks" and use it to justify dropping
more bombs and killing more people. The technology changed, the idea is old.
Mark Thompson Sept. 14, 2017, 8:09 a.m. UTC | #2
On 14/09/17 01:48, Michael Niedermayer wrote:
> On Wed, Sep 13, 2017 at 12:44:08AM +0100, Mark Thompson wrote:
>> Previously in the mpeg2_metadata filter.  Also adds a test.
> [...]
>> +    for (c = 1; c <= max_code; c++) {
>> +        for (n = 1; n <= (mpeg2 ? 4 : 1); n++) {
>> +            for (d = 1; d <= (mpeg2 ? 32 : 1); d++) {
>> +                AVRational test, error;
>> +                int cmp;
>> +
>> +                test = av_mul_q(ff_mpeg12_frame_rate_tab[c],
>> +                                (AVRational) { n, d });
>> +
>> +                cmp = av_cmp_q(test, frame_rate);
>> +                if (cmp == 0) {
>> +                    best_c = c;
>> +                    best_n = n;
>> +                    best_d = d;
>> +                    goto found;
>> +                }
>> +
>> +                if (cmp < 0)
>> +                    error = av_div_q(frame_rate, test);
>> +                else
>> +                    error = av_div_q(test, frame_rate);
>> +
>> +                cmp = av_cmp_q(error, best_error);
>> +                if (cmp < 0 || (cmp == 0 && n == 1 && d == 1)) {
>> +                    best_c = c;
>> +                    best_n = n;
>> +                    best_d = d;
>> +                    best_error = error;
>> +                }
>> +            }
>> +        }
>> +    }
> [...]
>> -        for (i = 1; i < FF_ARRAY_ELEMS(frame_rate_table); i++) {
>> -            AVRational fr, error;
>> -            int n, d, cmp;
>> -            for (n = 1; n <= 4; n++) {
>> -                for (d = 1; d <= 32; d++) {
>> -                    fr = av_mul_q(frame_rate_table[i],
>> -                                  (AVRational) { n, d });
>> -                    cmp = av_cmp_q(fr, ctx->frame_rate);
>> -                    if (cmp == 0) {
>> -                        code  = i;
>> -                        ext_n = n;
>> -                        ext_d = d;
>> -                        goto found_frame_rate;
>> -                    }
>> -                    if (cmp < 0)
>> -                        error = av_div_q(ctx->frame_rate, fr);
>> -                    else
>> -                        error = av_div_q(fr, ctx->frame_rate);
>> -                    cmp = av_cmp_q(error, best_error);
>> -                    if (cmp < 0 || (cmp == 0 && n == 1 && d == 1)) {
>> -                        code  = i;
>> -                        ext_n = n;
>> -                        ext_d = d;
>> -                        best_error = error;
>> -                    }
>> -                }
>> -            }
>> -        }
> 
> This doesnt just move the code, it also changes it
> (this makes it harder to determine if the chnages are just cosmetic or
>  not)

I'll rearrange the patches so that the common part gets applied first and never changed.

- Mark
diff mbox

Patch

diff --git a/libavcodec/Makefile b/libavcodec/Makefile
index 135eb1751f..605dba2d25 100644
--- a/libavcodec/Makefile
+++ b/libavcodec/Makefile
@@ -1069,6 +1069,7 @@  TESTPROGS-$(CONFIG_GOLOMB)                += golomb
 TESTPROGS-$(CONFIG_IDCTDSP)               += dct
 TESTPROGS-$(CONFIG_IIRFILTER)             += iirfilter
 TESTPROGS-$(HAVE_MMX)                     += motion
+TESTPROGS-$(CONFIG_MPEGVIDEO)             += mpeg12framerate
 TESTPROGS-$(CONFIG_RANGECODER)            += rangecoder
 TESTPROGS-$(CONFIG_SNOW_ENCODER)          += snowenc
 
diff --git a/libavcodec/mpeg12.h b/libavcodec/mpeg12.h
index f551504b8c..1ec99f17e1 100644
--- a/libavcodec/mpeg12.h
+++ b/libavcodec/mpeg12.h
@@ -73,4 +73,8 @@  void ff_mpeg1_encode_mb(MpegEncContext *s, int16_t block[8][64],
 void ff_mpeg1_encode_init(MpegEncContext *s);
 void ff_mpeg1_encode_slice_header(MpegEncContext *s);
 
+void ff_mpeg12_find_best_frame_rate(AVRational frame_rate,
+                                    int *code, int *ext_n, int *ext_d,
+                                    int nonstandard);
+
 #endif /* AVCODEC_MPEG12_H */
diff --git a/libavcodec/mpeg12framerate.c b/libavcodec/mpeg12framerate.c
index 094cd180a5..ab3d351173 100644
--- a/libavcodec/mpeg12framerate.c
+++ b/libavcodec/mpeg12framerate.c
@@ -18,6 +18,9 @@ 
 
 #include "libavutil/rational.h"
 
+#include "mpeg12.h"
+#include "mpeg12data.h"
+
 const AVRational ff_mpeg12_frame_rate_tab[16] = {
     {    0,    0},
     {24000, 1001},
@@ -37,3 +40,64 @@  const AVRational ff_mpeg12_frame_rate_tab[16] = {
     {   15,    1},
     {    0,    0},
 };
+
+void ff_mpeg12_find_best_frame_rate(AVRational frame_rate,
+                                    int *code, int *ext_n, int *ext_d,
+                                    int nonstandard)
+{
+    int mpeg2 = ext_n && ext_d;
+    int max_code = nonstandard ? 12 : 8;
+    int c, n, d, best_c, best_n, best_d;
+    AVRational best_error = { INT_MAX, 1 };
+
+    // Default to NTSC if the inputs make no sense.
+    best_c = 4;
+    best_n = best_d = 1;
+
+    for (c = 1; c <= max_code; c++) {
+        if (av_cmp_q(frame_rate, ff_mpeg12_frame_rate_tab[c]) == 0) {
+            best_c = c;
+            goto found;
+        }
+    }
+
+    for (c = 1; c <= max_code; c++) {
+        for (n = 1; n <= (mpeg2 ? 4 : 1); n++) {
+            for (d = 1; d <= (mpeg2 ? 32 : 1); d++) {
+                AVRational test, error;
+                int cmp;
+
+                test = av_mul_q(ff_mpeg12_frame_rate_tab[c],
+                                (AVRational) { n, d });
+
+                cmp = av_cmp_q(test, frame_rate);
+                if (cmp == 0) {
+                    best_c = c;
+                    best_n = n;
+                    best_d = d;
+                    goto found;
+                }
+
+                if (cmp < 0)
+                    error = av_div_q(frame_rate, test);
+                else
+                    error = av_div_q(test, frame_rate);
+
+                cmp = av_cmp_q(error, best_error);
+                if (cmp < 0 || (cmp == 0 && n == 1 && d == 1)) {
+                    best_c = c;
+                    best_n = n;
+                    best_d = d;
+                    best_error = error;
+                }
+            }
+        }
+    }
+
+found:
+    *code = best_c;
+    if (mpeg2) {
+        *ext_n = best_n - 1;
+        *ext_d = best_d - 1;
+    }
+}
diff --git a/libavcodec/mpeg2_metadata_bsf.c b/libavcodec/mpeg2_metadata_bsf.c
index 9da8fd128d..4d33f29bf2 100644
--- a/libavcodec/mpeg2_metadata_bsf.c
+++ b/libavcodec/mpeg2_metadata_bsf.c
@@ -23,6 +23,7 @@ 
 #include "bsf.h"
 #include "cbs.h"
 #include "cbs_mpeg2.h"
+#include "mpeg12.h"
 
 typedef struct MPEG2MetadataContext {
     const AVClass *class;
@@ -99,63 +100,14 @@  static int mpeg2_metadata_update_fragment(AVBSFContext *bsf,
     }
 
     if (ctx->frame_rate.num && ctx->frame_rate.den) {
-        // Table 6-4.
-        static AVRational frame_rate_table[] = {
-            { 0, 0 },
-            { 24000, 1001 },
-            { 24,    1    },
-            { 25,    1    },
-            { 30000, 1001 },
-            { 30,    1    },
-            { 50,    1    },
-            { 60000, 1001 },
-            { 60,    1    },
-        };
         int code, ext_n, ext_d;
-        AVRational best_error = { INT_MAX, 1 };
-
-        for (i = 1; i < FF_ARRAY_ELEMS(frame_rate_table); i++) {
-            if (av_cmp_q(ctx->frame_rate, frame_rate_table[i]) == 0) {
-                code  = i;
-                ext_n = 1;
-                ext_d = 1;
-                goto found_frame_rate;
-            }
-        }
 
-        for (i = 1; i < FF_ARRAY_ELEMS(frame_rate_table); i++) {
-            AVRational fr, error;
-            int n, d, cmp;
-            for (n = 1; n <= 4; n++) {
-                for (d = 1; d <= 32; d++) {
-                    fr = av_mul_q(frame_rate_table[i],
-                                  (AVRational) { n, d });
-                    cmp = av_cmp_q(fr, ctx->frame_rate);
-                    if (cmp == 0) {
-                        code  = i;
-                        ext_n = n;
-                        ext_d = d;
-                        goto found_frame_rate;
-                    }
-                    if (cmp < 0)
-                        error = av_div_q(ctx->frame_rate, fr);
-                    else
-                        error = av_div_q(fr, ctx->frame_rate);
-                    cmp = av_cmp_q(error, best_error);
-                    if (cmp < 0 || (cmp == 0 && n == 1 && d == 1)) {
-                        code  = i;
-                        ext_n = n;
-                        ext_d = d;
-                        best_error = error;
-                    }
-                }
-            }
-        }
+        ff_mpeg12_find_best_frame_rate(ctx->frame_rate,
+                                       &code, &ext_n, &ext_d, 0);
 
-    found_frame_rate:
         sh->frame_rate_code        = code;
-        se->frame_rate_extension_n = ext_n - 1;
-        se->frame_rate_extension_d = ext_d - 1;
+        se->frame_rate_extension_n = ext_n;
+        se->frame_rate_extension_d = ext_d;
     }
 
     if (ctx->video_format             >= 0 ||
diff --git a/libavcodec/tests/mpeg12framerate.c b/libavcodec/tests/mpeg12framerate.c
new file mode 100644
index 0000000000..595bdb278a
--- /dev/null
+++ b/libavcodec/tests/mpeg12framerate.c
@@ -0,0 +1,87 @@ 
+/*
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include "libavcodec/mpeg12.h"
+#include "libavcodec/mpeg12data.h"
+
+int main(void)
+{
+    int i;
+
+#define TEST_MATCH(frame_rate, code, ext_n, ext_d) do { \
+        AVRational fr = frame_rate; \
+        int c, n, d; \
+        ff_mpeg12_find_best_frame_rate(fr, &c, &n, &d, 0); \
+        if (c != code || n != ext_n || d != ext_d) { \
+            av_log(NULL, AV_LOG_ERROR, "Failed to match %d/%d: " \
+                   "code = %d, ext_n = %d, ext_d = %d.\n", \
+                   fr.num, fr.den, c, n, d); \
+            return 1; \
+        } \
+    } while (0)
+#define TEST_EXACT(frn, frd) do { \
+        AVRational fr = (AVRational) { frn, frd }; \
+        int c, n, d; \
+        ff_mpeg12_find_best_frame_rate(fr, &c, &n, &d, 0); \
+        if (av_cmp_q(fr, av_mul_q(ff_mpeg12_frame_rate_tab[c], \
+                                  (AVRational) { n + 1, d + 1 })) != 0) { \
+            av_log(NULL, AV_LOG_ERROR, "Failed to find exact %d/%d: " \
+                   "code = %d, ext_n = %d, ext_d = %d.\n", \
+                   fr.num, fr.den, c, n, d); \
+            return 1; \
+        } \
+    } while (0)
+
+    // Framerates in the table must be chosen exactly.
+    for (i = 1; i <= 8; i++)
+        TEST_MATCH(ff_mpeg12_frame_rate_tab[i], i, 0, 0);
+
+    // As should the same ones with small perturbations.
+    // (1/1000 used here to be smaller than half the difference
+    // between 24 and 24000/1001.)
+    for (i = 1; i <= 8; i++) {
+        TEST_MATCH(av_sub_q(ff_mpeg12_frame_rate_tab[i],
+                            (AVRational) { 1, 1000 }), i, 0, 0);
+        TEST_MATCH(av_add_q(ff_mpeg12_frame_rate_tab[i],
+                            (AVRational) { 1, 1000 }), i, 0, 0);
+    }
+
+    // Exactly constructable framerates should be exact.  Note that some
+    // values can be made in multiple ways (e.g. 12 = 24 / 2 == 60 / 5),
+    // and there is no reason to favour any particular choice.
+    TEST_EXACT(     1,    1);
+    TEST_EXACT(     2,    1);
+    TEST_EXACT(    12,    1);
+    TEST_EXACT( 15000, 1001);
+    TEST_EXACT(    15,    1);
+    TEST_EXACT(   120,    1);
+    TEST_EXACT(120000, 1001);
+    TEST_EXACT(   200,    1);
+    TEST_EXACT(   240,    1);
+
+    // Values higher than 240 (the highest representable, as 60 * 4 / 1)
+    // should be mapped to 240.
+    for (i = 240; i < 1000; i += 10)
+        TEST_MATCH(((AVRational) { i, 1 }), 8, 3, 0);
+    // Values lower than 24000/32032 (the lowest representable, as
+    // 24000/1001 * 1 / 32) should be mapped to 24000/32032.
+    for (i = 74; i > 0; i--)
+        TEST_MATCH(((AVRational) { i, 100 }), 1, 0, 31);
+
+    return 0;
+}
diff --git a/tests/fate/libavcodec.mak b/tests/fate/libavcodec.mak
index fc8075c532..2d85849309 100644
--- a/tests/fate/libavcodec.mak
+++ b/tests/fate/libavcodec.mak
@@ -50,6 +50,11 @@  FATE_LIBAVCODEC-$(CONFIG_IIRFILTER) += fate-iirfilter
 fate-iirfilter: libavcodec/tests/iirfilter$(EXESUF)
 fate-iirfilter: CMD = run libavcodec/tests/iirfilter
 
+FATE_LIBAVCODEC-$(CONFIG_MPEGVIDEO) += fate-mpeg12framerate
+fate-mpeg12framerate: libavcodec/tests/mpeg12framerate$(EXESUF)
+fate-mpeg12framerate: CMD = run libavcodec/tests/mpeg12framerate
+fate-mpeg12framerate: REF = /dev/null
+
 FATE_LIBAVCODEC-yes += fate-libavcodec-options
 fate-libavcodec-options: libavcodec/tests/options$(EXESUF)
 fate-libavcodec-options: CMD = run libavcodec/tests/options