diff mbox series

[FFmpeg-devel,v4,3/3] avformat/mxfenc: prefer to use the configured metadta

Message ID 1610168848-17933-3-git-send-email-lance.lmwang@gmail.com
State New
Headers show
Series [FFmpeg-devel,v4,1/3] avformat/udp: return the error code instead of generic EIO
Related show

Checks

Context Check Description
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

Limin Wang Jan. 9, 2021, 5:07 a.m. UTC 
From: Limin Wang <lance.lmwang@gmail.com>

The metadata company_name, product_name, product_version from input
file will be deleted to avoid overwriting information
Please to test with below command:
./ffmpeg -i ../fate-suite/mxf/Sony-00001.mxf -c:v copy -c:a copy out.mxf
and
./ffmpeg -i ../fate-suite/mxf/Sony-00001.mxf -c:v copy -c:a copy \
        -metadata company_name="xxx" \
        -metadata product_name="xxx" \
        -metadata product_version="xxx" \
        out.mxf

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 fftools/ffmpeg_opt.c |  3 +++
 libavformat/mxfenc.c | 12 ++++++++----
 2 files changed, 11 insertions(+), 4 deletions(-)

Comments

Tomas Härdin Jan. 15, 2021, 8:56 a.m. UTC | #1 
lör 2021-01-09 klockan 13:07 +0800 skrev lance.lmwang@gmail.com:
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> The metadata company_name, product_name, product_version from input
> file will be deleted to avoid overwriting information
> Please to test with below command:
> ./ffmpeg -i ../fate-suite/mxf/Sony-00001.mxf -c:v copy -c:a copy out.mxf
> and
> ./ffmpeg -i ../fate-suite/mxf/Sony-00001.mxf -c:v copy -c:a copy \
>         -metadata company_name="xxx" \
>         -metadata product_name="xxx" \
>         -metadata product_version="xxx" \
>         out.mxf
> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  fftools/ffmpeg_opt.c |  3 +++
>  libavformat/mxfenc.c | 12 ++++++++----
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
> index c295514..493763b 100644
> --- a/fftools/ffmpeg_opt.c
> +++ b/fftools/ffmpeg_opt.c
> @@ -2650,6 +2650,9 @@ loop_end:
>          if(o->recording_time != INT64_MAX)
>              av_dict_set(&oc->metadata, "duration", NULL, 0);
>          av_dict_set(&oc->metadata, "creation_time", NULL, 0);
> +        av_dict_set(&oc->metadata, "company_name", NULL, 0);
> +        av_dict_set(&oc->metadata, "product_name", NULL, 0);
> +        av_dict_set(&oc->metadata, "product_version", NULL, 0);
>      }
>      if (!o->metadata_streams_manual)
>          for (i = of->ost_index; i < nb_output_streams; i++) {
> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> index d8678c9..5244211 100644
> --- a/libavformat/mxfenc.c
> +++ b/libavformat/mxfenc.c
> @@ -722,16 +722,20 @@ static void mxf_write_identification(AVFormatContext *s)
>  {
>      MXFContext *mxf = s->priv_data;
>      AVIOContext *pb = s->pb;
> -    const char *company = "FFmpeg";
> -    const char *product = s->oformat != &ff_mxf_opatom_muxer ? "OP1a Muxer" : "OPAtom Muxer";
> -    const char *version;
> +    AVDictionaryEntry *com_entry =  av_dict_get(s->metadata, "company_name", NULL, 0);
> +    AVDictionaryEntry *product_entry =  av_dict_get(s->metadata, "product_name", NULL, 0);
> +    AVDictionaryEntry *version_entry =  av_dict_get(s->metadata, "product_version", NULL, 0);
> +    const char *company = com_entry ? com_entry->value : "FFmpeg";
> +    const char *product = product_entry ? product_entry->value : s->oformat != &ff_mxf_opatom_muxer ? "OP1a Muxer" : "OPAtom Muxer";
> +    const char *version = NULL;
> +    const char *product_version = version_entry ? version_entry->value : AV_STRINGIFY(LIBAVFORMAT_VERSION);
>      int length;
>  
>      mxf_write_metadata_key(pb, 0x013000);
>      PRINT_KEY(s, "identification key", pb->buf_ptr - 16);
>  
>      version = s->flags & AVFMT_FLAG_BITEXACT ?
> -        "0.0.0" : AV_STRINGIFY(LIBAVFORMAT_VERSION);
> +        "0.0.0" : product_version;

Again, why? If you have a company that maintains a fork of FFmpeg then
compile that info in here instead. Compare with FFmbc which always puts
"FFmbc" as CompanyName.

/Tomas
Marton Balint Jan. 15, 2021, 8:43 p.m. UTC | #2 
On Fri, 15 Jan 2021, Tomas Härdin wrote:

> lör 2021-01-09 klockan 13:07 +0800 skrev lance.lmwang@gmail.com:
>> From: Limin Wang <lance.lmwang@gmail.com>
>> 
>> The metadata company_name, product_name, product_version from input
>> file will be deleted to avoid overwriting information
>> Please to test with below command:
>> ./ffmpeg -i ../fate-suite/mxf/Sony-00001.mxf -c:v copy -c:a copy out.mxf
>> and
>> ./ffmpeg -i ../fate-suite/mxf/Sony-00001.mxf -c:v copy -c:a copy \
>>         -metadata company_name="xxx" \
>>         -metadata product_name="xxx" \
>>         -metadata product_version="xxx" \
>>         out.mxf
>> 
>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>> ---
>>  fftools/ffmpeg_opt.c |  3 +++
>>  libavformat/mxfenc.c | 12 ++++++++----
>>  2 files changed, 11 insertions(+), 4 deletions(-)
>> 
>> diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
>> index c295514..493763b 100644
>> --- a/fftools/ffmpeg_opt.c
>> +++ b/fftools/ffmpeg_opt.c
>> @@ -2650,6 +2650,9 @@ loop_end:
>>          if(o->recording_time != INT64_MAX)
>>              av_dict_set(&oc->metadata, "duration", NULL, 0);
>>          av_dict_set(&oc->metadata, "creation_time", NULL, 0);
>> +        av_dict_set(&oc->metadata, "company_name", NULL, 0);
>> +        av_dict_set(&oc->metadata, "product_name", NULL, 0);
>> +        av_dict_set(&oc->metadata, "product_version", NULL, 0);
>>      }
>>      if (!o->metadata_streams_manual)
>>          for (i = of->ost_index; i < nb_output_streams; i++) {
>> diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
>> index d8678c9..5244211 100644
>> --- a/libavformat/mxfenc.c
>> +++ b/libavformat/mxfenc.c
>> @@ -722,16 +722,20 @@ static void mxf_write_identification(AVFormatContext *s)
>>  {
>>      MXFContext *mxf = s->priv_data;
>>      AVIOContext *pb = s->pb;
>> -    const char *company = "FFmpeg";
>> -    const char *product = s->oformat != &ff_mxf_opatom_muxer ? "OP1a Muxer" : "OPAtom Muxer";
>> -    const char *version;
>> +    AVDictionaryEntry *com_entry =  av_dict_get(s->metadata, "company_name", NULL, 0);
>> +    AVDictionaryEntry *product_entry =  av_dict_get(s->metadata, "product_name", NULL, 0);
>> +    AVDictionaryEntry *version_entry =  av_dict_get(s->metadata, "product_version", NULL, 0);
>> +    const char *company = com_entry ? com_entry->value : "FFmpeg";
>> +    const char *product = product_entry ? product_entry->value : s->oformat != &ff_mxf_opatom_muxer ? "OP1a Muxer" : "OPAtom Muxer";
>> +    const char *version = NULL;
>> +    const char *product_version = version_entry ? version_entry->value : AV_STRINGIFY(LIBAVFORMAT_VERSION);
>>      int length;
>>
>>      mxf_write_metadata_key(pb, 0x013000);
>>      PRINT_KEY(s, "identification key", pb->buf_ptr - 16);
>>
>>      version = s->flags & AVFMT_FLAG_BITEXACT ?
>> -        "0.0.0" : AV_STRINGIFY(LIBAVFORMAT_VERSION);
>> +        "0.0.0" : product_version;
>
> Again, why? If you have a company that maintains a fork of FFmpeg then
> compile that info in here instead. Compare with FFmbc which always puts
> "FFmbc" as CompanyName.

And how can a product based on libavformat set the company name, product 
name and product version? It seems a valid use case for me that these are 
overridable. Also note that this product version is only the "user 
friendly" version string, for the numeric version still 
LIBAVFORMAT_VERSION values are used.

Regards,
Marton
Limin Wang Jan. 16, 2021, 12:43 a.m. UTC | #3 
On Fri, Jan 15, 2021 at 09:43:58PM +0100, Marton Balint wrote:
> 
> 
> On Fri, 15 Jan 2021, Tomas Härdin wrote:
> 
> > lör 2021-01-09 klockan 13:07 +0800 skrev lance.lmwang@gmail.com:
> > > From: Limin Wang <lance.lmwang@gmail.com>
> > > 
> > > The metadata company_name, product_name, product_version from input
> > > file will be deleted to avoid overwriting information
> > > Please to test with below command:
> > > ./ffmpeg -i ../fate-suite/mxf/Sony-00001.mxf -c:v copy -c:a copy out.mxf
> > > and
> > > ./ffmpeg -i ../fate-suite/mxf/Sony-00001.mxf -c:v copy -c:a copy \
> > >         -metadata company_name="xxx" \
> > >         -metadata product_name="xxx" \
> > >         -metadata product_version="xxx" \
> > >         out.mxf
> > > 
> > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > ---
> > >  fftools/ffmpeg_opt.c |  3 +++
> > >  libavformat/mxfenc.c | 12 ++++++++----
> > >  2 files changed, 11 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
> > > index c295514..493763b 100644
> > > --- a/fftools/ffmpeg_opt.c
> > > +++ b/fftools/ffmpeg_opt.c
> > > @@ -2650,6 +2650,9 @@ loop_end:
> > >          if(o->recording_time != INT64_MAX)
> > >              av_dict_set(&oc->metadata, "duration", NULL, 0);
> > >          av_dict_set(&oc->metadata, "creation_time", NULL, 0);
> > > +        av_dict_set(&oc->metadata, "company_name", NULL, 0);
> > > +        av_dict_set(&oc->metadata, "product_name", NULL, 0);
> > > +        av_dict_set(&oc->metadata, "product_version", NULL, 0);
> > >      }
> > >      if (!o->metadata_streams_manual)
> > >          for (i = of->ost_index; i < nb_output_streams; i++) {
> > > diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
> > > index d8678c9..5244211 100644
> > > --- a/libavformat/mxfenc.c
> > > +++ b/libavformat/mxfenc.c
> > > @@ -722,16 +722,20 @@ static void mxf_write_identification(AVFormatContext *s)
> > >  {
> > >      MXFContext *mxf = s->priv_data;
> > >      AVIOContext *pb = s->pb;
> > > -    const char *company = "FFmpeg";
> > > -    const char *product = s->oformat != &ff_mxf_opatom_muxer ? "OP1a Muxer" : "OPAtom Muxer";
> > > -    const char *version;
> > > +    AVDictionaryEntry *com_entry =  av_dict_get(s->metadata, "company_name", NULL, 0);
> > > +    AVDictionaryEntry *product_entry =  av_dict_get(s->metadata, "product_name", NULL, 0);
> > > +    AVDictionaryEntry *version_entry =  av_dict_get(s->metadata, "product_version", NULL, 0);
> > > +    const char *company = com_entry ? com_entry->value : "FFmpeg";
> > > +    const char *product = product_entry ? product_entry->value : s->oformat != &ff_mxf_opatom_muxer ? "OP1a Muxer" : "OPAtom Muxer";
> > > +    const char *version = NULL;
> > > +    const char *product_version = version_entry ? version_entry->value : AV_STRINGIFY(LIBAVFORMAT_VERSION);
> > >      int length;
> > > 
> > >      mxf_write_metadata_key(pb, 0x013000);
> > >      PRINT_KEY(s, "identification key", pb->buf_ptr - 16);
> > > 
> > >      version = s->flags & AVFMT_FLAG_BITEXACT ?
> > > -        "0.0.0" : AV_STRINGIFY(LIBAVFORMAT_VERSION);
> > > +        "0.0.0" : product_version;
> > 
> > Again, why? If you have a company that maintains a fork of FFmpeg then
> > compile that info in here instead. Compare with FFmbc which always puts
> > "FFmbc" as CompanyName.
> 
> And how can a product based on libavformat set the company name, product
> name and product version? It seems a valid use case for me that these are
> overridable. Also note that this product version is only the "user friendly"
> version string, for the numeric version still LIBAVFORMAT_VERSION values are
> used.

Yes, my use case is the product is using libavformat as library, so it's
prefer to have way to override these information as requirements.

> 
> 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".
Tomas Härdin Jan. 18, 2021, 10:53 p.m. UTC | #4 
lör 2021-01-16 klockan 08:43 +0800 skrev lance.lmwang@gmail.com:
> On Fri, Jan 15, 2021 at 09:43:58PM +0100, Marton Balint wrote:
> > 
> > On Fri, 15 Jan 2021, Tomas Härdin wrote:
> > > Again, why? If you have a company that maintains a fork of FFmpeg then
> > > compile that info in here instead. Compare with FFmbc which always puts
> > > "FFmbc" as CompanyName.
> > 
> > And how can a product based on libavformat set the company name, product
> > name and product version? It seems a valid use case for me that these are
> > overridable. Also note that this product version is only the "user friendly"
> > version string, for the numeric version still LIBAVFORMAT_VERSION values are
> > used.
> 
> Yes, my use case is the product is using libavformat as library, so it's
> prefer to have way to override these information as requirements.

What I'm worried about here is that we're going to get files which
claim to have been written by something other than libavformat. I've
had situations like this before, and it is a source of headache. For 
example, if mxfenc writes some field incorrectly then this might cause
us to hack mxfdec to accept that field instead of fixing mxfenc.

/Tomas
Tobias Rapp Jan. 19, 2021, 7:59 a.m. UTC | #5 
On 18.01.2021 23:53, Tomas Härdin wrote:
> lör 2021-01-16 klockan 08:43 +0800 skrev lance.lmwang@gmail.com:
>> On Fri, Jan 15, 2021 at 09:43:58PM +0100, Marton Balint wrote:
>>>
>>> On Fri, 15 Jan 2021, Tomas Härdin wrote:
>>>> Again, why? If you have a company that maintains a fork of FFmpeg then
>>>> compile that info in here instead. Compare with FFmbc which always puts
>>>> "FFmbc" as CompanyName.
>>>
>>> And how can a product based on libavformat set the company name, product
>>> name and product version? It seems a valid use case for me that these are
>>> overridable. Also note that this product version is only the "user friendly"
>>> version string, for the numeric version still LIBAVFORMAT_VERSION values are
>>> used.
>>
>> Yes, my use case is the product is using libavformat as library, so it's
>> prefer to have way to override these information as requirements.
> 
> What I'm worried about here is that we're going to get files which
> claim to have been written by something other than libavformat. I've
> had situations like this before, and it is a source of headache. For
> example, if mxfenc writes some field incorrectly then this might cause
> us to hack mxfdec to accept that field instead of fixing mxfenc.

I agree that especially for the MXF format with its flexible structure 
it is more relevant to know the muxing library rather than the hosting 
application. Have seen MXF output files of other commercial products 
that also contain library identifiers like "libMXF" or "MXFtk" here.

Other formats in FFmpeg use the "encoder" metadata key for embedding 
library information in the output file. A quick test with AVI output 
shows that this metadata is generated internally and cannot be 
overridden on the command-line.

Regards,
Tobias
Marton Balint Jan. 19, 2021, 11:27 p.m. UTC | #6 
On Tue, 19 Jan 2021, Tobias Rapp wrote:

> On 18.01.2021 23:53, Tomas Härdin wrote:
>> lör 2021-01-16 klockan 08:43 +0800 skrev lance.lmwang@gmail.com:
>>> On Fri, Jan 15, 2021 at 09:43:58PM +0100, Marton Balint wrote:
>>>>
>>>> On Fri, 15 Jan 2021, Tomas Härdin wrote:
>>>>> Again, why? If you have a company that maintains a fork of FFmpeg then
>>>>> compile that info in here instead. Compare with FFmbc which always puts
>>>>> "FFmbc" as CompanyName.
>>>>
>>>> And how can a product based on libavformat set the company name, product
>>>> name and product version? It seems a valid use case for me that these are
>>>> overridable. Also note that this product version is only the "user 
> friendly"
>>>> version string, for the numeric version still LIBAVFORMAT_VERSION values 
> are
>>>> used.
>>>
>>> Yes, my use case is the product is using libavformat as library, so it's
>>> prefer to have way to override these information as requirements.
>> 
>> What I'm worried about here is that we're going to get files which
>> claim to have been written by something other than libavformat. I've
>> had situations like this before, and it is a source of headache. For
>> example, if mxfenc writes some field incorrectly then this might cause
>> us to hack mxfdec to accept that field instead of fixing mxfenc.
>
> I agree that especially for the MXF format with its flexible structure 
> it is more relevant to know the muxing library rather than the hosting 
> application. Have seen MXF output files of other commercial products 
> that also contain library identifiers like "libMXF" or "MXFtk" here.
>
> Other formats in FFmpeg use the "encoder" metadata key for embedding 
> library information in the output file. A quick test with AVI output 
> shows that this metadata is generated internally and cannot be 
> overridden on the command-line.

If your concern is being able to identify our mxf muxer, then why not use 
the Platform metadata item for this?

"Human readable name of the toolkit and operating system used. Best 
practice is to use the form “SDK name (OS name)”"

Seems a lot more fitting than the other metadata fields for the purpose of 
muxer identification.

Regards,
Marton
Tomas Härdin Jan. 20, 2021, 3:41 p.m. UTC | #7 
ons 2021-01-20 klockan 00:27 +0100 skrev Marton Balint:
> 
> On Tue, 19 Jan 2021, Tobias Rapp wrote:
> 
> > On 18.01.2021 23:53, Tomas Härdin wrote:
> > > lör 2021-01-16 klockan 08:43 +0800 skrev lance.lmwang@gmail.com:
> > > > On Fri, Jan 15, 2021 at 09:43:58PM +0100, Marton Balint wrote:
> > > > > On Fri, 15 Jan 2021, Tomas Härdin wrote:
> > > > > > Again, why? If you have a company that maintains a fork of FFmpeg then
> > > > > > compile that info in here instead. Compare with FFmbc which always puts
> > > > > > "FFmbc" as CompanyName.
> > > > > 
> > > > > And how can a product based on libavformat set the company name, product
> > > > > name and product version? It seems a valid use case for me that these are
> > > > > overridable. Also note that this product version is only the "user 
> > friendly"
> > > > > version string, for the numeric version still LIBAVFORMAT_VERSION values 
> > are
> > > > > used.
> > > > 
> > > > Yes, my use case is the product is using libavformat as library, so it's
> > > > prefer to have way to override these information as requirements.
> > > 
> > > What I'm worried about here is that we're going to get files which
> > > claim to have been written by something other than libavformat. I've
> > > had situations like this before, and it is a source of headache. For
> > > example, if mxfenc writes some field incorrectly then this might cause
> > > us to hack mxfdec to accept that field instead of fixing mxfenc.
> > 
> > I agree that especially for the MXF format with its flexible structure 
> > it is more relevant to know the muxing library rather than the hosting 
> > application. Have seen MXF output files of other commercial products 
> > that also contain library identifiers like "libMXF" or "MXFtk" here.
> > 
> > Other formats in FFmpeg use the "encoder" metadata key for embedding 
> > library information in the output file. A quick test with AVI output 
> > shows that this metadata is generated internally and cannot be 
> > overridden on the command-line.
> 
> If your concern is being able to identify our mxf muxer, then why not use 
> the Platform metadata item for this?
> 
> "Human readable name of the toolkit and operating system used. Best 
> practice is to use the form “SDK name (OS name)”"

Where is this text from? Neither S377m-2004 nor RP210v10-2007 says
this.

/Tomas
diff mbox series

Patch

diff --git a/fftools/ffmpeg_opt.c b/fftools/ffmpeg_opt.c
index c295514..493763b 100644
--- a/fftools/ffmpeg_opt.c
+++ b/fftools/ffmpeg_opt.c
@@ -2650,6 +2650,9 @@  loop_end:
         if(o->recording_time != INT64_MAX)
             av_dict_set(&oc->metadata, "duration", NULL, 0);
         av_dict_set(&oc->metadata, "creation_time", NULL, 0);
+        av_dict_set(&oc->metadata, "company_name", NULL, 0);
+        av_dict_set(&oc->metadata, "product_name", NULL, 0);
+        av_dict_set(&oc->metadata, "product_version", NULL, 0);
     }
     if (!o->metadata_streams_manual)
         for (i = of->ost_index; i < nb_output_streams; i++) {
diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c
index d8678c9..5244211 100644
--- a/libavformat/mxfenc.c
+++ b/libavformat/mxfenc.c
@@ -722,16 +722,20 @@  static void mxf_write_identification(AVFormatContext *s)
 {
     MXFContext *mxf = s->priv_data;
     AVIOContext *pb = s->pb;
-    const char *company = "FFmpeg";
-    const char *product = s->oformat != &ff_mxf_opatom_muxer ? "OP1a Muxer" : "OPAtom Muxer";
-    const char *version;
+    AVDictionaryEntry *com_entry =  av_dict_get(s->metadata, "company_name", NULL, 0);
+    AVDictionaryEntry *product_entry =  av_dict_get(s->metadata, "product_name", NULL, 0);
+    AVDictionaryEntry *version_entry =  av_dict_get(s->metadata, "product_version", NULL, 0);
+    const char *company = com_entry ? com_entry->value : "FFmpeg";
+    const char *product = product_entry ? product_entry->value : s->oformat != &ff_mxf_opatom_muxer ? "OP1a Muxer" : "OPAtom Muxer";
+    const char *version = NULL;
+    const char *product_version = version_entry ? version_entry->value : AV_STRINGIFY(LIBAVFORMAT_VERSION);
     int length;
 
     mxf_write_metadata_key(pb, 0x013000);
     PRINT_KEY(s, "identification key", pb->buf_ptr - 16);
 
     version = s->flags & AVFMT_FLAG_BITEXACT ?
-        "0.0.0" : AV_STRINGIFY(LIBAVFORMAT_VERSION);
+        "0.0.0" : product_version;
     length = 100 +mxf_utf16_local_tag_length(company) +
                   mxf_utf16_local_tag_length(product) +
                   mxf_utf16_local_tag_length(version);