diff mbox series

[FFmpeg-devel,1/2] ffmpeg: add display_{rotation, hflip, vflip} options

Message ID 20220518180454.53301-1-jeebjp@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/2] ffmpeg: add display_{rotation, hflip, vflip} options | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Jan Ekström May 18, 2022, 6:04 p.m. UTC
This enables overriding the rotation as well as horizontal/vertical
flip state of a specific video stream on either the input or output
side.

Additionally, switch the singular test that was utilizing the rotation
metadata to instead override the input display rotation, thus leading
to the same result.
---
 doc/ffmpeg.texi             | 29 +++++++++++++++++++++
 fftools/ffmpeg.h            |  6 +++++
 fftools/ffmpeg_opt.c        | 52 +++++++++++++++++++++++++++++++++++++
 tests/fate/filter-video.mak |  2 +-
 4 files changed, 88 insertions(+), 1 deletion(-)

Comments

Gyan Doshi May 19, 2022, 4:21 a.m. UTC | #1
On 2022-05-18 11:34 pm, Jan Ekström wrote:
> This enables overriding the rotation as well as horizontal/vertical
> flip state of a specific video stream on either the input or output
> side.

Since rotation, flip and scale are stored in a single data structure, 
how about a single option i.e. -display "rotate=90,flip=hv,scale=2"

Regards,
Gyan

>
> Additionally, switch the singular test that was utilizing the rotation
> metadata to instead override the input display rotation, thus leading
> to the same result.
> ---
>   doc/ffmpeg.texi             | 29 +++++++++++++++++++++
>   fftools/ffmpeg.h            |  6 +++++
>   fftools/ffmpeg_opt.c        | 52 +++++++++++++++++++++++++++++++++++++
>   tests/fate/filter-video.mak |  2 +-
>   4 files changed, 88 insertions(+), 1 deletion(-)
>
> diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi
> index 0d7e1a479d..f44d863f02 100644
> --- a/doc/ffmpeg.texi
> +++ b/doc/ffmpeg.texi
> @@ -897,6 +897,35 @@ If used together with @option{-vcodec copy}, it will affect the aspect ratio
>   stored at container level, but not the aspect ratio stored in encoded
>   frames, if it exists.
>   
> +@item -display_rotation[:@var{stream_specifier}] @var{rotation} (@emph{input/output,per-stream})
> +Set the video display rotation in degrees specified by @var{rotation}.
> +
> +@var{rotation} is a floating point number that describes a pure
> +counter-clockwise rotation in degrees.
> +
> +When setting this for input streams, @code{-autorotate} logic will be affected.
> +For additional parameters affecting display matrix side data into which this
> +information is saved, see @code{-display_hflip} and @code{-display_vflip}.
> +
> +These options work as a unit, so if only one of them is set, then the display
> +matrix will be overridden to that specific value with the rest being set to
> +default values.
> +
> +If unset, the default value if a display matrix is being defined is a rotation
> +of zero degrees.
> +
> +@item -display_hflip[:@var{stream_specifier}] (@emph{input/output,per-stream})
> +Set whether on display the image should be horizontally flipped.
> +
> +If unset, the default value if a display matrix is being defined is that there
> +is no additional horizontal flip. See @code{-display_rotation}.
> +
> +@item -display_vflip[:@var{stream_specifier}] (@emph{input/output,per-stream})
> +Set whether on display the image should be vertically flipped.
> +
> +If unset, the default value if a display matrix is being defined is that there
> +is no additional vertical flip. See @code{-display_rotation}.
> +
>   @item -vn (@emph{input/output})
>   As an input option, blocks all video streams of a file from being filtered or
>   being automatically selected or mapped for any output. See @code{-discard}
> diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h
> index 9f0c093e34..0da01b0668 100644
> --- a/fftools/ffmpeg.h
> +++ b/fftools/ffmpeg.h
> @@ -180,6 +180,12 @@ typedef struct OptionsContext {
>       int        nb_force_fps;
>       SpecifierOpt *frame_aspect_ratios;
>       int        nb_frame_aspect_ratios;
> +    SpecifierOpt *display_rotations;
> +    int        nb_display_rotations;
> +    SpecifierOpt *display_hflips;
> +    int        nb_display_hflips;
> +    SpecifierOpt *display_vflips;
> +    int        nb_display_vflips;
>       SpecifierOpt *rc_overrides;
>       int        nb_rc_overrides;
>       SpecifierOpt *intra_matrices;
> diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
> index 47e8b9b7bd..0d1c84d6df 100644
> --- a/fftools/ffmpeg_opt.c
> +++ b/fftools/ffmpeg_opt.c
> @@ -20,6 +20,7 @@
>   
>   #include "config.h"
>   
> +#include <float.h>
>   #include <stdint.h>
>   
>   #if HAVE_SYS_RESOURCE_H
> @@ -43,6 +44,7 @@
>   #include "libavutil/avutil.h"
>   #include "libavutil/bprint.h"
>   #include "libavutil/channel_layout.h"
> +#include "libavutil/display.h"
>   #include "libavutil/intreadwrite.h"
>   #include "libavutil/fifo.h"
>   #include "libavutil/mathematics.h"
> @@ -82,6 +84,9 @@ static const char *const opt_name_qscale[]                    = {"q", "qscale",
>   static const char *const opt_name_forced_key_frames[]         = {"forced_key_frames", NULL};
>   static const char *const opt_name_force_fps[]                 = {"force_fps", NULL};
>   static const char *const opt_name_frame_aspect_ratios[]       = {"aspect", NULL};
> +static const char *const opt_name_display_rotations[]         = {"display_rotation", NULL};
> +static const char *const opt_name_display_hflips[]            = {"display_hflip", NULL};
> +static const char *const opt_name_display_vflips[]            = {"display_vflip", NULL};
>   static const char *const opt_name_rc_overrides[]              = {"rc_override", NULL};
>   static const char *const opt_name_intra_matrices[]            = {"intra_matrix", NULL};
>   static const char *const opt_name_inter_matrices[]            = {"inter_matrix", NULL};
> @@ -739,6 +744,39 @@ static int opt_recording_timestamp(void *optctx, const char *opt, const char *ar
>       return 0;
>   }
>   
> +static void add_display_matrix_to_stream(OptionsContext *o,
> +                                         AVFormatContext *ctx, AVStream *st)
> +{
> +    double display_rotation = DBL_MAX;
> +    int hflip = -1, vflip = -1,
> +        hflip_set = 0, vflip_set = 0, display_rotation_set = 0;
> +    uint8_t *buf = NULL;
> +
> +    MATCH_PER_STREAM_OPT(display_rotations, dbl, display_rotation, ctx, st);
> +    MATCH_PER_STREAM_OPT(display_hflips,    i,   hflip,            ctx, st);
> +    MATCH_PER_STREAM_OPT(display_vflips,    i,   vflip,            ctx, st);
> +
> +    display_rotation_set = display_rotation != DBL_MAX;
> +    hflip_set            = hflip != -1;
> +    vflip_set            = vflip != -1;
> +
> +    if (!display_rotation_set && !hflip_set && !vflip_set)
> +        return;
> +
> +    if (!(buf = av_stream_new_side_data(st, AV_PKT_DATA_DISPLAYMATRIX,
> +                                        sizeof(int32_t) * 9))) {
> +        av_log(NULL, AV_LOG_FATAL, "Failed to generate a display matrix!\n");
> +        exit_program(1);
> +    }
> +
> +    av_display_rotation_set((int32_t *)buf,
> +                            display_rotation_set ? -display_rotation :
> +                                                   -0.0f);
> +    av_display_matrix_flip((int32_t *)buf,
> +                           hflip_set ? hflip : 0,
> +                           vflip_set ? vflip : 0);
> +}
> +
>   static const AVCodec *find_codec_or_die(const char *name, enum AVMediaType type, int encoder)
>   {
>       const AVCodecDescriptor *desc;
> @@ -893,6 +931,8 @@ static void add_input_streams(OptionsContext *o, AVFormatContext *ic)
>               ist->top_field_first = -1;
>               MATCH_PER_STREAM_OPT(top_field_first, i, ist->top_field_first, ic, st);
>   
> +            add_display_matrix_to_stream(o, ic, st);
> +
>               MATCH_PER_STREAM_OPT(hwaccels, str, hwaccel, ic, st);
>               MATCH_PER_STREAM_OPT(hwaccel_output_formats, str,
>                                    hwaccel_output_format, ic, st);
> @@ -1752,6 +1792,8 @@ static OutputStream *new_video_stream(OptionsContext *o, AVFormatContext *oc, in
>           ost->frame_aspect_ratio = q;
>       }
>   
> +    add_display_matrix_to_stream(o, oc, st);
> +
>       MATCH_PER_STREAM_OPT(filter_scripts, str, ost->filters_script, oc, st);
>       MATCH_PER_STREAM_OPT(filters,        str, ost->filters,        oc, st);
>   
> @@ -3731,6 +3773,16 @@ const OptionDef options[] = {
>       { "aspect",       OPT_VIDEO | HAS_ARG  | OPT_STRING | OPT_SPEC |
>                         OPT_OUTPUT,                                                { .off = OFFSET(frame_aspect_ratios) },
>           "set aspect ratio (4:3, 16:9 or 1.3333, 1.7777)", "aspect" },
> +    { "display_rotation", OPT_VIDEO | HAS_ARG | OPT_DOUBLE | OPT_SPEC | OPT_INPUT |
> +                          OPT_OUTPUT,                                            { .off = OFFSET(display_rotations) },
> +        "set pure counter-clockwise rotation in degrees for stream(s)",
> +        "rotation" },
> +    { "display_hflip", OPT_VIDEO | OPT_BOOL | OPT_SPEC | OPT_INPUT | OPT_OUTPUT, { .off = OFFSET(display_hflips) },
> +        "set display horizontal flip for stream(s) "
> +        "(overrides any display rotation if it is not set)"},
> +    { "display_vflip", OPT_VIDEO | OPT_BOOL | OPT_SPEC | OPT_INPUT | OPT_OUTPUT, { .off = OFFSET(display_vflips) },
> +        "set display vertical flip for stream(s) "
> +        "(overrides any display rotation if it is not set)"},
>       { "pix_fmt",      OPT_VIDEO | HAS_ARG | OPT_EXPERT  | OPT_STRING | OPT_SPEC |
>                         OPT_INPUT | OPT_OUTPUT,                                    { .off = OFFSET(frame_pix_fmts) },
>           "set pixel format", "format" },
> diff --git a/tests/fate/filter-video.mak b/tests/fate/filter-video.mak
> index 86408dd58c..ce6ae255b4 100644
> --- a/tests/fate/filter-video.mak
> +++ b/tests/fate/filter-video.mak
> @@ -856,7 +856,7 @@ fate-filter-metadata-avf-aphase-meter-out-of-phase: SRC = $(TARGET_SAMPLES)/filt
>   fate-filter-metadata-avf-aphase-meter-out-of-phase: CMD = run $(FILTER_METADATA_COMMAND) "amovie='$(SRC)',aphasemeter=video=0"
>   
>   FATE_FILTER_SAMPLES-$(call TRANSCODE, RAWVIDEO H264, MOV, ARESAMPLE_FILTER  AAC_FIXED_DECODER) += fate-filter-meta-4560-rotate0
> -fate-filter-meta-4560-rotate0: CMD = transcode mov $(TARGET_SAMPLES)/filter/sample-in-issue-505.mov mov "-c copy -metadata:s:v:0 rotate=0" "-af aresample" "" "" "-flags +bitexact -c:a aac_fixed"
> +fate-filter-meta-4560-rotate0: CMD = transcode "mov -display_rotation:v:0 0" $(TARGET_SAMPLES)/filter/sample-in-issue-505.mov mov "-c copy" "-af aresample" "" "" "-flags +bitexact -c:a aac_fixed"
>   
>   REFCMP_DEPS = FFMPEG LAVFI_INDEV TESTSRC2_FILTER AVGBLUR_FILTER METADATA_FILTER
>
Jan Ekström May 19, 2022, 6:27 a.m. UTC | #2
On Thu, May 19, 2022, 07:21 Gyan Doshi <ffmpeg@gyani.pro> wrote:

>
>
> On 2022-05-18 11:34 pm, Jan Ekström wrote:
> > This enables overriding the rotation as well as horizontal/vertical
> > flip state of a specific video stream on either the input or output
> > side.
>
> Since rotation, flip and scale are stored in a single data structure,
> how about a single option i.e. -display "rotate=90,flip=hv,scale=2"
>

I did think about that, and I even implemented AVDict based options for
ffmpeg.c in a branch. But then pretty much got tired of lack of AVOption
automation (f.ex. for the flip flagging etc), and decided that since these
are relatively basic and simple, the non-generic option for this could be
just three options in the initial submission.

Jan
Jan Ekström May 26, 2022, 11:31 a.m. UTC | #3
On Thu, May 19, 2022 at 9:27 AM Jan Ekström <jeebjp@gmail.com> wrote:
>
> On Thu, May 19, 2022, 07:21 Gyan Doshi <ffmpeg@gyani.pro> wrote:
>>
>>
>>
>> On 2022-05-18 11:34 pm, Jan Ekström wrote:
>> > This enables overriding the rotation as well as horizontal/vertical
>> > flip state of a specific video stream on either the input or output
>> > side.
>>
>> Since rotation, flip and scale are stored in a single data structure,
>> how about a single option i.e. -display "rotate=90,flip=hv,scale=2"
>
>
> I did think about that, and I even implemented AVDict based options for ffmpeg.c in a branch. But then pretty much got tired of lack of AVOption automation (f.ex. for the flip flagging etc), and decided that since these are relatively basic and simple, the non-generic option for this could be just three options in the initial submission.

In the end I did implement this with a single dict-based thing in a
branch but never got to posting it to this thread:
https://github.com/jeeb/ffmpeg/commits/ffmpeg_c_display_matrix_with_dicts

So for the positive: Now it's all in a single option so it's clear
that it's defining a single structure.
And the negative: Needs a struct definition, struct, AVOption list,
AVClass and actually if you already made a dict out of the options
before, as the functions will free the original after usage and
generate a new one, it destroys the life time expectations of the dict
and thus it is just simpler to copy the dict when entering the
function (I think there is an arguments string based API which might
or might not actually be simpler for this case).

So yea, not sure if I prefer this version.

Jan
diff mbox series

Patch

diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi
index 0d7e1a479d..f44d863f02 100644
--- a/doc/ffmpeg.texi
+++ b/doc/ffmpeg.texi
@@ -897,6 +897,35 @@  If used together with @option{-vcodec copy}, it will affect the aspect ratio
 stored at container level, but not the aspect ratio stored in encoded
 frames, if it exists.
 
+@item -display_rotation[:@var{stream_specifier}] @var{rotation} (@emph{input/output,per-stream})
+Set the video display rotation in degrees specified by @var{rotation}.
+
+@var{rotation} is a floating point number that describes a pure
+counter-clockwise rotation in degrees.
+
+When setting this for input streams, @code{-autorotate} logic will be affected.
+For additional parameters affecting display matrix side data into which this
+information is saved, see @code{-display_hflip} and @code{-display_vflip}.
+
+These options work as a unit, so if only one of them is set, then the display
+matrix will be overridden to that specific value with the rest being set to
+default values.
+
+If unset, the default value if a display matrix is being defined is a rotation
+of zero degrees.
+
+@item -display_hflip[:@var{stream_specifier}] (@emph{input/output,per-stream})
+Set whether on display the image should be horizontally flipped.
+
+If unset, the default value if a display matrix is being defined is that there
+is no additional horizontal flip. See @code{-display_rotation}.
+
+@item -display_vflip[:@var{stream_specifier}] (@emph{input/output,per-stream})
+Set whether on display the image should be vertically flipped.
+
+If unset, the default value if a display matrix is being defined is that there
+is no additional vertical flip. See @code{-display_rotation}.
+
 @item -vn (@emph{input/output})
 As an input option, blocks all video streams of a file from being filtered or
 being automatically selected or mapped for any output. See @code{-discard}
diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h
index 9f0c093e34..0da01b0668 100644
--- a/fftools/ffmpeg.h
+++ b/fftools/ffmpeg.h
@@ -180,6 +180,12 @@  typedef struct OptionsContext {
     int        nb_force_fps;
     SpecifierOpt *frame_aspect_ratios;
     int        nb_frame_aspect_ratios;
+    SpecifierOpt *display_rotations;
+    int        nb_display_rotations;
+    SpecifierOpt *display_hflips;
+    int        nb_display_hflips;
+    SpecifierOpt *display_vflips;
+    int        nb_display_vflips;
     SpecifierOpt *rc_overrides;
     int        nb_rc_overrides;
     SpecifierOpt *intra_matrices;
diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
index 47e8b9b7bd..0d1c84d6df 100644
--- a/fftools/ffmpeg_opt.c
+++ b/fftools/ffmpeg_opt.c
@@ -20,6 +20,7 @@ 
 
 #include "config.h"
 
+#include <float.h>
 #include <stdint.h>
 
 #if HAVE_SYS_RESOURCE_H
@@ -43,6 +44,7 @@ 
 #include "libavutil/avutil.h"
 #include "libavutil/bprint.h"
 #include "libavutil/channel_layout.h"
+#include "libavutil/display.h"
 #include "libavutil/intreadwrite.h"
 #include "libavutil/fifo.h"
 #include "libavutil/mathematics.h"
@@ -82,6 +84,9 @@  static const char *const opt_name_qscale[]                    = {"q", "qscale",
 static const char *const opt_name_forced_key_frames[]         = {"forced_key_frames", NULL};
 static const char *const opt_name_force_fps[]                 = {"force_fps", NULL};
 static const char *const opt_name_frame_aspect_ratios[]       = {"aspect", NULL};
+static const char *const opt_name_display_rotations[]         = {"display_rotation", NULL};
+static const char *const opt_name_display_hflips[]            = {"display_hflip", NULL};
+static const char *const opt_name_display_vflips[]            = {"display_vflip", NULL};
 static const char *const opt_name_rc_overrides[]              = {"rc_override", NULL};
 static const char *const opt_name_intra_matrices[]            = {"intra_matrix", NULL};
 static const char *const opt_name_inter_matrices[]            = {"inter_matrix", NULL};
@@ -739,6 +744,39 @@  static int opt_recording_timestamp(void *optctx, const char *opt, const char *ar
     return 0;
 }
 
+static void add_display_matrix_to_stream(OptionsContext *o,
+                                         AVFormatContext *ctx, AVStream *st)
+{
+    double display_rotation = DBL_MAX;
+    int hflip = -1, vflip = -1,
+        hflip_set = 0, vflip_set = 0, display_rotation_set = 0;
+    uint8_t *buf = NULL;
+
+    MATCH_PER_STREAM_OPT(display_rotations, dbl, display_rotation, ctx, st);
+    MATCH_PER_STREAM_OPT(display_hflips,    i,   hflip,            ctx, st);
+    MATCH_PER_STREAM_OPT(display_vflips,    i,   vflip,            ctx, st);
+
+    display_rotation_set = display_rotation != DBL_MAX;
+    hflip_set            = hflip != -1;
+    vflip_set            = vflip != -1;
+
+    if (!display_rotation_set && !hflip_set && !vflip_set)
+        return;
+
+    if (!(buf = av_stream_new_side_data(st, AV_PKT_DATA_DISPLAYMATRIX,
+                                        sizeof(int32_t) * 9))) {
+        av_log(NULL, AV_LOG_FATAL, "Failed to generate a display matrix!\n");
+        exit_program(1);
+    }
+
+    av_display_rotation_set((int32_t *)buf,
+                            display_rotation_set ? -display_rotation :
+                                                   -0.0f);
+    av_display_matrix_flip((int32_t *)buf,
+                           hflip_set ? hflip : 0,
+                           vflip_set ? vflip : 0);
+}
+
 static const AVCodec *find_codec_or_die(const char *name, enum AVMediaType type, int encoder)
 {
     const AVCodecDescriptor *desc;
@@ -893,6 +931,8 @@  static void add_input_streams(OptionsContext *o, AVFormatContext *ic)
             ist->top_field_first = -1;
             MATCH_PER_STREAM_OPT(top_field_first, i, ist->top_field_first, ic, st);
 
+            add_display_matrix_to_stream(o, ic, st);
+
             MATCH_PER_STREAM_OPT(hwaccels, str, hwaccel, ic, st);
             MATCH_PER_STREAM_OPT(hwaccel_output_formats, str,
                                  hwaccel_output_format, ic, st);
@@ -1752,6 +1792,8 @@  static OutputStream *new_video_stream(OptionsContext *o, AVFormatContext *oc, in
         ost->frame_aspect_ratio = q;
     }
 
+    add_display_matrix_to_stream(o, oc, st);
+
     MATCH_PER_STREAM_OPT(filter_scripts, str, ost->filters_script, oc, st);
     MATCH_PER_STREAM_OPT(filters,        str, ost->filters,        oc, st);
 
@@ -3731,6 +3773,16 @@  const OptionDef options[] = {
     { "aspect",       OPT_VIDEO | HAS_ARG  | OPT_STRING | OPT_SPEC |
                       OPT_OUTPUT,                                                { .off = OFFSET(frame_aspect_ratios) },
         "set aspect ratio (4:3, 16:9 or 1.3333, 1.7777)", "aspect" },
+    { "display_rotation", OPT_VIDEO | HAS_ARG | OPT_DOUBLE | OPT_SPEC | OPT_INPUT |
+                          OPT_OUTPUT,                                            { .off = OFFSET(display_rotations) },
+        "set pure counter-clockwise rotation in degrees for stream(s)",
+        "rotation" },
+    { "display_hflip", OPT_VIDEO | OPT_BOOL | OPT_SPEC | OPT_INPUT | OPT_OUTPUT, { .off = OFFSET(display_hflips) },
+        "set display horizontal flip for stream(s) "
+        "(overrides any display rotation if it is not set)"},
+    { "display_vflip", OPT_VIDEO | OPT_BOOL | OPT_SPEC | OPT_INPUT | OPT_OUTPUT, { .off = OFFSET(display_vflips) },
+        "set display vertical flip for stream(s) "
+        "(overrides any display rotation if it is not set)"},
     { "pix_fmt",      OPT_VIDEO | HAS_ARG | OPT_EXPERT  | OPT_STRING | OPT_SPEC |
                       OPT_INPUT | OPT_OUTPUT,                                    { .off = OFFSET(frame_pix_fmts) },
         "set pixel format", "format" },
diff --git a/tests/fate/filter-video.mak b/tests/fate/filter-video.mak
index 86408dd58c..ce6ae255b4 100644
--- a/tests/fate/filter-video.mak
+++ b/tests/fate/filter-video.mak
@@ -856,7 +856,7 @@  fate-filter-metadata-avf-aphase-meter-out-of-phase: SRC = $(TARGET_SAMPLES)/filt
 fate-filter-metadata-avf-aphase-meter-out-of-phase: CMD = run $(FILTER_METADATA_COMMAND) "amovie='$(SRC)',aphasemeter=video=0"
 
 FATE_FILTER_SAMPLES-$(call TRANSCODE, RAWVIDEO H264, MOV, ARESAMPLE_FILTER  AAC_FIXED_DECODER) += fate-filter-meta-4560-rotate0
-fate-filter-meta-4560-rotate0: CMD = transcode mov $(TARGET_SAMPLES)/filter/sample-in-issue-505.mov mov "-c copy -metadata:s:v:0 rotate=0" "-af aresample" "" "" "-flags +bitexact -c:a aac_fixed"
+fate-filter-meta-4560-rotate0: CMD = transcode "mov -display_rotation:v:0 0" $(TARGET_SAMPLES)/filter/sample-in-issue-505.mov mov "-c copy" "-af aresample" "" "" "-flags +bitexact -c:a aac_fixed"
 
 REFCMP_DEPS = FFMPEG LAVFI_INDEV TESTSRC2_FILTER AVGBLUR_FILTER METADATA_FILTER