From patchwork Wed Jun 13 19:58:40 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Marton Balint X-Patchwork-Id: 9396 Delivered-To: ffmpegpatchwork@gmail.com Received: by 2002:a02:11c:0:0:0:0:0 with SMTP id c28-v6csp1055636jad; Wed, 13 Jun 2018 12:58:50 -0700 (PDT) X-Google-Smtp-Source: ADUXVKLVfRqmP0w3IrixyVhls0pg5JXplZQ8en+hy1K8zrV1sqdjuvdqScTzqhAXTrBjNGRiLjwh X-Received: by 2002:adf:be09:: with SMTP id n9-v6mr5525940wrh.267.1528919930208; Wed, 13 Jun 2018 12:58:50 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1528919930; cv=none; d=google.com; s=arc-20160816; b=ioj7MgxLTyo5S6Clh/QEwoiWXjl6JLpKl9+x8qAglpS3Xj/+hO6vAr0zdef7qDTAbH tsdEIQ4imsw3JFHv+jjnwEPjfK72xFUO8VWi7N0CYOxX/4SvyFptVc5n46gC9LkgLK6S d04Mljl9LeV/1xixf8DGFF0/QlTEKD6glubOywpDBsI/lYJ13lnzAmFwHzHs/fQTpYJr UO4mZiVBSkBhKlWXmbhchbyC6nb2oKfm4x21j+Mha5i+pqJ69D1UgyepENm22ZocZtLR iBUWJYm1bP/jWPMCb/sqj1zN/Q0qW01QzZl/dJgb8mQdPtZrgXue7KzBtDjAmiqz5ZQy tA8w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=sender:errors-to:reply-to:list-subscribe:list-help:list-post :list-archive:list-unsubscribe:list-id:precedence:subject:content-id :mime-version:user-agent:references:message-id:in-reply-to:to:from :date:delivered-to:arc-authentication-results; bh=pDzZNIay9ffpbseErEHIE7wIqvdGvu2Z9P+K4L4S/28=; b=tqQggNzWXz4rqiM/EmcU1ir6x8csX4vOdwhwKNc51i6pLQnF2lPowmp/xeJOVhda8S O4fosl9SrtIsrrR3n3dyO+ZbKqpDcp5BLni47HSWpbil6cmLM3Zt1hIDHz58nhR8s2a5 43Uu/0G8tXCEF7hlEQMbu2XGcr772n1GU/cPgOCboj1qkMnwS9afKWxaC3yeI0JtvKEW y74X/rMgsIqp/4Y1y36FiES3vnUa15+XkK8TVbg3HPkJdIZ9u0aWZXFw2e9krMeruDAR a/YVAvxSpmSa7VUi9yCnswwliIm8JDBfW/lzagf5tBs9mjoiAbjdHt4nRRSEWjA7dMcn Ks2g== ARC-Authentication-Results: i=1; mx.google.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 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org. [79.124.17.100]) by mx.google.com with ESMTP id o23-v6si2472473wmc.93.2018.06.13.12.58.49; Wed, 13 Jun 2018 12:58:50 -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; 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 Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id A864A68B03D; Wed, 13 Jun 2018 22:57:58 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from iq.passwd.hu (iq.passwd.hu [217.27.212.140]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 6461868AA9D for ; Wed, 13 Jun 2018 22:57:52 +0300 (EEST) Received: from localhost (localhost [127.0.0.1]) by iq.passwd.hu (Postfix) with ESMTP id B074DE04C5 for ; Wed, 13 Jun 2018 21:58:42 +0200 (CEST) X-Virus-Scanned: amavisd-new at passwd.hu Received: from iq.passwd.hu ([127.0.0.1]) by localhost (iq.passwd.hu [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id tiht8OL0FSk3 for ; Wed, 13 Jun 2018 21:58:41 +0200 (CEST) Received: from iq (iq [217.27.212.140]) by iq.passwd.hu (Postfix) with ESMTPS id CB495E044E for ; Wed, 13 Jun 2018 21:58:40 +0200 (CEST) Date: Wed, 13 Jun 2018 21:58:40 +0200 (CEST) From: Marton Balint X-X-Sender: cus@iq To: FFmpeg development discussions and patches In-Reply-To: <1528902275.19198.14.camel@acc.umu.se> Message-ID: References: <20180610103650.10155-1-cus@passwd.hu> <20180610103650.10155-4-cus@passwd.hu> <1528902275.19198.14.camel@acc.umu.se> User-Agent: Alpine 2.20 (LSU 67 2015-01-07) MIME-Version: 1.0 Content-ID: Subject: Re: [FFmpeg-devel] [PATCH 04/12] avformat/mxfdec: compute both essence_offset and essence_length in mxf_compute_essence_containers 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 Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" On Wed, 13 Jun 2018, Tomas Härdin wrote: > sön 2018-06-10 klockan 12:36 +0200 skrev Marton Balint: >> Also compute the correct essence_offset and essence_length for all clip wrapped >> essences. >> >> > Signed-off-by: Marton Balint >> --- >>  libavformat/mxfdec.c | 108 +++++++++++++++++++++++++++------------------------ >>  1 file changed, 57 insertions(+), 51 deletions(-) >> >> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c >> index 1ab34f4873..67b0028e88 100644 >> --- a/libavformat/mxfdec.c >> +++ b/libavformat/mxfdec.c >> @@ -98,6 +98,7 @@ typedef struct MXFPartition { >>      int pack_length; >>      int64_t pack_ofs;               ///< absolute offset of pack in file, including run-in >>      int64_t body_offset; >> +    KLVPacket first_essence_klv; >>  } MXFPartition; >>   >>  typedef struct MXFCryptoContext { >> @@ -2782,47 +2783,76 @@ static int mxf_parse_handle_partition_or_eof(MXFContext *mxf) >>      return mxf->parsing_backward ? mxf_seek_to_previous_partition(mxf) : 1; >>  } >>   >> +static int64_t round_to_kag(int64_t position, int kag_size) >> +{ >> +    /* TODO: account for run-in? the spec isn't clear whether KAG should account for it */ >> +    /* NOTE: kag_size may be any integer between 1 - 2^10 */ >> +    int64_t ret = (position / kag_size) * kag_size; >> +    return ret == position ? ret : ret + kag_size; > > This should probably just be > > return ((position + kag_size - 1) / kag_size) * kag_size; > > but maybe there's some fine point I'm overlooking. What happens if > kag_size is a smallish value like 16? > > A check for position > INT64_MAX - kag_size might also be appropriate. Well, this is your code as well :), I just moved it to avoid a forward declaration. > >> +} >> + >> +static MXFWrappingScheme mxf_get_wrapping_by_body_sid(AVFormatContext *s, int body_sid) >> +{ >> +    for (int i = 0; i < s->nb_streams; i++) { >> +        MXFTrack *track = s->streams[i]->priv_data; >> +        if (track && track->body_sid == body_sid && track->wrapping != UnknownWrapped) >> +            return track->wrapping; >> +    } >> +    return UnknownWrapped; >> +} >> + >>  /** >>   * Figures out the proper offset and length of the essence container in each partition >>   */ >> -static void mxf_compute_essence_containers(MXFContext *mxf) >> +static void mxf_compute_essence_containers(AVFormatContext *s) >>  { >> +    MXFContext *mxf = s->priv_data; >>      int x; >>   >> -    /* everything is already correct */ >> -    if (mxf->op == OPAtom) >> -        return; >> - >>      for (x = 0; x < mxf->partitions_count; x++) { >>          MXFPartition *p = &mxf->partitions[x]; >> +        MXFWrappingScheme wrapping; >>   >>          if (!p->body_sid) >>              continue;       /* BodySID == 0 -> no essence */ >>   >> -        if (x >= mxf->partitions_count - 1) >> -            break;          /* FooterPartition - can't compute length (and we don't need to) */ >> +        /* for non clip-wrapped essences we compute essence_offset >> +         * for clip wrapped essences we point essence_offset after the KL (usually klv.offset + 20 or 25) >> +         */ >>   >> -        /* essence container spans to the next partition */ >> -        p->essence_length = mxf->partitions[x+1].this_partition - p->essence_offset; >> +        wrapping = (mxf->op == OPAtom) ? ClipWrapped : mxf_get_wrapping_by_body_sid(s, p->body_sid); >>   >> -        if (p->essence_length < 0) { >> -            /* next ThisPartition < essence_offset */ >> -            p->essence_length = 0; >> -            av_log(mxf->fc, AV_LOG_ERROR, >> -                   "partition %i: bad ThisPartition = %"PRIX64"\n", >> -                   x+1, mxf->partitions[x+1].this_partition); >> +        if (wrapping == ClipWrapped) { >> +            p->essence_offset = p->first_essence_klv.next_klv - p->first_essence_klv.length; >> +            p->essence_length = p->first_essence_klv.length; > > Much nicer > >> +        } else { >> +            int64_t op1a_essence_offset = >> +                p->this_partition + >> +                round_to_kag(p->pack_length,       p->kag_size) + >> +                round_to_kag(p->header_byte_count, p->kag_size) + >> +                round_to_kag(p->index_byte_count,  p->kag_size); > > Is this really always the case? I guess with OP1a it isn't a huge > concern since the demuxer will find the next essence packet anyway. But > still.. > > I'm also fairly sure this is my code originally so.. :) I think this tends to work well, if kag_size is not guessed. However, if it _is_ guessed, then there might be problems, and having a 0 kagsize is valid (only means an unknown KAG). So I am inclined to simply use p->first_essence_klv.offset unconditionally, as you suggested in http://ffmpeg.org/pipermail/ffmpeg-devel/2018-February/225629.html See the attached patch. Regards, Marton From 6ff3c549fa78caeb8bfddfbee9961ecd59ca7c5f Mon Sep 17 00:00:00 2001 From: Marton Balint Date: Wed, 13 Jun 2018 21:46:34 +0200 Subject: [PATCH] avformat/mxfdec: simply use the first essence element for non frame-wrapped partition essence offset Also add the canopus essence element to the list of the recognized essence element keys. Signed-off-by: Marton Balint --- libavformat/mxfdec.c | 33 ++------------------------------- 1 file changed, 2 insertions(+), 31 deletions(-) diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c index 8f43ef46b9..2ec778e280 100644 --- a/libavformat/mxfdec.c +++ b/libavformat/mxfdec.c @@ -719,18 +719,6 @@ static int mxf_read_partition_pack(void *arg, AVIOContext *pb, int tag, int size mxf->op = OP1a; } - if (partition->kag_size <= 0 || partition->kag_size > (1 << 20)) { - av_log(mxf->fc, AV_LOG_WARNING, "invalid KAGSize %"PRId32" - guessing ", - partition->kag_size); - - if (mxf->op == OPSONYOpt) - partition->kag_size = 512; - else - partition->kag_size = 1; - - av_log(mxf->fc, AV_LOG_WARNING, "%"PRId32"\n", partition->kag_size); - } - return 0; } @@ -2800,14 +2788,6 @@ static int mxf_parse_handle_partition_or_eof(MXFContext *mxf) return mxf->parsing_backward ? mxf_seek_to_previous_partition(mxf) : 1; } -static int64_t round_to_kag(int64_t position, int kag_size) -{ - /* TODO: account for run-in? the spec isn't clear whether KAG should account for it */ - /* NOTE: kag_size may be any integer between 1 - 2^10 */ - int64_t ret = (position / kag_size) * kag_size; - return ret == position ? ret : ret + kag_size; -} - static MXFWrappingScheme mxf_get_wrapping_by_body_sid(AVFormatContext *s, int body_sid) { for (int i = 0; i < s->nb_streams; i++) { @@ -2843,17 +2823,7 @@ static void mxf_compute_essence_containers(AVFormatContext *s) p->essence_offset = p->first_essence_klv.next_klv - p->first_essence_klv.length; p->essence_length = p->first_essence_klv.length; } else { - int64_t op1a_essence_offset = - p->this_partition + - round_to_kag(p->pack_length, p->kag_size) + - round_to_kag(p->header_byte_count, p->kag_size) + - round_to_kag(p->index_byte_count, p->kag_size); - - /* NOTE: op1a_essence_offset may be less than to klv.offset (C0023S01.mxf) */ - if (IS_KLV_KEY(p->first_essence_klv.key, mxf_system_item_key_cp) || IS_KLV_KEY(p->first_essence_klv.key, mxf_system_item_key_gc)) - p->essence_offset = p->first_essence_klv.offset; - else - p->essence_offset = op1a_essence_offset; + p->essence_offset = p->first_essence_klv.offset; /* essence container spans to the next partition */ if (x < mxf->partitions_count - 1) @@ -3066,6 +3036,7 @@ static int mxf_read_header(AVFormatContext *s) av_log(s, AV_LOG_TRACE, "size %"PRIu64" offset %#"PRIx64"\n", klv.length, klv.offset); if (IS_KLV_KEY(klv.key, mxf_encrypted_triplet_key) || IS_KLV_KEY(klv.key, mxf_essence_element_key) || + IS_KLV_KEY(klv.key, mxf_canopus_essence_element_key) || IS_KLV_KEY(klv.key, mxf_avid_essence_element_key) || IS_KLV_KEY(klv.key, mxf_system_item_key_cp) || IS_KLV_KEY(klv.key, mxf_system_item_key_gc)) { -- 2.16.4