diff mbox series

[FFmpeg-devel,v6,1/3] avformat/mxfdec: set toolkit version metadata

Message ID 1612137055-19114-1-git-send-email-lance.lmwang@gmail.com
State Superseded
Headers show
Series [FFmpeg-devel,v6,1/3] avformat/mxfdec: set toolkit version metadata | expand

Checks

Context Check Description
andriy/x86_make_warn warning New warnings during build
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Lance Wang Jan. 31, 2021, 11:50 p.m. UTC 
From: Limin Wang <lance.lmwang@gmail.com>

Please check the string of toolkit version with below command:
./ffmpeg -i ../fate-suite/mxf/Sony-00001.mxf -c:v copy -c:a copy out.mxf
./ffmpeg -i out.mxf
....
toolkit_version : 58.65.101

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavformat/mxfdec.c                | 25 +++++++++++++++++++++++++
 tests/ref/fate/mxf-probe-applehdr10 |  1 +
 tests/ref/fate/mxf-probe-dnxhd      |  1 +
 3 files changed, 27 insertions(+)

Comments

Tomas Härdin Feb. 1, 2021, 10:56 a.m. UTC | #1 
mån 2021-02-01 klockan 07:50 +0800 skrev lance.lmwang@gmail.com:
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> Please check the string of toolkit version with below command:
> ./ffmpeg -i ../fate-suite/mxf/Sony-00001.mxf -c:v copy -c:a copy out.mxf
> ./ffmpeg -i out.mxf
> ....
> toolkit_version : 58.65.101
> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  libavformat/mxfdec.c                | 25 +++++++++++++++++++++++++
>  tests/ref/fate/mxf-probe-applehdr10 |  1 +
>  tests/ref/fate/mxf-probe-dnxhd      |  1 +
>  3 files changed, 27 insertions(+)
> 
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index afff204..61c8104 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -1970,6 +1970,18 @@ static int mxf_umid_to_str(UID ul, UID uid, char **str)
>      return 0;
>  }
>  
> +static int mxf_version_to_str(uint16_t major, uint16_t minor, uint16_t tertiary, char **str)
> +{
> +    int size = sizeof(major) * 5 + 1;

This is just wrong. Should be 3*5+2+1 = 18.

> +
> +    *str = av_mallocz(size);
> +    if (!*str)
> +        return AVERROR(ENOMEM);
> +
> +    snprintf(*str, size, "%d.%d.%d", major, minor, tertiary);

snprintf() is not safe - *str can end up not NUL terminated

> +#define SET_VERSION_METADATA(pb, name, major, minor, tertiary, str) do { \
> +    major = avio_rb16(pb); \
> +    minor = avio_rb16(pb); \
> +    tertiary = avio_rb16(pb); \
> +    if ((ret = mxf_version_to_str(major, minor, tertiary, &str)) < 0) \
> +        return ret; \
> +    av_dict_set(&s->metadata, name, str, AV_DICT_DONT_STRDUP_VAL); \
> +} while (0)

Why not a function?

/Tomas
Lance Wang Feb. 1, 2021, 2:47 p.m. UTC | #2 
On Mon, Feb 01, 2021 at 11:56:25AM +0100, Tomas Härdin wrote:
> mån 2021-02-01 klockan 07:50 +0800 skrev lance.lmwang@gmail.com:
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > Please check the string of toolkit version with below command:
> > ./ffmpeg -i ../fate-suite/mxf/Sony-00001.mxf -c:v copy -c:a copy out.mxf
> > ./ffmpeg -i out.mxf
> > ....
> > toolkit_version : 58.65.101
> > 
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> >  libavformat/mxfdec.c                | 25 +++++++++++++++++++++++++
> >  tests/ref/fate/mxf-probe-applehdr10 |  1 +
> >  tests/ref/fate/mxf-probe-dnxhd      |  1 +
> >  3 files changed, 27 insertions(+)
> > 
> > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > index afff204..61c8104 100644
> > --- a/libavformat/mxfdec.c
> > +++ b/libavformat/mxfdec.c
> > @@ -1970,6 +1970,18 @@ static int mxf_umid_to_str(UID ul, UID uid, char **str)
> >      return 0;
> >  }
> >  
> > +static int mxf_version_to_str(uint16_t major, uint16_t minor, uint16_t tertiary, char **str)
> > +{
> > +    int size = sizeof(major) * 5 + 1;
> 
> This is just wrong. Should be 3*5+2+1 = 18.

will fix it.

> 
> > +
> > +    *str = av_mallocz(size);
> > +    if (!*str)
> > +        return AVERROR(ENOMEM);
> > +
> > +    snprintf(*str, size, "%d.%d.%d", major, minor, tertiary);
> 
> snprintf() is not safe - *str can end up not NUL terminated

what's your suggestion?
I think allocate *str with +1 byte and zero it to sure the string will be NULL
terminated.

> 
> > +#define SET_VERSION_METADATA(pb, name, major, minor, tertiary, str) do { \
> > +    major = avio_rb16(pb); \
> > +    minor = avio_rb16(pb); \
> > +    tertiary = avio_rb16(pb); \
> > +    if ((ret = mxf_version_to_str(major, minor, tertiary, &str)) < 0) \
> > +        return ret; \
> > +    av_dict_set(&s->metadata, name, str, AV_DICT_DONT_STRDUP_VAL); \
> > +} while (0)
> 
> Why not a function?

Function is OK to me, I'm just following the same style for SET_STR_METADATA, SET_UID_METADATA,
SET_TS_METADATA.

> 
> /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".
Andreas Rheinhardt Feb. 1, 2021, 5:23 p.m. UTC | #3 
Tomas Härdin:
> mån 2021-02-01 klockan 07:50 +0800 skrev lance.lmwang@gmail.com:
>> From: Limin Wang <lance.lmwang@gmail.com>
>>
>> Please check the string of toolkit version with below command:
>> ./ffmpeg -i ../fate-suite/mxf/Sony-00001.mxf -c:v copy -c:a copy out.mxf
>> ./ffmpeg -i out.mxf
>> ....
>> toolkit_version : 58.65.101
>>
>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>> ---
>>  libavformat/mxfdec.c                | 25 +++++++++++++++++++++++++
>>  tests/ref/fate/mxf-probe-applehdr10 |  1 +
>>  tests/ref/fate/mxf-probe-dnxhd      |  1 +
>>  3 files changed, 27 insertions(+)
>>
>> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
>> index afff204..61c8104 100644
>> --- a/libavformat/mxfdec.c
>> +++ b/libavformat/mxfdec.c
>> @@ -1970,6 +1970,18 @@ static int mxf_umid_to_str(UID ul, UID uid, char **str)
>>      return 0;
>>  }
>>  
>> +static int mxf_version_to_str(uint16_t major, uint16_t minor, uint16_t tertiary, char **str)
>> +{
>> +    int size = sizeof(major) * 5 + 1;
> 
> This is just wrong. Should be 3*5+2+1 = 18.
> 
>> +
>> +    *str = av_mallocz(size);
>> +    if (!*str)
>> +        return AVERROR(ENOMEM);
>> +
>> +    snprintf(*str, size, "%d.%d.%d", major, minor, tertiary);
> 
> snprintf() is not safe - *str can end up not NUL terminated
> 
Not true -- snprintf always zero-terminates (and truncates the output if
it needs to do so) unless size is zero (which it is not).

But nevertheless av_asprintf would be better IMO.

- Andreas
Tomas Härdin Feb. 1, 2021, 7:14 p.m. UTC | #4 
mån 2021-02-01 klockan 18:23 +0100 skrev Andreas Rheinhardt:
> Tomas Härdin:
> > mån 2021-02-01 klockan 07:50 +0800 skrev lance.lmwang@gmail.com:
> > > From: Limin Wang <lance.lmwang@gmail.com>
> > > 
> > > Please check the string of toolkit version with below command:
> > > ./ffmpeg -i ../fate-suite/mxf/Sony-00001.mxf -c:v copy -c:a copy out.mxf
> > > ./ffmpeg -i out.mxf
> > > ....
> > > toolkit_version : 58.65.101
> > > 
> > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > ---
> > >  libavformat/mxfdec.c                | 25 +++++++++++++++++++++++++
> > >  tests/ref/fate/mxf-probe-applehdr10 |  1 +
> > >  tests/ref/fate/mxf-probe-dnxhd      |  1 +
> > >  3 files changed, 27 insertions(+)
> > > 
> > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > > index afff204..61c8104 100644
> > > --- a/libavformat/mxfdec.c
> > > +++ b/libavformat/mxfdec.c
> > > @@ -1970,6 +1970,18 @@ static int mxf_umid_to_str(UID ul, UID uid, char **str)
> > >      return 0;
> > >  }
> > >  
> > > +static int mxf_version_to_str(uint16_t major, uint16_t minor, uint16_t tertiary, char **str)
> > > +{
> > > +    int size = sizeof(major) * 5 + 1;
> > 
> > This is just wrong. Should be 3*5+2+1 = 18.
> > 
> > > +
> > > +    *str = av_mallocz(size);
> > > +    if (!*str)
> > > +        return AVERROR(ENOMEM);
> > > +
> > > +    snprintf(*str, size, "%d.%d.%d", major, minor, tertiary);
> > 
> > snprintf() is not safe - *str can end up not NUL terminated
> > 
> Not true -- snprintf always zero-terminates (and truncates the output if
> it needs to do so) unless size is zero (which it is not).

Ack, you're right. I was thinking of strn*

> But nevertheless av_asprintf would be better IMO.

Yes

/Tomas
Marton Balint Feb. 1, 2021, 7:15 p.m. UTC | #5 
On Mon, 1 Feb 2021, lance.lmwang@gmail.com wrote:

> From: Limin Wang <lance.lmwang@gmail.com>
>
> Please check the string of toolkit version with below command:
> ./ffmpeg -i ../fate-suite/mxf/Sony-00001.mxf -c:v copy -c:a copy out.mxf
> ./ffmpeg -i out.mxf
> ....
> toolkit_version : 58.65.101
>
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
> libavformat/mxfdec.c                | 25 +++++++++++++++++++++++++
> tests/ref/fate/mxf-probe-applehdr10 |  1 +
> tests/ref/fate/mxf-probe-dnxhd      |  1 +
> 3 files changed, 27 insertions(+)
>
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index afff204..61c8104 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -1970,6 +1970,18 @@ static int mxf_umid_to_str(UID ul, UID uid, char **str)
>     return 0;
> }
> 
> +static int mxf_version_to_str(uint16_t major, uint16_t minor, uint16_t tertiary, char **str)
> +{
> +    int size = sizeof(major) * 5 + 1;
> +
> +    *str = av_mallocz(size);
> +    if (!*str)
> +        return AVERROR(ENOMEM);
> +
> +    snprintf(*str, size, "%d.%d.%d", major, minor, tertiary);

ToolkitVersion is a ProductVersion which means it consists of 5 UInt16 
numbers, not 3. So you should present all 5 values.

Regards,
Marton
Lance Wang Feb. 2, 2021, 12:19 a.m. UTC | #6 
On Mon, Feb 01, 2021 at 08:15:33PM +0100, Marton Balint wrote:
> 
> 
> On Mon, 1 Feb 2021, lance.lmwang@gmail.com wrote:
> 
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > Please check the string of toolkit version with below command:
> > ./ffmpeg -i ../fate-suite/mxf/Sony-00001.mxf -c:v copy -c:a copy out.mxf
> > ./ffmpeg -i out.mxf
> > ....
> > toolkit_version : 58.65.101
> > 
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> > libavformat/mxfdec.c                | 25 +++++++++++++++++++++++++
> > tests/ref/fate/mxf-probe-applehdr10 |  1 +
> > tests/ref/fate/mxf-probe-dnxhd      |  1 +
> > 3 files changed, 27 insertions(+)
> > 
> > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > index afff204..61c8104 100644
> > --- a/libavformat/mxfdec.c
> > +++ b/libavformat/mxfdec.c
> > @@ -1970,6 +1970,18 @@ static int mxf_umid_to_str(UID ul, UID uid, char **str)
> >     return 0;
> > }
> > 
> > +static int mxf_version_to_str(uint16_t major, uint16_t minor, uint16_t tertiary, char **str)
> > +{
> > +    int size = sizeof(major) * 5 + 1;
> > +
> > +    *str = av_mallocz(size);
> > +    if (!*str)
> > +        return AVERROR(ENOMEM);
> > +
> > +    snprintf(*str, size, "%d.%d.%d", major, minor, tertiary);
> 
> ToolkitVersion is a ProductVersion which means it consists of 5 UInt16
> numbers, not 3. So you should present all 5 values.

OK, will add patch and release number also.

> 
> 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".
Lance Wang Feb. 2, 2021, 12:19 a.m. UTC | #7 
On Mon, Feb 01, 2021 at 08:14:25PM +0100, Tomas Härdin wrote:
> mån 2021-02-01 klockan 18:23 +0100 skrev Andreas Rheinhardt:
> > Tomas Härdin:
> > > mån 2021-02-01 klockan 07:50 +0800 skrev lance.lmwang@gmail.com:
> > > > From: Limin Wang <lance.lmwang@gmail.com>
> > > > 
> > > > Please check the string of toolkit version with below command:
> > > > ./ffmpeg -i ../fate-suite/mxf/Sony-00001.mxf -c:v copy -c:a copy out.mxf
> > > > ./ffmpeg -i out.mxf
> > > > ....
> > > > toolkit_version : 58.65.101
> > > > 
> > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > > ---
> > > >  libavformat/mxfdec.c                | 25 +++++++++++++++++++++++++
> > > >  tests/ref/fate/mxf-probe-applehdr10 |  1 +
> > > >  tests/ref/fate/mxf-probe-dnxhd      |  1 +
> > > >  3 files changed, 27 insertions(+)
> > > > 
> > > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > > > index afff204..61c8104 100644
> > > > --- a/libavformat/mxfdec.c
> > > > +++ b/libavformat/mxfdec.c
> > > > @@ -1970,6 +1970,18 @@ static int mxf_umid_to_str(UID ul, UID uid, char **str)
> > > >      return 0;
> > > >  }
> > > >  
> > > > +static int mxf_version_to_str(uint16_t major, uint16_t minor, uint16_t tertiary, char **str)
> > > > +{
> > > > +    int size = sizeof(major) * 5 + 1;
> > > 
> > > This is just wrong. Should be 3*5+2+1 = 18.
> > > 
> > > > +
> > > > +    *str = av_mallocz(size);
> > > > +    if (!*str)
> > > > +        return AVERROR(ENOMEM);
> > > > +
> > > > +    snprintf(*str, size, "%d.%d.%d", major, minor, tertiary);
> > > 
> > > snprintf() is not safe - *str can end up not NUL terminated
> > > 
> > Not true -- snprintf always zero-terminates (and truncates the output if
> > it needs to do so) unless size is zero (which it is not).
> 
> Ack, you're right. I was thinking of strn*
> 
> > But nevertheless av_asprintf would be better IMO.
> 
> Yes

sure, will change to use av_asprintf.

> 
> /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".
diff mbox series

Patch

diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index afff204..61c8104 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -1970,6 +1970,18 @@  static int mxf_umid_to_str(UID ul, UID uid, char **str)
     return 0;
 }
 
+static int mxf_version_to_str(uint16_t major, uint16_t minor, uint16_t tertiary, char **str)
+{
+    int size = sizeof(major) * 5 + 1;
+
+    *str = av_mallocz(size);
+    if (!*str)
+        return AVERROR(ENOMEM);
+
+    snprintf(*str, size, "%d.%d.%d", major, minor, tertiary);
+    return 0;
+}
+
 static int mxf_add_umid_metadata(AVDictionary **pm, const char *key, MXFPackage* package)
 {
     char *str;
@@ -2731,6 +2743,15 @@  static int64_t mxf_timestamp_to_int64(uint64_t timestamp)
     av_dict_set(&s->metadata, name, str, AV_DICT_DONT_STRDUP_VAL); \
 } while (0)
 
+#define SET_VERSION_METADATA(pb, name, major, minor, tertiary, str) do { \
+    major = avio_rb16(pb); \
+    minor = avio_rb16(pb); \
+    tertiary = avio_rb16(pb); \
+    if ((ret = mxf_version_to_str(major, minor, tertiary, &str)) < 0) \
+        return ret; \
+    av_dict_set(&s->metadata, name, str, AV_DICT_DONT_STRDUP_VAL); \
+} while (0)
+
 #define SET_UID_METADATA(pb, name, var, str) do { \
     avio_read(pb, var, 16); \
     if ((ret = mxf_uid_to_str(var, &str)) < 0) \
@@ -2752,6 +2773,7 @@  static int mxf_read_identification_metadata(void *arg, AVIOContext *pb, int tag,
     UID uid = { 0 };
     char *str = NULL;
     uint64_t ts;
+    uint16_t major, minor, tertiary;
     switch (tag) {
     case 0x3C01:
         SET_STR_METADATA(pb, "company_name", str);
@@ -2768,6 +2790,9 @@  static int mxf_read_identification_metadata(void *arg, AVIOContext *pb, int tag,
     case 0x3C06:
         SET_TS_METADATA(pb, "modification_date", ts, str);
         break;
+    case 0x3C07:
+        SET_VERSION_METADATA(pb, "toolkit_version", major, minor, tertiary, str);
+        break;
     case 0x3C08:
         SET_STR_METADATA(pb, "application_platform", str);
         break;
diff --git a/tests/ref/fate/mxf-probe-applehdr10 b/tests/ref/fate/mxf-probe-applehdr10
index 53a767c..924850b 100644
--- a/tests/ref/fate/mxf-probe-applehdr10
+++ b/tests/ref/fate/mxf-probe-applehdr10
@@ -164,6 +164,7 @@  TAG:product_name=Compressor
 TAG:product_version=4.4.7 (4.4.7)
 TAG:product_uid=00000000-0000-0000-0000-000000000000
 TAG:modification_date=2020-09-08T16:18:57.036000Z
+TAG:toolkit_version=3.8.0
 TAG:material_package_umid=0x060A2B340101010501010D201300000045843C9FE69D4B8FA90DDAAA1602A2E8
 TAG:timecode=00:01:15;26
 [/FORMAT]
diff --git a/tests/ref/fate/mxf-probe-dnxhd b/tests/ref/fate/mxf-probe-dnxhd
index 5a6221b..dad6417 100644
--- a/tests/ref/fate/mxf-probe-dnxhd
+++ b/tests/ref/fate/mxf-probe-dnxhd
@@ -173,6 +173,7 @@  TAG:project_name=UHD
 TAG:uid=784c8132-ae36-ed4d-b0ff-2edf1f3f2d92
 TAG:generation_uid=b6bcfcab-70ff-7331-c47c-478869de11d2
 TAG:application_platform=AAFSDK (MacOS X)
+TAG:toolkit_version=1.1.0
 TAG:modification_date=2016-09-18T19:25:25.000000Z
 TAG:product_uid=acfbf03a-4f42-a231-d0b7-c06ecd3d4ad7
 TAG:product_version=Unknown version