From patchwork Sat Mar 25 11:28:26 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: wm4 X-Patchwork-Id: 3094 Delivered-To: ffmpegpatchwork@gmail.com Received: by 10.103.50.79 with SMTP id y76csp472202vsy; Sat, 25 Mar 2017 04:28:30 -0700 (PDT) X-Received: by 10.223.170.157 with SMTP id h29mr3732828wrc.140.1490441310890; Sat, 25 Mar 2017 04:28:30 -0700 (PDT) Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org. [79.124.17.100]) by mx.google.com with ESMTP id l4si2432212wrb.169.2017.03.25.04.28.30; Sat, 25 Mar 2017 04:28:30 -0700 (PDT) Received-SPF: pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) client-ip=79.124.17.100; Authentication-Results: mx.google.com; dkim=neutral (body hash did not verify) header.i=@googlemail.com; spf=pass (google.com: domain of ffmpeg-devel-bounces@ffmpeg.org designates 79.124.17.100 as permitted sender) smtp.mailfrom=ffmpeg-devel-bounces@ffmpeg.org; dmarc=fail (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=googlemail.com Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 3BB9868988A; Sat, 25 Mar 2017 13:28:07 +0200 (EET) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wr0-f174.google.com (mail-wr0-f174.google.com [209.85.128.174]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id B246D689834 for ; Sat, 25 Mar 2017 13:28:00 +0200 (EET) Received: by mail-wr0-f174.google.com with SMTP id u108so7516949wrb.3 for ; Sat, 25 Mar 2017 04:28:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=googlemail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=J1BGAKSrGp2mc8Dv0r7yGzoxLEjXtmizuVCCttOd5Xs=; b=qZ227lsg3RtNdni7pUZbLCVdVVlm/3kwzLW4gin+ZdBRFllil5PhHvNFCZzm0iNy3n 1b2ebB9aHMZz5X+gHjZCoKUB5cVH2hucwR1MeDAV82GEMIoDXNryKIy+ZqMxCJAsrdWG YQSlwuXRY2VJ88gdDefXahcHhLEPgfY1yPm5QSGiKeEgcXYy3IbQnOUZyyRo8MHh0pI4 B1LQIr3F6wuvXfe+zCgyYV9sgtzjpnrNDlmcJind97q3SRJlqY8LJkx9nrRigD5oBCZC pQ/TvHnzN2AzM2IlF7x0JsZE9BdYjFOBTOjabUzNoKajycrd9UXz92x2k8ONd/yQBRfo syTw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=J1BGAKSrGp2mc8Dv0r7yGzoxLEjXtmizuVCCttOd5Xs=; b=p48mNHTxnLykMJOM/93eWhFMLcktdNLm6Mq9K3rsvhe2afsSJiRFep6RGvd/S1xn21 aYROCShzcdP3iUSI84fdKLdmXihmQunLD2mT8LRvVgO8neykQqU9f44NHv90xFW7KeeQ o+N9qm6bPb+g/WK9DNUUB9kuLWROWhnZWaFgao3uswpTrTz35Z1deQX+IOC6nlBDoZPv OK9JcNRY6hZWPlX/XhWPONG9c68aMTVaP78+Q1lkScNveeh5tDrDFtYNFn/brL+0Qa4L B/r810A7j42cNi4cV87APUjVumBdvd9WYiTAScsIanewVSqT3zCukTp8TNtWziww+NEe Et2w== X-Gm-Message-State: AFeK/H2SRdAk09cNSCmuxSG8xRuRaBvWuwC/rUX+13FeYOPIctF6QxwDkfByIII4qJecAw== X-Received: by 10.223.139.152 with SMTP id o24mr11990518wra.61.1490441300806; Sat, 25 Mar 2017 04:28:20 -0700 (PDT) Received: from localhost.localdomain (p4FF02162.dip0.t-ipconnect.de. [79.240.33.98]) by smtp.googlemail.com with ESMTPSA id w99sm6543126wrb.3.2017.03.25.04.28.19 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 25 Mar 2017 04:28:19 -0700 (PDT) From: wm4 To: ffmpeg-devel@ffmpeg.org Date: Sat, 25 Mar 2017 12:28:26 +0100 Message-Id: <20170325112826.31592-1-nfxjfg@googlemail.com> X-Mailer: git-send-email 2.11.0 In-Reply-To: <20170325015503.GZ4714@nb4> References: <20170325015503.GZ4714@nb4> Subject: [FFmpeg-devel] [PATCH v3] avformat, ffmpeg: deprecate old rotation API X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Cc: wm4 MIME-Version: 1.0 Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" 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