[FFmpeg-devel] [PATCH 1/2] avformat/mxfdec: move resolving Descriptors to the multi descriptor resolve function

[FFmpeg-devel] [PATCH 1/2] avformat/mxfdec: move resolving Descriptors to the multi descriptor resolve function

[Marton Balint]

Also remove unused descriptor member from MXFPackage.

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

diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index e42975e7fd..4e4e3e7a84 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -266,7 +266,6 @@ typedef struct MXFPackage {
     UID package_ul;
     UID *tracks_refs;
     int tracks_count;
-    MXFDescriptor *descriptor; /* only one */
     UID descriptor_ref;
     char *name;
     UID *comment_refs;
@@ -2257,11 +2256,12 @@ static MXFPackage* mxf_resolve_source_package(MXFContext *mxf, UID package_ul, U
     return NULL;
 }
 
-static MXFDescriptor* mxf_resolve_multidescriptor(MXFContext *mxf, MXFDescriptor *descriptor, int track_id)
+static MXFDescriptor* mxf_resolve_descriptor(MXFContext *mxf, UID *strong_ref, int track_id)
 {
-    MXFDescriptor *file_descriptor = NULL;
+    MXFDescriptor *descriptor, *file_descriptor = NULL;
     int i;
 
+    descriptor = mxf_resolve_strong_ref(mxf, strong_ref, AnyType);
     if (!descriptor)
         return NULL;
 
@@ -2782,8 +2782,7 @@ static int mxf_parse_structural_metadata(MXFContext *mxf)
         st->id = material_track->track_id;
         st->priv_data = source_track;
 
-        source_package->descriptor = mxf_resolve_strong_ref(mxf, &source_package->descriptor_ref, AnyType);
-        descriptor = mxf_resolve_multidescriptor(mxf, source_package->descriptor, source_track->track_id);
+        descriptor = mxf_resolve_descriptor(mxf, &source_package->descriptor_ref, source_track->track_id);
 
         /* A SourceClip from a EssenceGroup may only be a single frame of essence data. The clips duration is then how many
          * frames its suppose to repeat for. Descriptor->duration, if present, contains the real duration of the essence data */
-- 
2.35.3

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

[FFmpeg-devel] [PATCH 2/2] avformat/mxfdec: do not use AnyType when resolving Descriptors and MultipleDescriptors

[Marton Balint]

Using AnyType should not be a problem for proper MXF files because UIDs are
supposed to be unique themselves. Unfortunately that is not the case for some
broken files, so let's check the type more strictly.

Fixes ticket #10865.

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

diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index 4e4e3e7a84..446bcf3276 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -2258,16 +2258,14 @@ static MXFPackage* mxf_resolve_source_package(MXFContext *mxf, UID package_ul, U
 
 static MXFDescriptor* mxf_resolve_descriptor(MXFContext *mxf, UID *strong_ref, int track_id)
 {
-    MXFDescriptor *descriptor, *file_descriptor = NULL;
-    int i;
-
-    descriptor = mxf_resolve_strong_ref(mxf, strong_ref, AnyType);
-    if (!descriptor)
-        return NULL;
+    MXFDescriptor *descriptor = mxf_resolve_strong_ref(mxf, strong_ref, Descriptor);
+    if (descriptor)
+        return descriptor;
 
-    if (descriptor->meta.type == MultipleDescriptor) {
-        for (i = 0; i < descriptor->file_descriptors_count; i++) {
-            file_descriptor = mxf_resolve_strong_ref(mxf, &descriptor->file_descriptors_refs[i], Descriptor);
+    descriptor = mxf_resolve_strong_ref(mxf, strong_ref, MultipleDescriptor);
+    if (descriptor) {
+        for (int i = 0; i < descriptor->file_descriptors_count; i++) {
+            MXFDescriptor *file_descriptor = mxf_resolve_strong_ref(mxf, &descriptor->file_descriptors_refs[i], Descriptor);
 
             if (!file_descriptor) {
                 av_log(mxf->fc, AV_LOG_ERROR, "could not resolve file descriptor strong ref\n");
@@ -2277,8 +2275,7 @@ static MXFDescriptor* mxf_resolve_descriptor(MXFContext *mxf, UID *strong_ref, i
                 return file_descriptor;
             }
         }
-    } else if (descriptor->meta.type == Descriptor)
-        return descriptor;
+    }
 
     return NULL;
 }
-- 
2.35.3

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 1/2] avformat/mxfdec: move resolving Descriptors to the multi descriptor resolve function

[Tomas Härdin]

fre 2024-02-16 klockan 22:18 +0100 skrev Marton Balint:
> Also remove unused descriptor member from MXFPackage.
> 
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  libavformat/mxfdec.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)

Looks OK

/Tomas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] avformat/mxfdec: do not use AnyType when resolving Descriptors and MultipleDescriptors

[Tomas Härdin]

fre 2024-02-16 klockan 22:18 +0100 skrev Marton Balint:
> Using AnyType should not be a problem for proper MXF files because
> UIDs are
> supposed to be unique themselves. Unfortunately that is not the case
> for some
> broken files, so let's check the type more strictly.

Here's what S377m-2004 says:

> StrongRef: “One-to-one” relationship between sets and implemented in
> MXF with UUIDs. Strong references
> are typed which means that the definition identifies the kind of set
> which is the target of the reference.


So non-unique UUIDs are fine so long as their type sets them apart.
Therefore we should not use AnyType unless there is a good reason to do
so. UMID lookup would be such a reason. Resolving SourceClips and
TimeCodeComponents are not. And since we don't do any UMID lookup we
could just as well ditch AnyType entirely.

> Fixes ticket #10865.
> 
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  libavformat/mxfdec.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index 4e4e3e7a84..446bcf3276 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -2258,16 +2258,14 @@ static MXFPackage*
> mxf_resolve_source_package(MXFContext *mxf, UID package_ul, U
>  
>  static MXFDescriptor* mxf_resolve_descriptor(MXFContext *mxf, UID
> *strong_ref, int track_id)
>  {
> -    MXFDescriptor *descriptor, *file_descriptor = NULL;
> -    int i;

Mixing functional and style changes make the patch annoying to read.
Anyway it looks fine

/Tomas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 2/2] avformat/mxfdec: do not use AnyType when resolving Descriptors and MultipleDescriptors

[Marton Balint]

On Mon, 19 Feb 2024, Tomas Härdin wrote:

> fre 2024-02-16 klockan 22:18 +0100 skrev Marton Balint:
>> Using AnyType should not be a problem for proper MXF files because
>> UIDs are
>> supposed to be unique themselves. Unfortunately that is not the case
>> for some
>> broken files, so let's check the type more strictly.
>
> Here's what S377m-2004 says:
>
>> StrongRef: “One-to-one” relationship between sets and implemented in
>> MXF with UUIDs. Strong references
>> are typed which means that the definition identifies the kind of set
>> which is the target of the reference.
>
>
> So non-unique UUIDs are fine so long as their type sets them apart.
> Therefore we should not use AnyType unless there is a good reason to do
> so. UMID lookup would be such a reason. Resolving SourceClips and
> TimeCodeComponents are not. And since we don't do any UMID lookup we
> could just as well ditch AnyType entirely.

Thanks, I was not aware of this, with MXF, you can never be sure :)

I have already prepared patches to get rid of AnyType, will send it soon.

>
>> Fixes ticket #10865.
>> 
>> Signed-off-by: Marton Balint <cus@passwd.hu>
>> ---
>>  libavformat/mxfdec.c | 19 ++++++++-----------
>>  1 file changed, 8 insertions(+), 11 deletions(-)
>> 
>> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
>> index 4e4e3e7a84..446bcf3276 100644
>> --- a/libavformat/mxfdec.c
>> +++ b/libavformat/mxfdec.c
>> @@ -2258,16 +2258,14 @@ static MXFPackage*
>> mxf_resolve_source_package(MXFContext *mxf, UID package_ul, U
>>  
>>  static MXFDescriptor* mxf_resolve_descriptor(MXFContext *mxf, UID
>> *strong_ref, int track_id)
>>  {
>> -    MXFDescriptor *descriptor, *file_descriptor = NULL;
>> -    int i;
>
> Mixing functional and style changes make the patch annoying to read.
> Anyway it looks fine

Sorry, it was such a tiny change, I did not think it would matter in 
practice.

Thanks for your feedback, I will push this set and send a follow up.

Regards,
Marton
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".