diff mbox

[FFmpeg-devel] avformat, ffmpeg: deprecate old rotation API

Message ID 20170321074258.7426-1-nfxjfg@googlemail.com
State Superseded
Headers show

Commit Message

wm4 March 21, 2017, 7:42 a.m. UTC
The old "API" that signaled rotation as a metadata value has been
replaced by DISPLAYMATRIX side data quite a while ago.

There is no reason to make muxers/demuxers/API users support both. In
addition, the metadata API is dangerous, as user tags could "leak" into
it, creating unintended features or bugs.

Unfortunately, hacks have been added, that allow the user to override
rotation by setting metadata explicitly, e.g. via

  -metadata:s:v:0 rotate=0

See references to trac #4560. fate-filter-meta-4560-rotate0 tests this.
It's easier to adjust the hack for supporting it than arguing for its
removal, so ffmpeg CLI now explicitly catches this case, and essentially
replaces the "rotate" value with a display matrix side data. (It would
be easier for both user and implementation to create an explicit option
for rotation.)

When the code under FF_API_OLD_ROTATE_API is disabled, one FATE
reference file has to be updated (because "rotate" is not exported
anymore).
---
 cmdutils.c            |  4 ++++
 ffmpeg.c              | 11 ++++++++---
 ffmpeg.h              |  1 +
 ffmpeg_opt.c          | 12 ++++++++----
 libavformat/mov.c     |  2 ++
 libavformat/movenc.c  |  4 ++++
 libavformat/version.h |  3 +++
 7 files changed, 30 insertions(+), 7 deletions(-)

Comments

Carl Eugen Hoyos March 21, 2017, 7:45 a.m. UTC | #1
2017-03-21 8:42 GMT+01:00 wm4 <nfxjfg@googlemail.com>:

> There is no reason to make muxers/demuxers/API users support both.

I don't understand this sentence: You mean having both APIs means
all users of FFmpeg have to support both APIs?

Carl Eugen
Ronald S. Bultje March 21, 2017, 12:07 p.m. UTC | #2
Hi,

On Tue, Mar 21, 2017 at 3:45 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com>
wrote:

> 2017-03-21 8:42 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
>
> > There is no reason to make muxers/demuxers/API users support both.
>
> I don't understand this sentence: You mean having both APIs means
> all users of FFmpeg have to support both APIs?


Yes, of course.

Let's not further nitpick on seemingly irrelevant details like that,
though. Can we discuss the patch at hand? We all agree that we don't want 2
rotation APIs. (I didn't look at the patch itself but I like the idea of
getting rid of a duplicate API.)

Ronald
Clément Bœsch March 21, 2017, 1:09 p.m. UTC | #3
On Tue, Mar 21, 2017 at 08:42:58AM +0100, wm4 wrote:
> The old "API" that signaled rotation as a metadata value has been
> replaced by DISPLAYMATRIX side data quite a while ago.
> 
> There is no reason to make muxers/demuxers/API users support both. In
> addition, the metadata API is dangerous, as user tags could "leak" into
> it, creating unintended features or bugs.
> 
> Unfortunately, hacks have been added, that allow the user to override
> rotation by setting metadata explicitly, e.g. via
> 
>   -metadata:s:v:0 rotate=0
> 

does remux and transcode properly keeps the property? And the other way
around, can we still remove the rotate property?

[...]
wm4 March 21, 2017, 1:14 p.m. UTC | #4
On Tue, 21 Mar 2017 14:09:49 +0100
Clément Bœsch <u@pkh.me> wrote:

> On Tue, Mar 21, 2017 at 08:42:58AM +0100, wm4 wrote:
> > The old "API" that signaled rotation as a metadata value has been
> > replaced by DISPLAYMATRIX side data quite a while ago.
> > 
> > There is no reason to make muxers/demuxers/API users support both. In
> > addition, the metadata API is dangerous, as user tags could "leak" into
> > it, creating unintended features or bugs.
> > 
> > Unfortunately, hacks have been added, that allow the user to override
> > rotation by setting metadata explicitly, e.g. via
> > 
> >   -metadata:s:v:0 rotate=0
> >   
> 
> does remux and transcode properly keeps the property? And the other way
> around, can we still remove the rotate property?

Remuxing should keep the DISPLAY_MATRIX, there's nothing special about
it. Not sure what you mean by removing the rotate "property". The mp4
muxer, which is the only thing seems to support writing rotation,
always writes a display matrix. For removing metadata fields, no idea
how that works.
Michael Niedermayer March 22, 2017, 6:32 p.m. UTC | #5
On Tue, Mar 21, 2017 at 08:42:58AM +0100, wm4 wrote:
> The old "API" that signaled rotation as a metadata value has been
> replaced by DISPLAYMATRIX side data quite a while ago.
> 
> There is no reason to make muxers/demuxers/API users support both. In
> addition, the metadata API is dangerous, as user tags could "leak" into
> it, creating unintended features or bugs.
> 
> Unfortunately, hacks have been added, that allow the user to override
> rotation by setting metadata explicitly, e.g. via
> 
>   -metadata:s:v:0 rotate=0
> 
> See references to trac #4560. fate-filter-meta-4560-rotate0 tests this.
> It's easier to adjust the hack for supporting it than arguing for its
> removal, so ffmpeg CLI now explicitly catches this case, and essentially
> replaces the "rotate" value with a display matrix side data. (It would
> be easier for both user and implementation to create an explicit option
> for rotation.)
> 
> When the code under FF_API_OLD_ROTATE_API is disabled, one FATE
> reference file has to be updated (because "rotate" is not exported
> anymore).
> ---
>  cmdutils.c            |  4 ++++
>  ffmpeg.c              | 11 ++++++++---
>  ffmpeg.h              |  1 +
>  ffmpeg_opt.c          | 12 ++++++++----
>  libavformat/mov.c     |  2 ++
>  libavformat/movenc.c  |  4 ++++
>  libavformat/version.h |  3 +++
>  7 files changed, 30 insertions(+), 7 deletions(-)

this breaks rotate handling

for example:
./ffmpeg -i ~/tickets/4012/IMG_4596.MOV  -t 0.5  test.mov
./ffplay test.mov
is upside down

https://s.natalian.org/2014-10-07/IMG_4596.MOV

[...]
wm4 March 23, 2017, 6:26 a.m. UTC | #6
On Wed, 22 Mar 2017 19:32:11 +0100
Michael Niedermayer <michael@niedermayer.cc> wrote:

> On Tue, Mar 21, 2017 at 08:42:58AM +0100, wm4 wrote:
> > The old "API" that signaled rotation as a metadata value has been
> > replaced by DISPLAYMATRIX side data quite a while ago.
> > 
> > There is no reason to make muxers/demuxers/API users support both. In
> > addition, the metadata API is dangerous, as user tags could "leak" into
> > it, creating unintended features or bugs.
> > 
> > Unfortunately, hacks have been added, that allow the user to override
> > rotation by setting metadata explicitly, e.g. via
> > 
> >   -metadata:s:v:0 rotate=0
> > 
> > See references to trac #4560. fate-filter-meta-4560-rotate0 tests this.
> > It's easier to adjust the hack for supporting it than arguing for its
> > removal, so ffmpeg CLI now explicitly catches this case, and essentially
> > replaces the "rotate" value with a display matrix side data. (It would
> > be easier for both user and implementation to create an explicit option
> > for rotation.)
> > 
> > When the code under FF_API_OLD_ROTATE_API is disabled, one FATE
> > reference file has to be updated (because "rotate" is not exported
> > anymore).
> > ---
> >  cmdutils.c            |  4 ++++
> >  ffmpeg.c              | 11 ++++++++---
> >  ffmpeg.h              |  1 +
> >  ffmpeg_opt.c          | 12 ++++++++----
> >  libavformat/mov.c     |  2 ++
> >  libavformat/movenc.c  |  4 ++++
> >  libavformat/version.h |  3 +++
> >  7 files changed, 30 insertions(+), 7 deletions(-)  
> 
> this breaks rotate handling
> 
> for example:
> ./ffmpeg -i ~/tickets/4012/IMG_4596.MOV  -t 0.5  test.mov
> ./ffplay test.mov
> is upside down
> 
> https://s.natalian.org/2014-10-07/IMG_4596.MOV
> 
> [...]
> 

With or without the FF_API symbol disabled?
Michael Niedermayer March 23, 2017, 3:50 p.m. UTC | #7
On Thu, Mar 23, 2017 at 07:26:23AM +0100, wm4 wrote:
> On Wed, 22 Mar 2017 19:32:11 +0100
> Michael Niedermayer <michael@niedermayer.cc> wrote:
> 
> > On Tue, Mar 21, 2017 at 08:42:58AM +0100, wm4 wrote:
> > > The old "API" that signaled rotation as a metadata value has been
> > > replaced by DISPLAYMATRIX side data quite a while ago.
> > > 
> > > There is no reason to make muxers/demuxers/API users support both. In
> > > addition, the metadata API is dangerous, as user tags could "leak" into
> > > it, creating unintended features or bugs.
> > > 
> > > Unfortunately, hacks have been added, that allow the user to override
> > > rotation by setting metadata explicitly, e.g. via
> > > 
> > >   -metadata:s:v:0 rotate=0
> > > 
> > > See references to trac #4560. fate-filter-meta-4560-rotate0 tests this.
> > > It's easier to adjust the hack for supporting it than arguing for its
> > > removal, so ffmpeg CLI now explicitly catches this case, and essentially
> > > replaces the "rotate" value with a display matrix side data. (It would
> > > be easier for both user and implementation to create an explicit option
> > > for rotation.)
> > > 
> > > When the code under FF_API_OLD_ROTATE_API is disabled, one FATE
> > > reference file has to be updated (because "rotate" is not exported
> > > anymore).
> > > ---
> > >  cmdutils.c            |  4 ++++
> > >  ffmpeg.c              | 11 ++++++++---
> > >  ffmpeg.h              |  1 +
> > >  ffmpeg_opt.c          | 12 ++++++++----
> > >  libavformat/mov.c     |  2 ++
> > >  libavformat/movenc.c  |  4 ++++
> > >  libavformat/version.h |  3 +++
> > >  7 files changed, 30 insertions(+), 7 deletions(-)  
> > 
> > this breaks rotate handling
> > 
> > for example:
> > ./ffmpeg -i ~/tickets/4012/IMG_4596.MOV  -t 0.5  test.mov
> > ./ffplay test.mov
> > is upside down
> > 
> > https://s.natalian.org/2014-10-07/IMG_4596.MOV
> > 
> > [...]
> > 
> 
> With or without the FF_API symbol disabled?

i changed nothing related to FF_API except what the patches change

[...]
diff mbox

Patch

diff --git a/cmdutils.c b/cmdutils.c
index 2373dbf841..594581faa4 100644
--- a/cmdutils.c
+++ b/cmdutils.c
@@ -2097,17 +2097,21 @@  void *grow_array(void *array, int elem_size, int *size, int new_size)
 
 double get_rotation(AVStream *st)
 {
+#if FF_API_OLD_ROTATE_API
     AVDictionaryEntry *rotate_tag = av_dict_get(st->metadata, "rotate", NULL, 0);
+#endif
     uint8_t* displaymatrix = av_stream_get_side_data(st,
                                                      AV_PKT_DATA_DISPLAYMATRIX, NULL);
     double theta = 0;
 
+#if FF_API_OLD_ROTATE_API
     if (rotate_tag && *rotate_tag->value && strcmp(rotate_tag->value, "0")) {
         char *tail;
         theta = av_strtod(rotate_tag->value, &tail);
         if (*tail)
             theta = 0;
     }
+#endif
     if (displaymatrix && !theta)
         theta = -av_display_rotation_get((int32_t*) displaymatrix);
 
diff --git a/ffmpeg.c b/ffmpeg.c
index dcb7720241..6dc36c9e0a 100644
--- a/ffmpeg.c
+++ b/ffmpeg.c
@@ -50,6 +50,7 @@ 
 #include "libavutil/internal.h"
 #include "libavutil/intreadwrite.h"
 #include "libavutil/dict.h"
+#include "libavutil/display.h"
 #include "libavutil/mathematics.h"
 #include "libavutil/pixdesc.h"
 #include "libavutil/avstring.h"
@@ -3051,9 +3052,6 @@  static int init_output_stream_streamcopy(OutputStream *ost)
             const AVPacketSideData *sd_src = &ist->st->side_data[i];
             AVPacketSideData *sd_dst = &ost->st->side_data[ost->st->nb_side_data];
 
-            if (ost->rotate_overridden && sd_src->type == AV_PKT_DATA_DISPLAYMATRIX)
-                continue;
-
             sd_dst->data = av_malloc(sd_src->size);
             if (!sd_dst->data)
                 return AVERROR(ENOMEM);
@@ -3064,6 +3062,13 @@  static int init_output_stream_streamcopy(OutputStream *ost)
         }
     }
 
+    if (ost->rotate_overridden) {
+        uint8_t *sd = av_stream_new_side_data(ost->st, AV_PKT_DATA_DISPLAYMATRIX,
+                                              sizeof(int32_t) * 9);
+        if (sd)
+            av_display_rotation_set((int32_t *)sd, -ost->rotate_override_value);
+    }
+
     ost->parser = av_parser_init(par_dst->codec_id);
     ost->parser_avctx = avcodec_alloc_context3(NULL);
     if (!ost->parser_avctx)
diff --git a/ffmpeg.h b/ffmpeg.h
index c3ed6ced78..4d0456c1fb 100644
--- a/ffmpeg.h
+++ b/ffmpeg.h
@@ -475,6 +475,7 @@  typedef struct OutputStream {
     int force_fps;
     int top_field_first;
     int rotate_overridden;
+    double rotate_override_value;
 
     AVRational frame_aspect_ratio;
 
diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c
index fc885dfac3..ffe1abdb38 100644
--- a/ffmpeg_opt.c
+++ b/ffmpeg_opt.c
@@ -2549,8 +2549,6 @@  loop_end:
             av_dict_copy(&output_streams[i]->st->metadata, ist->st->metadata, AV_DICT_DONT_OVERWRITE);
             if (!output_streams[i]->stream_copy) {
                 av_dict_set(&output_streams[i]->st->metadata, "encoder", NULL, 0);
-                if (ist->autorotate)
-                    av_dict_set(&output_streams[i]->st->metadata, "rotate", NULL, 0);
             }
         }
 
@@ -2640,9 +2638,15 @@  loop_end:
             for (j = 0; j < oc->nb_streams; j++) {
                 ost = output_streams[nb_output_streams - oc->nb_streams + j];
                 if ((ret = check_stream_specifier(oc, oc->streams[j], stream_spec)) > 0) {
-                    av_dict_set(&oc->streams[j]->metadata, o->metadata[i].u.str, *val ? val : NULL, 0);
                     if (!strcmp(o->metadata[i].u.str, "rotate")) {
-                        ost->rotate_overridden = 1;
+                        char *tail;
+                        double theta = av_strtod(val, &tail);
+                        if (!*tail) {
+                            ost->rotate_overridden = 1;
+                            ost->rotate_override_value = theta;
+                        }
+                    } else {
+                        av_dict_set(&oc->streams[j]->metadata, o->metadata[i].u.str, *val ? val : NULL, 0);
                     }
                 } else if (ret < 0)
                     exit_program(1);
diff --git a/libavformat/mov.c b/libavformat/mov.c
index f7dd2502c5..9581c965a3 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -4030,6 +4030,7 @@  static int mov_read_tkhd(MOVContext *c, AVIOContext *pb, MOVAtom atom)
             for (j = 0; j < 3; j++)
                 sc->display_matrix[i * 3 + j] = res_display_matrix[i][j];
 
+#if FF_API_OLD_ROTATE_API
         rotate = av_display_rotation_get(sc->display_matrix);
         if (!isnan(rotate)) {
             char rotate_buf[64];
@@ -4039,6 +4040,7 @@  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);
         }
+#endif
     }
 
     // transform the display width/height according to the matrix
diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index a28621080d..c049aa972f 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -2496,19 +2496,23 @@  static int mov_write_tkhd_tag(AVIOContext *pb, MOVMuxContext *mov,
     avio_wb16(pb, 0); /* reserved */
 
     /* Matrix structure */
+#if FF_API_OLD_ROTATE_API
     if (st && st->metadata) {
         AVDictionaryEntry *rot = av_dict_get(st->metadata, "rotate", NULL, 0);
         rotation = (rot && rot->value) ? atoi(rot->value) : 0;
     }
+#endif
     if (display_matrix) {
         for (i = 0; i < 9; i++)
             avio_wb32(pb, display_matrix[i]);
+#if FF_API_OLD_ROTATE_API
     } else if (rotation == 90) {
         write_matrix(pb,  0,  1, -1,  0, track->par->height, 0);
     } else if (rotation == 180) {
         write_matrix(pb, -1,  0,  0, -1, track->par->width, track->par->height);
     } else if (rotation == 270) {
         write_matrix(pb,  0, -1,  1,  0, 0, track->par->width);
+#endif
     } else {
         write_matrix(pb,  1,  0,  0,  1, 0, 0);
     }
diff --git a/libavformat/version.h b/libavformat/version.h
index bfc42e3f15..2ff4e4e9d0 100644
--- a/libavformat/version.h
+++ b/libavformat/version.h
@@ -94,6 +94,9 @@ 
 #ifndef FF_API_LAVF_KEEPSIDE_FLAG
 #define FF_API_LAVF_KEEPSIDE_FLAG       (LIBAVFORMAT_VERSION_MAJOR < 58)
 #endif
+#ifndef FF_API_OLD_ROTATE_API
+#define FF_API_OLD_ROTATE_API           (LIBAVFORMAT_VERSION_MAJOR < 58)
+#endif
 
 
 #ifndef FF_API_R_FRAME_RATE