Re: [FFmpeg-devel] [PATCH] avutil/display: fix inverted doc

Re: [FFmpeg-devel] [PATCH] avutil/display: fix inverted doc

[Andreas Rheinhardt]

Zhao Zhili:
> ---
>  libavutil/display.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavutil/display.h b/libavutil/display.h
> index 515adad795..d87bf68425 100644
> --- a/libavutil/display.h
> +++ b/libavutil/display.h
> @@ -88,7 +88,7 @@
>  double av_display_rotation_get(const int32_t matrix[9]);
>  
>  /**
> - * Initialize a transformation matrix describing a pure counterclockwise
> + * Initialize a transformation matrix describing a pure clockwise
>   * rotation by the specified angle (in degrees).
>   *
>   * @param matrix an allocated transformation matrix (will be fully overwritten
> 

a) This change brings the documentation in line with actual behaviour.
b) This bug has been introduced in
e4fe535d12f4f30df2dd672e30304af112a5a827: The mov demuxer did not abide
by the documentation of the format of the display matrix side data (it
had an incorrect transposition). Instead of just fixing the mov demuxer
to abide by this format (which coincides with the native mov format!),
said commit also negated the degrees in av_display_rotation_get/set. In
case of av_display_rotation_get() this fixed a bug, in case of
av_display_rotation_set() this introduced a bug. No one seems to have
thought to actually test whether av_display_rotation_get() actually
returns the degree.
c) There is a test for this (libavutil/tests/display.c). It does not
perform the test just mentioned; instead it has the current behaviour
hardcoded in its ref file.
d) The rotate_override stuff in ffmpeg.c compensates for the weirdness
of av_display_rotation_set() by negating the degree. get_rotation in
fftools/cmdutils.c also negates the degree, but it seems to do so
deliberately in order to return a clockwise degree (both callers expect
it that way).
e) av_display_matrix_flip() is equivalent to multiplying the matrix with
a diagonal matrix (with diagonal entries +-1) from the right; given that
applying the displaymatrix corresponds to multiplying the coordinate row
vector with the matrix on the right, this means that flipping is applied
on top of the display matrix (i.e. after the transformation specified by
the earlier display matrix has been applied). This is in line with my
expectations of what it does given its documentation.
f) Yet it is not in line with how it is used by the H.2645 SEI parsing
code: According to the H.2645 specs, the flips should be applied first.
Flipping (regardless of axis) first, then rotating by phi is equivalent
to rotating by -phi first and then flipping around the same axis. This
implies that the code in libavcodec/hevcdec.c, libavcodec/h264_slice.c
and libavcodec/h264_metadata_bsf.c (when creating side data) leads to
correct results if there is exactly one flip; without a flip or two
flips, the sign of the degree is wrong. This is the reason for the issue
in your other mail [1].
g) The code to create an SEI from side data in h264_metadata_bsf.c is
incorrect.
h) I don't know whether the exif rotation code is correct; same for
android_camera.c.

- Andreas

[1]: http://ffmpeg.org/pipermail/ffmpeg-devel/2021-December/289488.html
_______________________________________________
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".

[FFmpeg-devel] [PATCH 1/4] avcodec/mjpegdec: Fix exif rotation->displaymatrix conversion

[Andreas Rheinhardt]

The cases in which there was flipping together with a rotation
that is not a multiple of the identity were wrong.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
This whole patchset relies on the mismatch in av_display_rotation_set()
being solved by keeping the current behaviour and updating the
documentation.

 libavcodec/mjpegdec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
index 8b154ce0ab..0dbbc14bae 100644
--- a/libavcodec/mjpegdec.c
+++ b/libavcodec/mjpegdec.c
@@ -2896,14 +2896,14 @@ the_end:
                     break;
                 case 5:
                     av_display_rotation_set(matrix, 90.0);
-                    av_display_matrix_flip(matrix, 0, 1);
+                    av_display_matrix_flip(matrix, 1, 0);
                     break;
                 case 6:
                     av_display_rotation_set(matrix, 90.0);
                     break;
                 case 7:
                     av_display_rotation_set(matrix, -90.0);
-                    av_display_matrix_flip(matrix, 0, 1);
+                    av_display_matrix_flip(matrix, 1, 0);
                     break;
                 case 8:
                     av_display_rotation_set(matrix, -90.0);
-- 
2.32.0

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH 1/4] avcodec/mjpegdec: Fix exif rotation->displaymatrix conversion

[Andreas Rheinhardt]

Andreas Rheinhardt:
> The cases in which there was flipping together with a rotation
> that is not a multiple of the identity were wrong.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> This whole patchset relies on the mismatch in av_display_rotation_set()
> being solved by keeping the current behaviour and updating the
> documentation.
> 
>  libavcodec/mjpegdec.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c
> index 8b154ce0ab..0dbbc14bae 100644
> --- a/libavcodec/mjpegdec.c
> +++ b/libavcodec/mjpegdec.c
> @@ -2896,14 +2896,14 @@ the_end:
>                      break;
>                  case 5:
>                      av_display_rotation_set(matrix, 90.0);
> -                    av_display_matrix_flip(matrix, 0, 1);
> +                    av_display_matrix_flip(matrix, 1, 0);
>                      break;
>                  case 6:
>                      av_display_rotation_set(matrix, 90.0);
>                      break;
>                  case 7:
>                      av_display_rotation_set(matrix, -90.0);
> -                    av_display_matrix_flip(matrix, 0, 1);
> +                    av_display_matrix_flip(matrix, 1, 0);
>                      break;
>                  case 8:
>                      av_display_rotation_set(matrix, -90.0);
> 

Will apply this patchset tonight unless there are objections.

- Andreas
_______________________________________________
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".

[FFmpeg-devel] [PATCH 2/4] fftools/ffmpeg_filter: Fix autorotation

[Andreas Rheinhardt]

In case of an orthogonal transformation av_display_rotation_get()
returns the (anticlockwise) degree that the unit vector in x-direction
gets rotated by; get_rotation in cmdutils.c makes a clockwise degree
out of this. So if one inserts a transpose filter corresponding to
this degree, then the x-vector gets mapped correctly and there are
two possibilities for image of the y-vector, namely the two unit
vectors orthogonal to the image of the x-vector.

E.g. if the x-vector gets rotated by 90° clockwise, then the two
possibilities for the y-vector are the unit vector in x direction
or its opposite. The latter case is a simple 90° rotation for both
vectors* whereas the former is a simple 90° clockwise rotation followed
by a horizontal flip. These two cases can be distinguished by looking
at the x-coordinate of the image of the y-vector, i.e. by looking
at displaymatrix[3]. Similarly for the case of a 270° clockwise
rotation.

These two cases were previously wrong (they were made to match
wrongly parsed exif rotation tag values).

*: For display matrices, the y-axis points downward.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 fftools/ffmpeg_filter.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fftools/ffmpeg_filter.c b/fftools/ffmpeg_filter.c
index 501a0acd61..8c929ab9fa 100644
--- a/fftools/ffmpeg_filter.c
+++ b/fftools/ffmpeg_filter.c
@@ -761,12 +761,12 @@ static int configure_input_video_filter(FilterGraph *fg, InputFilter *ifilter,
         theta = get_rotation(displaymatrix);
 
         if (fabs(theta - 90) < 1.0) {
+            ret = insert_filter(&last_filter, &pad_idx, "transpose", "clock");
             if (displaymatrix[3] > 0) {
                 ret = insert_filter(&last_filter, &pad_idx, "hflip", NULL);
                 if (ret < 0)
                     return ret;
             }
-            ret = insert_filter(&last_filter, &pad_idx, "transpose", "clock");
         } else if (fabs(theta - 180) < 1.0) {
             if (displaymatrix[0] < 0) {
                 ret = insert_filter(&last_filter, &pad_idx, "hflip", NULL);
@@ -777,12 +777,12 @@ static int configure_input_video_filter(FilterGraph *fg, InputFilter *ifilter,
                 ret = insert_filter(&last_filter, &pad_idx, "vflip", NULL);
             }
         } else if (fabs(theta - 270) < 1.0) {
+            ret = insert_filter(&last_filter, &pad_idx, "transpose", "cclock");
             if (displaymatrix[3] < 0) {
                 ret = insert_filter(&last_filter, &pad_idx, "hflip", NULL);
                 if (ret < 0)
                     return ret;
             }
-            ret = insert_filter(&last_filter, &pad_idx, "transpose", "cclock");
         } else if (fabs(theta) > 1.0) {
             char rotate_buf[64];
             snprintf(rotate_buf, sizeof(rotate_buf), "%f*PI/180", theta);
-- 
2.32.0

_______________________________________________
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".

[FFmpeg-devel] [PATCH 3/4] fftools/ffmpeg_filter: Avoid inserting hflip filter

[Andreas Rheinhardt]

The transpose filter has modes equivalent to "rotation by 90°/270°"
followed by horizontal flips.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 fftools/ffmpeg_filter.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/fftools/ffmpeg_filter.c b/fftools/ffmpeg_filter.c
index 8c929ab9fa..1f6cba2c04 100644
--- a/fftools/ffmpeg_filter.c
+++ b/fftools/ffmpeg_filter.c
@@ -761,12 +761,8 @@ static int configure_input_video_filter(FilterGraph *fg, InputFilter *ifilter,
         theta = get_rotation(displaymatrix);
 
         if (fabs(theta - 90) < 1.0) {
-            ret = insert_filter(&last_filter, &pad_idx, "transpose", "clock");
-            if (displaymatrix[3] > 0) {
-                ret = insert_filter(&last_filter, &pad_idx, "hflip", NULL);
-                if (ret < 0)
-                    return ret;
-            }
+            ret = insert_filter(&last_filter, &pad_idx, "transpose",
+                                displaymatrix[3] > 0 ? "cclock_flip" : "clock");
         } else if (fabs(theta - 180) < 1.0) {
             if (displaymatrix[0] < 0) {
                 ret = insert_filter(&last_filter, &pad_idx, "hflip", NULL);
@@ -777,12 +773,8 @@ static int configure_input_video_filter(FilterGraph *fg, InputFilter *ifilter,
                 ret = insert_filter(&last_filter, &pad_idx, "vflip", NULL);
             }
         } else if (fabs(theta - 270) < 1.0) {
-            ret = insert_filter(&last_filter, &pad_idx, "transpose", "cclock");
-            if (displaymatrix[3] < 0) {
-                ret = insert_filter(&last_filter, &pad_idx, "hflip", NULL);
-                if (ret < 0)
-                    return ret;
-            }
+            ret = insert_filter(&last_filter, &pad_idx, "transpose",
+                                displaymatrix[3] < 0 ? "clock_flip" : "cclock");
         } else if (fabs(theta) > 1.0) {
             char rotate_buf[64];
             snprintf(rotate_buf, sizeof(rotate_buf), "%f*PI/180", theta);
-- 
2.32.0

_______________________________________________
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".

[FFmpeg-devel] [PATCH 4/4] avcodec/h2645: Fix SEI->display matrix transformation

[Andreas Rheinhardt]

The earlier code did not account for the fact that
av_display_rotation_set() wants the angle in the anticlockwise
direction (despite what its documentation stated for a long time);
furthermore, the H.2645 spec wants the flips applied first,
whereas our code did it the other way around. This can be fixed
by negating the angle once for every flip.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
Maybe the common H.264 and H.265 code could be shared one day?

 libavcodec/h264_metadata_bsf.c | 15 ++++++++++++---
 libavcodec/h264_slice.c        |  9 +++++++++
 libavcodec/hevcdec.c           | 10 ++++++++++
 3 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/libavcodec/h264_metadata_bsf.c b/libavcodec/h264_metadata_bsf.c
index 452a8ec5dc..8c5d19c5a8 100644
--- a/libavcodec/h264_metadata_bsf.c
+++ b/libavcodec/h264_metadata_bsf.c
@@ -341,15 +341,24 @@ static int h264_metadata_handle_display_orientation(AVBSFContext *bsf,
                                    SEI_TYPE_DISPLAY_ORIENTATION,
                                    &message) == 0) {
         H264RawSEIDisplayOrientation *disp = message->payload;
+        double angle = disp->anticlockwise_rotation * 180.0 / 65536.0;
         int32_t *matrix;
 
         matrix = av_malloc(9 * sizeof(int32_t));
         if (!matrix)
             return AVERROR(ENOMEM);
 
-        av_display_rotation_set(matrix,
-                                disp->anticlockwise_rotation *
-                                180.0 / 65536.0);
+        /* av_display_rotation_set() expects the angle in the clockwise
+         * direction, hence the first minus.
+         * The below code applies the flips after the rotation, yet
+         * the H.2645 specs require flipping to be applied first.
+         * Because of R O(phi) = O(-phi) R (where R is flipping around
+         * an arbitatry axis and O(phi) is the proper rotation by phi)
+         * we can create display matrices as desired by negating
+         * the degree once for every flip applied. */
+        angle = -angle * (1 - 2 * !!disp->hor_flip) * (1 - 2 * !!disp->ver_flip);
+
+        av_display_rotation_set(matrix, angle);
         av_display_matrix_flip(matrix, disp->hor_flip, disp->ver_flip);
 
         // If there are multiple display orientation messages in an
diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
index 4467882775..c21004df97 100644
--- a/libavcodec/h264_slice.c
+++ b/libavcodec/h264_slice.c
@@ -1305,6 +1305,15 @@ static int h264_export_frame_props(H264Context *h)
                                                            AV_FRAME_DATA_DISPLAYMATRIX,
                                                            sizeof(int32_t) * 9);
         if (rotation) {
+            /* av_display_rotation_set() expects the angle in the clockwise
+             * direction, hence the first minus.
+             * The below code applies the flips after the rotation, yet
+             * the H.2645 specs require flipping to be applied first.
+             * Because of R O(phi) = O(-phi) R (where R is flipping around
+             * an arbitatry axis and O(phi) is the proper rotation by phi)
+             * we can create display matrices as desired by negating
+             * the degree once for every flip applied. */
+            angle = -angle * (1 - 2 * !!o->hflip) * (1 - 2 * !!o->vflip);
             av_display_rotation_set((int32_t *)rotation->data, angle);
             av_display_matrix_flip((int32_t *)rotation->data,
                                    o->hflip, o->vflip);
diff --git a/libavcodec/hevcdec.c b/libavcodec/hevcdec.c
index 46d9edf8eb..3aa70e2245 100644
--- a/libavcodec/hevcdec.c
+++ b/libavcodec/hevcdec.c
@@ -2769,6 +2769,16 @@ static int set_side_data(HEVCContext *s)
         if (!rotation)
             return AVERROR(ENOMEM);
 
+        /* av_display_rotation_set() expects the angle in the clockwise
+         * direction, hence the first minus.
+         * The below code applies the flips after the rotation, yet
+         * the H.2645 specs require flipping to be applied first.
+         * Because of R O(phi) = O(-phi) R (where R is flipping around
+         * an arbitatry axis and O(phi) is the proper rotation by phi)
+         * we can create display matrices as desired by negating
+         * the degree once for every flip applied. */
+        angle = -angle * (1 - 2 * !!s->sei.display_orientation.hflip)
+                       * (1 - 2 * !!s->sei.display_orientation.vflip);
         av_display_rotation_set((int32_t *)rotation->data, angle);
         av_display_matrix_flip((int32_t *)rotation->data,
                                s->sei.display_orientation.hflip,
-- 
2.32.0

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] avutil/display: fix inverted doc

[Andreas Rheinhardt]

Zhao Zhili:
> ---
>  libavutil/display.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavutil/display.h b/libavutil/display.h
> index 515adad795..d87bf68425 100644
> --- a/libavutil/display.h
> +++ b/libavutil/display.h
> @@ -88,7 +88,7 @@
>  double av_display_rotation_get(const int32_t matrix[9]);
>  
>  /**
> - * Initialize a transformation matrix describing a pure counterclockwise
> + * Initialize a transformation matrix describing a pure clockwise
>   * rotation by the specified angle (in degrees).
>   *
>   * @param matrix an allocated transformation matrix (will be fully overwritten
> 

Will apply this together with an APIchanges entry (and micro version
bump) that explains that the documentation has been modified to match
the actual behaviour.

- Andreas
_______________________________________________
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".