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

wm4 nfxjfg at googlemail.com
Mon Mar 27 14:24:49 EEST 2017


On Sat, 25 Mar 2017 12:28:26 +0100
wm4 <nfxjfg at googlemail.com> 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.
> 
> ffmpeg CLI has to be updated to use the new API. In particular, we must
> not allow to leak the "rotate" tag into the muxer. Some muxers will
> catch this properly (like mov), but others (like mkv) can add it as
> generic tag. Note applications, which use libavformat and assume the
> old rotate API, will interpret such "rotate" user tags as rotate
> metadata (which it is not), and incorrectly rotate the video.
> 
> The ffmpeg/ffplay tools drop the use of the old API for muxing and
> demuxing, as all muxers/demuxers support the new API. This will mean
> that the tools will not mistakenly interpret per-track "rotate" user
> tags as rotate metadata. It will _not_ be treated as regression.
> 
> 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).
> ---
> Not sure what happened - probably removed some redundant code and failed
> to retest properly. Maybe now?
> ---
>  cmdutils.c            | 10 +---------
>  ffmpeg.c              | 41 ++++++++++++++++++++++++++++++++++++-----
>  ffmpeg.h              |  1 +
>  ffmpeg_opt.c          | 12 ++++++++----
>  libavformat/mov.c     |  2 ++
>  libavformat/movenc.c  |  4 ++++
>  libavformat/version.h |  3 +++
>  7 files changed, 55 insertions(+), 18 deletions(-)
> 
> diff --git a/cmdutils.c b/cmdutils.c
> index 2373dbf841..3d428f3eea 100644
> --- a/cmdutils.c
> +++ b/cmdutils.c
> @@ -2097,18 +2097,10 @@ void *grow_array(void *array, int elem_size, int *size, int new_size)
>  
>  double get_rotation(AVStream *st)
>  {
> -    AVDictionaryEntry *rotate_tag = av_dict_get(st->metadata, "rotate", NULL, 0);
>      uint8_t* displaymatrix = av_stream_get_side_data(st,
>                                                       AV_PKT_DATA_DISPLAYMATRIX, NULL);
>      double theta = 0;
> -
> -    if (rotate_tag && *rotate_tag->value && strcmp(rotate_tag->value, "0")) {
> -        char *tail;
> -        theta = av_strtod(rotate_tag->value, &tail);
> -        if (*tail)
> -            theta = 0;
> -    }
> -    if (displaymatrix && !theta)
> +    if (displaymatrix)
>          theta = -av_display_rotation_get((int32_t*) displaymatrix);
>  
>      theta -= 360*floor(theta/360 + 0.9/360);
> diff --git a/ffmpeg.c b/ffmpeg.c
> index 532db80e0b..90441224f9 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"
> @@ -3067,9 +3068,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);
> @@ -3080,6 +3078,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)
> @@ -3233,6 +3238,11 @@ static int init_output_stream_encode(OutputStream *ost)
>  
>      set_encoder_id(output_files[ost->file_index], ost);
>  
> +    // Muxers use AV_PKT_DATA_DISPLAYMATRIX to signal rotation. On the other
> +    // hand, the legacy API makes demuxers set "rotate" metadata entries,
> +    // which have to be filtered out to prevent leaking them to output files.
> +    av_dict_set(&ost->st->metadata, "rotate", NULL, 0);
> +
>      if (ist) {
>          ost->st->disposition          = ist->st->disposition;
>  
> @@ -3470,6 +3480,26 @@ static int init_output_stream(OutputStream *ost, char *error, int error_len)
>              }
>          }
>  
> +        /*
> +         * Add global input side data. For now this is naive, and copies it
> +         * from the input stream's global side data. All side data should
> +         * really be funneled over AVFrame and libavfilter, then added back to
> +         * packet side data, and then potentially using the first packet for
> +         * global side data.
> +         */
> +        if (ist) {
> +            int i;
> +            for (i = 0; i < ist->st->nb_side_data; i++) {
> +                AVPacketSideData *sd = &ist->st->side_data[i];
> +                uint8_t *dst = av_stream_new_side_data(ost->st, sd->type, sd->size);
> +                if (!dst)
> +                    return AVERROR(ENOMEM);
> +                memcpy(dst, sd->data, sd->size);
> +                if (ist->autorotate && sd->type == AV_PKT_DATA_DISPLAYMATRIX)
> +                    av_display_rotation_set((uint32_t *)dst, 0);
> +            }
> +        }
> +
>          // copy timebase while removing common factors
>          if (ost->st->time_base.num <= 0 || ost->st->time_base.den <= 0)
>              ost->st->time_base = av_add_q(ost->enc_ctx->time_base, (AVRational){0, 1});
> @@ -4266,9 +4296,10 @@ static int process_input(int file_index)
>              AVPacketSideData *src_sd = &ist->st->side_data[i];
>              uint8_t *dst_data;
>  
> -            if (av_packet_get_side_data(&pkt, src_sd->type, NULL))
> +            if (src_sd->type == AV_PKT_DATA_DISPLAYMATRIX)
>                  continue;
> -            if (ist->autorotate && src_sd->type == AV_PKT_DATA_DISPLAYMATRIX)
> +
> +            if (av_packet_get_side_data(&pkt, src_sd->type, NULL))
>                  continue;
>  
>              dst_data = av_packet_new_side_data(&pkt, src_sd->type, src_sd->size);
> 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 3754346f9e..a00c53a676 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 11b26708ae..3b4e3b519c 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 dd4c680803..7b92255bea 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

Pushed, with added version bump + APIchanges entry. Thanks for review.


More information about the ffmpeg-devel mailing list