diff mbox series

[FFmpeg-devel,v3,3/3] avformat/mxfenc: prefer to use the existing metadata

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

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

Lance Wang Jan. 8, 2021, 6:32 a.m. UTC 
From: Limin Wang <lance.lmwang@gmail.com>

Please check metadata with below command:
./ffmpeg -i ../fate-suite/mxf/Sony-00001.mxf -c:v copy -c:a copy out.mxf
./ffmpeg -i out.mxf

    company_name    : FFmpeg
    product_name    : OP1a Muxer
    product_version : 58.65.101o
    =>
    company_name    : SONY
    product_name    : eVTR
    product_version : 1.00

So need to update fate-mxf fate test.

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 libavformat/mxfenc.c                    | 12 ++++++++----
 tests/ref/fate/mxf-d10-user-comments    |  2 +-
 tests/ref/fate/mxf-opatom-user-comments |  2 +-
 tests/ref/fate/mxf-reel_name            |  2 +-
 tests/ref/fate/mxf-user-comments        |  2 +-
 5 files changed, 12 insertions(+), 8 deletions(-)

Comments

Tobias Rapp Jan. 8, 2021, 8:09 a.m. UTC | #1 
On 08.01.2021 07:32, lance.lmwang@gmail.com wrote:
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> Please check metadata with below command:
> ./ffmpeg -i ../fate-suite/mxf/Sony-00001.mxf -c:v copy -c:a copy out.mxf
> ./ffmpeg -i out.mxf
> 
>      company_name    : FFmpeg
>      product_name    : OP1a Muxer
>      product_version : 58.65.101o
>      =>
>      company_name    : SONY
>      product_name    : eVTR
>      product_version : 1.00
> 
> So need to update fate-mxf fate test.
> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
> [...]

In my opinion the MXF identification set should contain data about the 
application that wrote the current version of a file, not some previous 
file version.

Regards,
Tobias
Lance Wang Jan. 8, 2021, 10:01 a.m. UTC | #2 
On Fri, Jan 08, 2021 at 09:09:34AM +0100, Tobias Rapp wrote:
> On 08.01.2021 07:32, lance.lmwang@gmail.com wrote:
> > From: Limin Wang <lance.lmwang@gmail.com>
> > 
> > Please check metadata with below command:
> > ./ffmpeg -i ../fate-suite/mxf/Sony-00001.mxf -c:v copy -c:a copy out.mxf
> > ./ffmpeg -i out.mxf
> > 
> >      company_name    : FFmpeg
> >      product_name    : OP1a Muxer
> >      product_version : 58.65.101o
> >      =>
> >      company_name    : SONY
> >      product_name    : eVTR
> >      product_version : 1.00
> > 
> > So need to update fate-mxf fate test.
> > 
> > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > ---
> > [...]
> 
> In my opinion the MXF identification set should contain data about the
> application that wrote the current version of a file, not some previous file
> version.

The example command shows what's change for the fate testing, if you want to
update to use  your own product version, please use -metadata product_version="xxxxx".

> 
> Regards,
> Tobias
> 
> _______________________________________________
> 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".
Tobias Rapp Jan. 8, 2021, 12:43 p.m. UTC | #3 
On 08.01.2021 11:01, lance.lmwang@gmail.com wrote:
> On Fri, Jan 08, 2021 at 09:09:34AM +0100, Tobias Rapp wrote:
>> On 08.01.2021 07:32, lance.lmwang@gmail.com wrote:
>>> From: Limin Wang <lance.lmwang@gmail.com>
>>>
>>> Please check metadata with below command:
>>> ./ffmpeg -i ../fate-suite/mxf/Sony-00001.mxf -c:v copy -c:a copy out.mxf
>>> ./ffmpeg -i out.mxf
>>>
>>>       company_name    : FFmpeg
>>>       product_name    : OP1a Muxer
>>>       product_version : 58.65.101o
>>>       =>
>>>       company_name    : SONY
>>>       product_name    : eVTR
>>>       product_version : 1.00
>>>
>>> So need to update fate-mxf fate test.
>>>
>>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>>> ---
>>> [...]
>>
>> In my opinion the MXF identification set should contain data about the
>> application that wrote the current version of a file, not some previous file
>> version.
> 
> The example command shows what's change for the fate testing, if you want to
> update to use  your own product version, please use -metadata product_version="xxxxx".

It looks wrong that a MXF file that is muxed by FFmpeg pretends to be 
written by a SONY application. I see that with "-codec copy" you can 
avoid re-encoding of the video and audio streams, and thus might want to 
indicate the A/V encoder software info somewhere separate from the 
container format writer software. But this patch overrides both information.

Regards,
Tobias
Marton Balint Jan. 9, 2021, 12:09 a.m. UTC | #4 
On Fri, 8 Jan 2021, Tobias Rapp wrote:

> On 08.01.2021 11:01, lance.lmwang@gmail.com wrote:
>> On Fri, Jan 08, 2021 at 09:09:34AM +0100, Tobias Rapp wrote:
>>> On 08.01.2021 07:32, lance.lmwang@gmail.com wrote:
>>>> From: Limin Wang <lance.lmwang@gmail.com>
>>>>
>>>> Please check metadata with below command:
>>>> ./ffmpeg -i ../fate-suite/mxf/Sony-00001.mxf -c:v copy -c:a copy out.mxf
>>>> ./ffmpeg -i out.mxf
>>>>
>>>>       company_name    : FFmpeg
>>>>       product_name    : OP1a Muxer
>>>>       product_version : 58.65.101o
>>>>       =>
>>>>       company_name    : SONY
>>>>       product_name    : eVTR
>>>>       product_version : 1.00
>>>>
>>>> So need to update fate-mxf fate test.
>>>>
>>>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>>>> ---
>>>> [...]
>>>
>>> In my opinion the MXF identification set should contain data about the
>>> application that wrote the current version of a file, not some previous 
> file
>>> version.
>> 
>> The example command shows what's change for the fate testing, if you want 
> to
>> update to use  your own product version, please use -metadata 
> product_version="xxxxx".
>
> It looks wrong that a MXF file that is muxed by FFmpeg pretends to be 
> written by a SONY application. I see that with "-codec copy" you can 
> avoid re-encoding of the video and audio streams, and thus might want to 
> indicate the A/V encoder software info somewhere separate from the 
> container format writer software. But this patch overrides both information.

We might remove company_name, product_name and product_version metadata in 
fftools/ffmpeg_opt.c similarly how creation_time is removed for automatic 
metadata copy.

Regards,
Marton
Lance Wang Jan. 9, 2021, 3:17 a.m. UTC | #5 
On Sat, Jan 09, 2021 at 01:09:22AM +0100, Marton Balint wrote:
> 
> 
> On Fri, 8 Jan 2021, Tobias Rapp wrote:
> 
> > On 08.01.2021 11:01, lance.lmwang@gmail.com wrote:
> > > On Fri, Jan 08, 2021 at 09:09:34AM +0100, Tobias Rapp wrote:
> > > > On 08.01.2021 07:32, lance.lmwang@gmail.com wrote:
> > > > > From: Limin Wang <lance.lmwang@gmail.com>
> > > > > 
> > > > > Please check metadata with below command:
> > > > > ./ffmpeg -i ../fate-suite/mxf/Sony-00001.mxf -c:v copy -c:a copy out.mxf
> > > > > ./ffmpeg -i out.mxf
> > > > > 
> > > > >       company_name    : FFmpeg
> > > > >       product_name    : OP1a Muxer
> > > > >       product_version : 58.65.101o
> > > > >       =>
> > > > >       company_name    : SONY
> > > > >       product_name    : eVTR
> > > > >       product_version : 1.00
> > > > > 
> > > > > So need to update fate-mxf fate test.
> > > > > 
> > > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > > > ---
> > > > > [...]
> > > > 
> > > > In my opinion the MXF identification set should contain data about the
> > > > application that wrote the current version of a file, not some
> > > > previous
> > file
> > > > version.
> > > 
> > > The example command shows what's change for the fate testing, if you
> > > want
> > to
> > > update to use  your own product version, please use -metadata
> > product_version="xxxxx".
> > 
> > It looks wrong that a MXF file that is muxed by FFmpeg pretends to be
> > written by a SONY application. I see that with "-codec copy" you can
> > avoid re-encoding of the video and audio streams, and thus might want to
> > indicate the A/V encoder software info somewhere separate from the
> > container format writer software. But this patch overrides both
> > information.
> 
> We might remove company_name, product_name and product_version metadata in
> fftools/ffmpeg_opt.c similarly how creation_time is removed for automatic
> metadata copy.

This is good suggestion, I'll update the patch by this way to avoid overrides 
with input information.

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

Patch

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);
diff --git a/tests/ref/fate/mxf-d10-user-comments b/tests/ref/fate/mxf-d10-user-comments
index de4f26c..4aebcaf 100644
--- a/tests/ref/fate/mxf-d10-user-comments
+++ b/tests/ref/fate/mxf-d10-user-comments
@@ -1 +1 @@ 
-68f0fa62b6a676894afbbe4c34ebf70b
+84e289a83797e793bfa0d3a31f94ac6c
diff --git a/tests/ref/fate/mxf-opatom-user-comments b/tests/ref/fate/mxf-opatom-user-comments
index 90e3fb2..374a72a 100644
--- a/tests/ref/fate/mxf-opatom-user-comments
+++ b/tests/ref/fate/mxf-opatom-user-comments
@@ -1 +1 @@ 
-f6760a9e710ba478bc3949f3e5c9b34a
+e7c41639b79ac54c4df05475fb0eea66
diff --git a/tests/ref/fate/mxf-reel_name b/tests/ref/fate/mxf-reel_name
index 16022b1..6e3218f 100644
--- a/tests/ref/fate/mxf-reel_name
+++ b/tests/ref/fate/mxf-reel_name
@@ -1 +1 @@ 
-73a891041b2fc836a893ffb49fff4fff
+be4c1b76138c855ac3e2d2579cbecc17
diff --git a/tests/ref/fate/mxf-user-comments b/tests/ref/fate/mxf-user-comments
index ddf51d9..46db6a3 100644
--- a/tests/ref/fate/mxf-user-comments
+++ b/tests/ref/fate/mxf-user-comments
@@ -1 +1 @@ 
-1255faf854223a74d707553121e5eca3
+8f2360104655971dc5fb68f98eda1b84