[FFmpeg-devel,3/5] avformat/mxfdec: guess wrapping of tracks by other tracks with the same body sid

Submitted by Marton Balint on April 11, 2019, 11:09 p.m.

Details

Message ID 20190411230920.6630-3-cus@passwd.hu
State New
Headers show

Commit Message

Marton Balint April 11, 2019, 11:09 p.m.
This affects the following samples:

samples/ffmpeg-bugs/roundup/issue1775/av_seek_frame_failure.mxf
samples/ffmpeg-bugs/trac/ticket1957/16ch.mxf
samples/ffmpeg-bugs/trac/ticket5016/r0.mxf
samples/ffmpeg-bugs/trac/ticket5016/r1.mxf
samples/ffmpeg-bugs/trac/ticket5316/hq.MXF
samples/ffmpeg-bugs/trac/ticket5316/hqx.MXF

Some AVPacket->pos values are changed because for frame wrapped tracks we point
to the KLV offset and not the data.

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavformat/mxfdec.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

Comments

Tomas Härdin April 14, 2019, 3:58 p.m.
fre 2019-04-12 klockan 01:09 +0200 skrev Marton Balint:
> This affects the following samples:
> 
> samples/ffmpeg-bugs/roundup/issue1775/av_seek_frame_failure.mxf
> samples/ffmpeg-bugs/trac/ticket1957/16ch.mxf
> samples/ffmpeg-bugs/trac/ticket5016/r0.mxf
> samples/ffmpeg-bugs/trac/ticket5016/r1.mxf
> samples/ffmpeg-bugs/trac/ticket5316/hq.MXF
> samples/ffmpeg-bugs/trac/ticket5316/hqx.MXF
> 
> Some AVPacket->pos values are changed because for frame wrapped tracks we point
> to the KLV offset and not the data.
> 
> > Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  libavformat/mxfdec.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index 18c038c3f6..236294880e 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -2144,6 +2144,16 @@ static int mxf_add_metadata_stream(MXFContext *mxf, MXFTrack *track)
>      return 0;
>  }
>  
> +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;

Maybe print a warning? Especially if, god forbid, the file mixes
ClipWrapping and FrameWrapping..

But now I see this function was merely moved further up, so that could
be a separate patch..

> @@ -2553,6 +2563,12 @@ static int mxf_parse_structural_metadata(MXFContext *mxf)
>          }
>      }
>  
> +    for (int i = 0; i < mxf->fc->nb_streams; i++) {
> +        MXFTrack *track = mxf->fc->streams[i]->priv_data;
> +        if (track && track->body_sid && track->wrapping == UnknownWrapped)
> +            track->wrapping = mxf_get_wrapping_by_body_sid(mxf->fc, track->body_sid);
> +    }

Or maybe put a warning here instead. Vendors should fix their dang MXF
muxers..

/Tomas
Baptiste Coudurier April 14, 2019, 5:05 p.m.
Hi Tomas, I hope you are doing well

> On Apr 14, 2019, at 8:58 AM, Tomas Härdin <tjoppen@acc.umu.se> wrote:
> 
> […]
> 
>> @@ -2553,6 +2563,12 @@ static int mxf_parse_structural_metadata(MXFContext *mxf)
>>          }
>>      }
>> 
>> +    for (int i = 0; i < mxf->fc->nb_streams; i++) {
>> +        MXFTrack *track = mxf->fc->streams[i]->priv_data;
>> +        if (track && track->body_sid && track->wrapping == UnknownWrapped)
>> +            track->wrapping = mxf_get_wrapping_by_body_sid(mxf->fc, track->body_sid);
>> +    }
> 
> Or maybe put a warning here instead. Vendors should fix their dang MXF
> muxers..


Yeah…

Since MXF is a container exclusively used in a professional environment, I believe we could be
way more strict than other formats. I also feel we don’t have to support older files that were
often broken.

—
Baptiste
Marton Balint April 14, 2019, 5:29 p.m.
On Sun, 14 Apr 2019, Tomas Härdin wrote:

> fre 2019-04-12 klockan 01:09 +0200 skrev Marton Balint:
>> This affects the following samples:
>> 
>> samples/ffmpeg-bugs/roundup/issue1775/av_seek_frame_failure.mxf
>> samples/ffmpeg-bugs/trac/ticket1957/16ch.mxf
>> samples/ffmpeg-bugs/trac/ticket5016/r0.mxf
>> samples/ffmpeg-bugs/trac/ticket5016/r1.mxf
>> samples/ffmpeg-bugs/trac/ticket5316/hq.MXF
>> samples/ffmpeg-bugs/trac/ticket5316/hqx.MXF
>> 
>> Some AVPacket->pos values are changed because for frame wrapped tracks we point
>> to the KLV offset and not the data.
>> 
>> > Signed-off-by: Marton Balint <cus@passwd.hu>
>> ---
>>  libavformat/mxfdec.c | 26 ++++++++++++++++----------
>>  1 file changed, 16 insertions(+), 10 deletions(-)
>> 
>> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
>> index 18c038c3f6..236294880e 100644
>> --- a/libavformat/mxfdec.c
>> +++ b/libavformat/mxfdec.c
>> @@ -2144,6 +2144,16 @@ static int mxf_add_metadata_stream(MXFContext *mxf, MXFTrack *track)
>>      return 0;
>>  }
>>  
>> +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;
>
> Maybe print a warning? Especially if, god forbid, the file mixes
> ClipWrapping and FrameWrapping..
>
> But now I see this function was merely moved further up, so that could
> be a separate patch..
>
>> @@ -2553,6 +2563,12 @@ static int mxf_parse_structural_metadata(MXFContext *mxf)
>>          }
>>      }
>>  
>> +    for (int i = 0; i < mxf->fc->nb_streams; i++) {
>> +        MXFTrack *track = mxf->fc->streams[i]->priv_data;
>> +        if (track && track->body_sid && track->wrapping == UnknownWrapped)
>> +            track->wrapping = mxf_get_wrapping_by_body_sid(mxf->fc, track->body_sid);
>> +    }
>
> Or maybe put a warning here instead. Vendors should fix their dang MXF
> muxers..

We already have a warning (info) if we encounter a stream with unknown 
wrapping in mxf_structural_metadata.

Based on the samples that are affected by this, wrapping is assumed 
unknown for some GrassValley HQ/HQX files, probably we should fix 
our own demuxer here and assume frame wrapped. Although we have no idea if 
clip wrapped HQ/HQX exists...

The other one is some Omneon files with uncompressed audio in "AAF 
AIFF-AIFC Audio Container". This can just as easily be frame wrapped as 
clip wrapped, no?

Thanks,
Marton

Patch hide | download patch | download mbox

diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index 18c038c3f6..236294880e 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -2144,6 +2144,16 @@  static int mxf_add_metadata_stream(MXFContext *mxf, MXFTrack *track)
     return 0;
 }
 
+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;
+}
+
 static int mxf_parse_structural_metadata(MXFContext *mxf)
 {
     MXFPackage *material_package = NULL;
@@ -2553,6 +2563,12 @@  static int mxf_parse_structural_metadata(MXFContext *mxf)
         }
     }
 
+    for (int i = 0; i < mxf->fc->nb_streams; i++) {
+        MXFTrack *track = mxf->fc->streams[i]->priv_data;
+        if (track && track->body_sid && track->wrapping == UnknownWrapped)
+            track->wrapping = mxf_get_wrapping_by_body_sid(mxf->fc, track->body_sid);
+    }
+
     ret = 0;
 fail_and_free:
     return ret;
@@ -2913,16 +2929,6 @@  static int mxf_parse_handle_partition_or_eof(MXFContext *mxf)
     return mxf->parsing_backward ? mxf_seek_to_previous_partition(mxf) : 1;
 }
 
-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
  */