[FFmpeg-devel] Export display matrix mirroring info as part of the rotate API

Submitted by Ted Meyer on May 16, 2019, 11:34 p.m.

Details

Message ID CAELrAsqXNotjJzdK=3Z6ULF-0eiShoJeVzKvZMw+Toy35t_gnw@mail.gmail.com
State New
Headers show

Commit Message

Ted Meyer May 16, 2019, 11:34 p.m.
Right now ffmpeg doesn't export a mirroring status when checking the
display matrix for rotation.
Here is an example video: https://files.tedm.io/flip.mp4
-Ted

Comments

Jun Li May 17, 2019, 1:32 a.m.
On Thu, May 16, 2019 at 4:34 PM Ted Meyer <
tmathmeyer-at-google.com@ffmpeg.org> wrote:

> Right now ffmpeg doesn't export a mirroring status when checking the
> display matrix for rotation.
> Here is an example video: https://files.tedm.io/flip.mp4
> -Ted
> _______________________________________________
> 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".


There is a patch but not merged into master, quality not guaranteed :)
Hope this helps.
https://patchwork.ffmpeg.org/patch/13130/
Vittorio Giovara May 17, 2019, 2:38 a.m.
On Thu, May 16, 2019 at 9:32 PM Jun Li <junli1026@gmail.com> wrote:

> On Thu, May 16, 2019 at 4:34 PM Ted Meyer <
> tmathmeyer-at-google.com@ffmpeg.org> wrote:
>
> > Right now ffmpeg doesn't export a mirroring status when checking the
> > display matrix for rotation.
> > Here is an example video: https://files.tedm.io/flip.mp4
> > -Ted
> > _______________________________________________
> > 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".
>
>
> There is a patch but not merged into master, quality not guaranteed :)
> Hope this helps.
> https://patchwork.ffmpeg.org/patch/13130/
>

hey i just noticed this patch

+    if (CONV_FP(m[0]) * CONV_FP(m[4]) < CONV_FP(m[1]) *
CONV_FP(m[3])) {+        *hflip = 1;+        av_display_matrix_flip(m,
1, 0);+    }

the long if is basically computing the determinant of the matrix, but you
only need the fact whether it's positive or negative, you can discard the
result so you can avoid converting to CONV_FP, and just cast to int64_t

+    return av_display_rotation_get(m);

don't you need to set vertical flip only if det < 0 and rot = 180?

beside that small point, this patch introduces an api that basically
supersedes the normal av_display_rotation_get(), and does many more things,
I'd be tempted to deprecate any other use, and what do you think? in that
case you could just call it av_display_rotation_get2() like is tradition

I can't find the other patches from the set to review, would you be able to
send an updated version?
thanks
Jun Li May 17, 2019, 7:07 a.m.
On Thu, May 16, 2019 at 7:38 PM Vittorio Giovara <vittorio.giovara@gmail.com>
wrote:

> On Thu, May 16, 2019 at 9:32 PM Jun Li <junli1026@gmail.com> wrote:
>
> > On Thu, May 16, 2019 at 4:34 PM Ted Meyer <
> > tmathmeyer-at-google.com@ffmpeg.org> wrote:
> >
> > > Right now ffmpeg doesn't export a mirroring status when checking the
> > > display matrix for rotation.
> > > Here is an example video: https://files.tedm.io/flip.mp4
> > > -Ted
> > > _______________________________________________
> > > 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".
> >
> >
> > There is a patch but not merged into master, quality not guaranteed :)
> > Hope this helps.
> > https://patchwork.ffmpeg.org/patch/13130/
> >
>
> hey i just noticed this patch
>
> +    if (CONV_FP(m[0]) * CONV_FP(m[4]) < CONV_FP(m[1]) *
> CONV_FP(m[3])) {+        *hflip = 1;+        av_display_matrix_flip(m,
> 1, 0);+    }
>
> the long if is basically computing the determinant of the matrix, but you
> only need the fact whether it's positive or negative, you can discard the
> result so you can avoid converting to CONV_FP, and just cast to int64_t
>
+    return av_display_rotation_get(m);
>
> don't you need to set vertical flip only if det < 0 and rot = 180?
>

I think it should be fine and not need to check. Because vlip could be
represented as hflip+rotation, the cross product (Michael's brilliant idea)
m[0] * m[4] - m[1]*[3]  cannot reflect it is horizontal or vertical flip.
It all depends how we want to represents the transform, it could be
"rotation+hflip"  or "rotation+vlip".    Any combination of rotation+flip
can be represented by either one.



> beside that small point, this patch introduces an api that basically
> supersedes the normal av_display_rotation_get(), and does many more things,
> I'd be tempted to deprecate any other use, and what do you think? in that
> case you could just call it av_display_rotation_get2() like is tradition

I can't find the other patches from the set to review, would you be able to
> send an updated version?
>

Thanks Vittorio, I updated the version as suggested:
https://patchwork.ffmpeg.org/patch/13181/

The original patch includes two part, but I am going to abandon it after
discussing with Nicolas.
The patches were for per frame flip/mirror and rotation. Basically it
checks every frame's meta-data and address flip or rotation or both.
I think it also covers stream level flip, addressed in ffmpeg_filter.c
(correct me if I am wrong).
https://patchwork.ffmpeg.org/patch/13131/
https://patchwork.ffmpeg.org/patch/13130/

Best Regards,
Jun


> thanks
> --
> Vittorio
> _______________________________________________
> 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

From 7121cecdd54f403cb857dbfd056ca22253d6948a Mon Sep 17 00:00:00 2001
From: Ted Meyer <tmathmeyer@chromium.org>
Date: Thu, 16 May 2019 14:17:51 -0700
Subject: [PATCH 1/1] Export display matrix mirroring information
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

normally a display matrix will look like:
[ cos(Θ), -sin(Θ), ...]
[ sin(Θ),  cos(Θ), ...]
[ ...,     ...,    ...]
but switching the signs can be used to imply a mirroring effect.
If the cosine values are of a different sign, then there is a
mirroring about the y-axis, and if the sine values are the same
sign, but non-zero, then there is a mirroring about the x-axis.

Signed-off-by: Ted Meyer <tmathmeyer@chromium.org>
---
 libavformat/mov.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/libavformat/mov.c b/libavformat/mov.c
index e40bcf3b86..b8748ee374 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -4560,6 +4560,16 @@  static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
             snprintf(rotate_buf, sizeof(rotate_buf), "%g", rotate);
             av_dict_set(&st->metadata, "rotate", rotate_buf, 0);
         }
+
+        if (sc->display_matrix[0] != sc->display_matrix[4]) {
+            av_dict_set(&st->metadata, "mirror-y", "yes", 0);
+        }
+
+        // non-mirrored matricies have differing signs - except 0
+        if (sc->display_matrix[1] == sc->display_matrix[3] &&
+            sc->display_matrix[1] != 0) {
+            av_dict_set(&st->metadata, "mirror-x", "yes", 0);
+        }
 #endif
     }
 
-- 
2.21.0.1020.gf2820cf01a-goog