[FFmpeg-devel,04/12] avformat/mxfdec: compute both essence_offset and essence_length in mxf_compute_essence_containers

Submitted by Marton Balint on June 13, 2018, 7:58 p.m.

Details

Message ID alpine.LSU.2.20.1806132049270.17215@iq
State New
Headers show

Commit Message

Marton Balint June 13, 2018, 7:58 p.m.
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 <cus@passwd.hu>
>> ---
>>  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

Comments

Tomas Härdin June 14, 2018, 12:31 p.m.
ons 2018-06-13 klockan 21:58 +0200 skrev Marton Balint:
> 
> 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 <cus@passwd.hu>
> > > 
> > > ---
> > >  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

Yes, past me seems to have the right idea :)

> See the attached patch.

I think we should still parse kag_size even if we don't end up using it
for much. Especially since there's special cases for Sony files in
there. Did we have a sample for that? I forget.

Are there cases where we might not run across an essence KLV during
scanning but where there would be one anyway? I suspect not, not even
if we're reading an unseekable file

/Tomas
Marton Balint June 14, 2018, 8:16 p.m.
On Thu, 14 Jun 2018, Tomas Härdin wrote:

[...]

>> > > +        } 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
>
> Yes, past me seems to have the right idea :)
>
>> See the attached patch.
>
> I think we should still parse kag_size even if we don't end up using it
> for much. Especially since there's special cases for Sony files in
> there. Did we have a sample for that? I forget.

fate-suite/mxf/C0023S01.mxf

>
> Are there cases where we might not run across an essence KLV during
> scanning but where there would be one anyway? I suspect not, not even
> if we're reading an unseekable file

No, I don't think so either.

Regards,
Marton

Patch hide | download patch | download mbox

From 6ff3c549fa78caeb8bfddfbee9961ecd59ca7c5f Mon Sep 17 00:00:00 2001
From: Marton Balint <cus@passwd.hu>
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 <cus@passwd.hu>
---
 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