[FFmpeg-devel] lavf/mov: Export vendor metadata

Submitted by Carl Eugen Hoyos on Dec. 12, 2016, 11:12 a.m.

Details

Message ID 201612121212.37253.cehoyos@ag.or.at
State New
Headers show

Commit Message

Carl Eugen Hoyos Dec. 12, 2016, 11:12 a.m.
Hi!

I saw only after writing this patch that a Google employee has sent 
a more complicated variant of this patch in November (searching for 
a description of the vendor field showed the patch).

Please comment, Carl Eugen
From 182ee7b3dca51a49a606c4d11758819ec29fc181 Mon Sep 17 00:00:00 2001
From: Carl Eugen Hoyos <cehoyos@ag.or.at>
Date: Mon, 12 Dec 2016 12:05:38 +0100
Subject: [PATCH 1/2] ffmpeg: Do not copy vendor metadata on reencoding.

---
 ffmpeg.c |    1 +
 1 file changed, 1 insertion(+)

Comments

Carl Eugen Hoyos Jan. 24, 2017, 11:53 p.m.
2016-12-12 12:12 GMT+01:00 Carl Eugen Hoyos <cehoyos@ag.or.at>:

> I saw only after writing this patch that a Google employee has sent
> a more complicated variant of this patch in November (searching for
> a description of the vendor field showed the patch).

Ping for the ffmpeg patch.

I will add audio vendor to the mov patch.

Carl Eugen
wm4 Jan. 25, 2017, 1:22 p.m.
On Mon, 12 Dec 2016 12:12:37 +0100
Carl Eugen Hoyos <cehoyos@ag.or.at> wrote:

> From 7c26220a8734fe7dc293efe6c13e3baf91defc7e Mon Sep 17 00:00:00 2001
> From: Carl Eugen Hoyos <cehoyos@ag.or.at>
> Date: Mon, 12 Dec 2016 12:07:27 +0100
> Subject: [PATCH 2/2] lavf/mov: Export vendor metadata.
> 
> ---
>  libavformat/mov.c         |    5 ++++-
>  tests/ref/fate/mov-zombie |    2 +-
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index 0b1c182..a19ebbf 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -1842,6 +1842,7 @@ static void mov_parse_stsd_video(MOVContext *c, AVIOContext *pb,
>                                   AVStream *st, MOVStreamContext *sc)
>  {
>      uint8_t codec_name[32] = { 0 };
> +    uint8_t vendor[5] = { 0 };
>      int64_t stsd_start;
>      unsigned int len;
>  
> @@ -1851,7 +1852,9 @@ static void mov_parse_stsd_video(MOVContext *c, AVIOContext *pb,
>  
>      avio_rb16(pb); /* version */
>      avio_rb16(pb); /* revision level */
> -    avio_rb32(pb); /* vendor */
> +    avio_read(pb, vendor, 4);
> +    if (vendor[0])
> +        av_dict_set(&st->metadata, "vendor", vendor, 0);

Does this mean transcoding to a format with per-stream tags will add
this as a tag?
Carl Eugen Hoyos Jan. 25, 2017, 11:04 p.m.
2017-01-25 14:22 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> On Mon, 12 Dec 2016 12:12:37 +0100
> Carl Eugen Hoyos <cehoyos@ag.or.at> wrote:
>
>> From 7c26220a8734fe7dc293efe6c13e3baf91defc7e Mon Sep 17 00:00:00 2001
>> From: Carl Eugen Hoyos <cehoyos@ag.or.at>
>> Date: Mon, 12 Dec 2016 12:07:27 +0100
>> Subject: [PATCH 2/2] lavf/mov: Export vendor metadata.
>>
>> ---
>>  libavformat/mov.c         |    5 ++++-
>>  tests/ref/fate/mov-zombie |    2 +-
>>  2 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> index 0b1c182..a19ebbf 100644
>> --- a/libavformat/mov.c
>> +++ b/libavformat/mov.c
>> @@ -1842,6 +1842,7 @@ static void mov_parse_stsd_video(MOVContext *c, AVIOContext *pb,
>>                                   AVStream *st, MOVStreamContext *sc)
>>  {
>>      uint8_t codec_name[32] = { 0 };
>> +    uint8_t vendor[5] = { 0 };
>>      int64_t stsd_start;
>>      unsigned int len;
>>
>> @@ -1851,7 +1852,9 @@ static void mov_parse_stsd_video(MOVContext *c, AVIOContext *pb,
>>
>>      avio_rb16(pb); /* version */
>>      avio_rb16(pb); /* revision level */
>> -    avio_rb32(pb); /* vendor */
>> +    avio_read(pb, vendor, 4);
>> +    if (vendor[0])
>> +        av_dict_set(&st->metadata, "vendor", vendor, 0);
>
> Does this mean transcoding to a format with per-stream
> tags will add this as a tag?

The patch you quoted allows libavformat users to read the
vendor tag from mov files, I don't understand how it adds
tags.
Do you object?

Carl Eugen
Hendrik Leppkes Jan. 25, 2017, 11:19 p.m.
On Thu, Jan 26, 2017 at 10:04 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> 2017-01-25 14:22 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
>> On Mon, 12 Dec 2016 12:12:37 +0100
>> Carl Eugen Hoyos <cehoyos@ag.or.at> wrote:
>>
>>> From 7c26220a8734fe7dc293efe6c13e3baf91defc7e Mon Sep 17 00:00:00 2001
>>> From: Carl Eugen Hoyos <cehoyos@ag.or.at>
>>> Date: Mon, 12 Dec 2016 12:07:27 +0100
>>> Subject: [PATCH 2/2] lavf/mov: Export vendor metadata.
>>>
>>> ---
>>>  libavformat/mov.c         |    5 ++++-
>>>  tests/ref/fate/mov-zombie |    2 +-
>>>  2 files changed, 5 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>>> index 0b1c182..a19ebbf 100644
>>> --- a/libavformat/mov.c
>>> +++ b/libavformat/mov.c
>>> @@ -1842,6 +1842,7 @@ static void mov_parse_stsd_video(MOVContext *c, AVIOContext *pb,
>>>                                   AVStream *st, MOVStreamContext *sc)
>>>  {
>>>      uint8_t codec_name[32] = { 0 };
>>> +    uint8_t vendor[5] = { 0 };
>>>      int64_t stsd_start;
>>>      unsigned int len;
>>>
>>> @@ -1851,7 +1852,9 @@ static void mov_parse_stsd_video(MOVContext *c, AVIOContext *pb,
>>>
>>>      avio_rb16(pb); /* version */
>>>      avio_rb16(pb); /* revision level */
>>> -    avio_rb32(pb); /* vendor */
>>> +    avio_read(pb, vendor, 4);
>>> +    if (vendor[0])
>>> +        av_dict_set(&st->metadata, "vendor", vendor, 0);
>>
>> Does this mean transcoding to a format with per-stream
>> tags will add this as a tag?
>
> The patch you quoted allows libavformat users to read the
> vendor tag from mov files, I don't understand how it adds
> tags.
> Do you object?

Any metadata you export can and will get copied to a new file when
remuxing, therefor exporting arbitrary info that isn't actual stream
metadata tags in metadata is problematic - it carries over to the
destination file, in which it would be entirely meaningless.

- Hendrik
Carl Eugen Hoyos Jan. 25, 2017, 11:35 p.m.
2017-01-26 0:19 GMT+01:00 Hendrik Leppkes <h.leppkes@gmail.com>:
> On Thu, Jan 26, 2017 at 10:04 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>> 2017-01-25 14:22 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
>>> On Mon, 12 Dec 2016 12:12:37 +0100
>>> Carl Eugen Hoyos <cehoyos@ag.or.at> wrote:
>>>
>>>> From 7c26220a8734fe7dc293efe6c13e3baf91defc7e Mon Sep 17 00:00:00 2001
>>>> From: Carl Eugen Hoyos <cehoyos@ag.or.at>
>>>> Date: Mon, 12 Dec 2016 12:07:27 +0100
>>>> Subject: [PATCH 2/2] lavf/mov: Export vendor metadata.
>>>>
>>>> ---
>>>>  libavformat/mov.c         |    5 ++++-
>>>>  tests/ref/fate/mov-zombie |    2 +-
>>>>  2 files changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>>>> index 0b1c182..a19ebbf 100644
>>>> --- a/libavformat/mov.c
>>>> +++ b/libavformat/mov.c
>>>> @@ -1842,6 +1842,7 @@ static void mov_parse_stsd_video(MOVContext *c, AVIOContext *pb,
>>>>                                   AVStream *st, MOVStreamContext *sc)
>>>>  {
>>>>      uint8_t codec_name[32] = { 0 };
>>>> +    uint8_t vendor[5] = { 0 };
>>>>      int64_t stsd_start;
>>>>      unsigned int len;
>>>>
>>>> @@ -1851,7 +1852,9 @@ static void mov_parse_stsd_video(MOVContext *c, AVIOContext *pb,
>>>>
>>>>      avio_rb16(pb); /* version */
>>>>      avio_rb16(pb); /* revision level */
>>>> -    avio_rb32(pb); /* vendor */
>>>> +    avio_read(pb, vendor, 4);
>>>> +    if (vendor[0])
>>>> +        av_dict_set(&st->metadata, "vendor", vendor, 0);
>>>
>>> Does this mean transcoding to a format with per-stream
>>> tags will add this as a tag?
>>
>> The patch you quoted allows libavformat users to read the
>> vendor tag from mov files, I don't understand how it adds
>> tags.
>> Do you object?
>
> Any metadata you export can and will get copied to a new file when
> remuxing, therefor exporting arbitrary info that isn't actual stream
> metadata tags in metadata is problematic - it carries over to the
> destination file, in which it would be entirely meaningless.

Sorry, I don't understand:
Which application do you mean?

Carl Eugen
wm4 Jan. 26, 2017, 5:24 a.m.
On Thu, 26 Jan 2017 00:35:17 +0100
Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> 2017-01-26 0:19 GMT+01:00 Hendrik Leppkes <h.leppkes@gmail.com>:
> > On Thu, Jan 26, 2017 at 10:04 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:  
> >> 2017-01-25 14:22 GMT+01:00 wm4 <nfxjfg@googlemail.com>:  
> >>> On Mon, 12 Dec 2016 12:12:37 +0100
> >>> Carl Eugen Hoyos <cehoyos@ag.or.at> wrote:
> >>>  
> >>>> From 7c26220a8734fe7dc293efe6c13e3baf91defc7e Mon Sep 17 00:00:00 2001
> >>>> From: Carl Eugen Hoyos <cehoyos@ag.or.at>
> >>>> Date: Mon, 12 Dec 2016 12:07:27 +0100
> >>>> Subject: [PATCH 2/2] lavf/mov: Export vendor metadata.
> >>>>
> >>>> ---
> >>>>  libavformat/mov.c         |    5 ++++-
> >>>>  tests/ref/fate/mov-zombie |    2 +-
> >>>>  2 files changed, 5 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/libavformat/mov.c b/libavformat/mov.c
> >>>> index 0b1c182..a19ebbf 100644
> >>>> --- a/libavformat/mov.c
> >>>> +++ b/libavformat/mov.c
> >>>> @@ -1842,6 +1842,7 @@ static void mov_parse_stsd_video(MOVContext *c, AVIOContext *pb,
> >>>>                                   AVStream *st, MOVStreamContext *sc)
> >>>>  {
> >>>>      uint8_t codec_name[32] = { 0 };
> >>>> +    uint8_t vendor[5] = { 0 };
> >>>>      int64_t stsd_start;
> >>>>      unsigned int len;
> >>>>
> >>>> @@ -1851,7 +1852,9 @@ static void mov_parse_stsd_video(MOVContext *c, AVIOContext *pb,
> >>>>
> >>>>      avio_rb16(pb); /* version */
> >>>>      avio_rb16(pb); /* revision level */
> >>>> -    avio_rb32(pb); /* vendor */
> >>>> +    avio_read(pb, vendor, 4);
> >>>> +    if (vendor[0])
> >>>> +        av_dict_set(&st->metadata, "vendor", vendor, 0);  
> >>>
> >>> Does this mean transcoding to a format with per-stream
> >>> tags will add this as a tag?  
> >>
> >> The patch you quoted allows libavformat users to read the
> >> vendor tag from mov files, I don't understand how it adds
> >> tags.
> >> Do you object?  
> >
> > Any metadata you export can and will get copied to a new file when
> > remuxing, therefor exporting arbitrary info that isn't actual stream
> > metadata tags in metadata is problematic - it carries over to the
> > destination file, in which it would be entirely meaningless.  
> 
> Sorry, I don't understand:
> Which application do you mean?

ffmpeg

Or does it somehow have a whitelist of which tags to copy? I didn't
attempt to confirm the behavior.
Carl Eugen Hoyos Jan. 26, 2017, 7:26 a.m.
2017-01-26 6:24 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> On Thu, 26 Jan 2017 00:35:17 +0100
> Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
>> 2017-01-26 0:19 GMT+01:00 Hendrik Leppkes <h.leppkes@gmail.com>:
>> > On Thu, Jan 26, 2017 at 10:04 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>> >> 2017-01-25 14:22 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
>> >>> On Mon, 12 Dec 2016 12:12:37 +0100
>> >>> Carl Eugen Hoyos <cehoyos@ag.or.at> wrote:
>> >>>
>> >>>> From 7c26220a8734fe7dc293efe6c13e3baf91defc7e Mon Sep 17 00:00:00 2001
>> >>>> From: Carl Eugen Hoyos <cehoyos@ag.or.at>
>> >>>> Date: Mon, 12 Dec 2016 12:07:27 +0100
>> >>>> Subject: [PATCH 2/2] lavf/mov: Export vendor metadata.
>> >>>>
>> >>>> ---
>> >>>>  libavformat/mov.c         |    5 ++++-
>> >>>>  tests/ref/fate/mov-zombie |    2 +-
>> >>>>  2 files changed, 5 insertions(+), 2 deletions(-)
>> >>>>
>> >>>> diff --git a/libavformat/mov.c b/libavformat/mov.c
>> >>>> index 0b1c182..a19ebbf 100644
>> >>>> --- a/libavformat/mov.c
>> >>>> +++ b/libavformat/mov.c
>> >>>> @@ -1842,6 +1842,7 @@ static void mov_parse_stsd_video(MOVContext *c, AVIOContext *pb,
>> >>>>                                   AVStream *st, MOVStreamContext *sc)
>> >>>>  {
>> >>>>      uint8_t codec_name[32] = { 0 };
>> >>>> +    uint8_t vendor[5] = { 0 };
>> >>>>      int64_t stsd_start;
>> >>>>      unsigned int len;
>> >>>>
>> >>>> @@ -1851,7 +1852,9 @@ static void mov_parse_stsd_video(MOVContext *c, AVIOContext *pb,
>> >>>>
>> >>>>      avio_rb16(pb); /* version */
>> >>>>      avio_rb16(pb); /* revision level */
>> >>>> -    avio_rb32(pb); /* vendor */
>> >>>> +    avio_read(pb, vendor, 4);
>> >>>> +    if (vendor[0])
>> >>>> +        av_dict_set(&st->metadata, "vendor", vendor, 0);
>> >>>
>> >>> Does this mean transcoding to a format with per-stream
>> >>> tags will add this as a tag?
>> >>
>> >> The patch you quoted allows libavformat users to read the
>> >> vendor tag from mov files, I don't understand how it adds
>> >> tags.
>> >> Do you object?
>> >
>> > Any metadata you export can and will get copied to a new file when
>> > remuxing, therefor exporting arbitrary info that isn't actual stream
>> > metadata tags in metadata is problematic - it carries over to the
>> > destination file, in which it would be entirely meaningless.
>>
>> Sorry, I don't understand:
>> Which application do you mean?
>
> ffmpeg

Didn't I ping yesterday a patch that avoids this?
And wasn't this the mail you answered?

Carl Eugen
wm4 Jan. 26, 2017, 8:07 a.m.
On Thu, 26 Jan 2017 08:26:02 +0100
Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> 2017-01-26 6:24 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> > On Thu, 26 Jan 2017 00:35:17 +0100
> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >  
> >> 2017-01-26 0:19 GMT+01:00 Hendrik Leppkes <h.leppkes@gmail.com>:  
> >> > On Thu, Jan 26, 2017 at 10:04 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:  
> >> >> 2017-01-25 14:22 GMT+01:00 wm4 <nfxjfg@googlemail.com>:  
> >> >>> On Mon, 12 Dec 2016 12:12:37 +0100
> >> >>> Carl Eugen Hoyos <cehoyos@ag.or.at> wrote:
> >> >>>  
> >> >>>> From 7c26220a8734fe7dc293efe6c13e3baf91defc7e Mon Sep 17 00:00:00 2001
> >> >>>> From: Carl Eugen Hoyos <cehoyos@ag.or.at>
> >> >>>> Date: Mon, 12 Dec 2016 12:07:27 +0100
> >> >>>> Subject: [PATCH 2/2] lavf/mov: Export vendor metadata.
> >> >>>>
> >> >>>> ---
> >> >>>>  libavformat/mov.c         |    5 ++++-
> >> >>>>  tests/ref/fate/mov-zombie |    2 +-
> >> >>>>  2 files changed, 5 insertions(+), 2 deletions(-)
> >> >>>>
> >> >>>> diff --git a/libavformat/mov.c b/libavformat/mov.c
> >> >>>> index 0b1c182..a19ebbf 100644
> >> >>>> --- a/libavformat/mov.c
> >> >>>> +++ b/libavformat/mov.c
> >> >>>> @@ -1842,6 +1842,7 @@ static void mov_parse_stsd_video(MOVContext *c, AVIOContext *pb,
> >> >>>>                                   AVStream *st, MOVStreamContext *sc)
> >> >>>>  {
> >> >>>>      uint8_t codec_name[32] = { 0 };
> >> >>>> +    uint8_t vendor[5] = { 0 };
> >> >>>>      int64_t stsd_start;
> >> >>>>      unsigned int len;
> >> >>>>
> >> >>>> @@ -1851,7 +1852,9 @@ static void mov_parse_stsd_video(MOVContext *c, AVIOContext *pb,
> >> >>>>
> >> >>>>      avio_rb16(pb); /* version */
> >> >>>>      avio_rb16(pb); /* revision level */
> >> >>>> -    avio_rb32(pb); /* vendor */
> >> >>>> +    avio_read(pb, vendor, 4);
> >> >>>> +    if (vendor[0])
> >> >>>> +        av_dict_set(&st->metadata, "vendor", vendor, 0);  
> >> >>>
> >> >>> Does this mean transcoding to a format with per-stream
> >> >>> tags will add this as a tag?  
> >> >>
> >> >> The patch you quoted allows libavformat users to read the
> >> >> vendor tag from mov files, I don't understand how it adds
> >> >> tags.
> >> >> Do you object?  
> >> >
> >> > Any metadata you export can and will get copied to a new file when
> >> > remuxing, therefor exporting arbitrary info that isn't actual stream
> >> > metadata tags in metadata is problematic - it carries over to the
> >> > destination file, in which it would be entirely meaningless.  
> >>
> >> Sorry, I don't understand:
> >> Which application do you mean?  
> >
> > ffmpeg  
> 
> Didn't I ping yesterday a patch that avoids this?
> And wasn't this the mail you answered?

Sorry, I couldn't find it just now. What was the subject line?

I'm not necessarily blocking your patch. There are already plenty of
other cases where it breaks this way, and your patch just adds another.
But maybe they can all be fixed by that patch you mentioned.
Carl Eugen Hoyos Jan. 26, 2017, 8:16 a.m.
2017-01-26 9:07 GMT+01:00 wm4 <nfxjfg@googlemail.com>:

>> >> > Any metadata you export can and will get copied to a new file when
>> >> > remuxing, therefor exporting arbitrary info that isn't actual stream
>> >> > metadata tags in metadata is problematic - it carries over to the
>> >> > destination file, in which it would be entirely meaningless.
>> >>
>> >> Sorry, I don't understand:
>> >> Which application do you mean?
>> >
>> > ffmpeg
>>
>> Didn't I ping yesterday a patch that avoids this?
>> And wasn't this the mail you answered?
>
> Sorry, I couldn't find it just now. What was the subject line?

My mailer (gmail) shows it as the first mail of this thread, so
it (hopefully) uses the same subject.

> I'm not necessarily blocking your patch.

> There are already plenty of other cases where it breaks this
> way,

Where did you report this?
I believe you know how difficult it is to fix bugs that do not get
reported.

Carl Eugen
wm4 Jan. 26, 2017, 8:26 a.m.
On Thu, 26 Jan 2017 09:16:00 +0100
Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> 2017-01-26 9:07 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> 
> >> >> > Any metadata you export can and will get copied to a new file when
> >> >> > remuxing, therefor exporting arbitrary info that isn't actual stream
> >> >> > metadata tags in metadata is problematic - it carries over to the
> >> >> > destination file, in which it would be entirely meaningless.  
> >> >>
> >> >> Sorry, I don't understand:
> >> >> Which application do you mean?  
> >> >
> >> > ffmpeg  
> >>
> >> Didn't I ping yesterday a patch that avoids this?
> >> And wasn't this the mail you answered?  
> >
> > Sorry, I couldn't find it just now. What was the subject line?  
> 
> My mailer (gmail) shows it as the first mail of this thread, so
> it (hopefully) uses the same subject.

Sorry, I missed that. But this code is not called for remuxing, or is
it?

> > I'm not necessarily blocking your patch.  
> 
> > There are already plenty of other cases where it breaks this
> > way,  
> 
> Where did you report this?
> I believe you know how difficult it is to fix bugs that do not get
> reported.

I think I mentioned it multiple times.

I do not report ffmpeg bugs because I'm more productive not doing that,
sorry.
Carl Eugen Hoyos Jan. 26, 2017, 5:06 p.m.
2017-01-26 9:26 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> On Thu, 26 Jan 2017 09:16:00 +0100
> Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
>> 2017-01-26 9:07 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
>>
>> >> >> > Any metadata you export can and will get copied to a new file when
>> >> >> > remuxing, therefor exporting arbitrary info that isn't actual stream
>> >> >> > metadata tags in metadata is problematic - it carries over to the
>> >> >> > destination file, in which it would be entirely meaningless.
>> >> >>
>> >> >> Sorry, I don't understand:
>> >> >> Which application do you mean?
>> >> >
>> >> > ffmpeg
>> >>
>> >> Didn't I ping yesterday a patch that avoids this?
>> >> And wasn't this the mail you answered?
>> >
>> > Sorry, I couldn't find it just now. What was the subject line?
>>
>> My mailer (gmail) shows it as the first mail of this thread, so
>> it (hopefully) uses the same subject.
>
> Sorry, I missed that. But this code is not called for remuxing,
> or is it?

Afair, this behaviour (copying the vendor on remuxing) is
consistent with the Apple documentation.

Carl Eugen
wm4 Jan. 27, 2017, 6:04 a.m.
On Thu, 26 Jan 2017 18:06:39 +0100
Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> 2017-01-26 9:26 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> > On Thu, 26 Jan 2017 09:16:00 +0100
> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >  
> >> 2017-01-26 9:07 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> >>  
> >> >> >> > Any metadata you export can and will get copied to a new file when
> >> >> >> > remuxing, therefor exporting arbitrary info that isn't actual stream
> >> >> >> > metadata tags in metadata is problematic - it carries over to the
> >> >> >> > destination file, in which it would be entirely meaningless.  
> >> >> >>
> >> >> >> Sorry, I don't understand:
> >> >> >> Which application do you mean?  
> >> >> >
> >> >> > ffmpeg  
> >> >>
> >> >> Didn't I ping yesterday a patch that avoids this?
> >> >> And wasn't this the mail you answered?  
> >> >
> >> > Sorry, I couldn't find it just now. What was the subject line?  
> >>
> >> My mailer (gmail) shows it as the first mail of this thread, so
> >> it (hopefully) uses the same subject.  
> >
> > Sorry, I missed that. But this code is not called for remuxing,
> > or is it?  
> 
> Afair, this behaviour (copying the vendor on remuxing) is
> consistent with the Apple documentation.
> 

What if you remux to mkv?
Carl Eugen Hoyos Jan. 27, 2017, 7:26 a.m.
2017-01-27 7:04 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> On Thu, 26 Jan 2017 18:06:39 +0100
> Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
>> 2017-01-26 9:26 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
>> > On Thu, 26 Jan 2017 09:16:00 +0100
>> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>> >
>> >> 2017-01-26 9:07 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
>> >>
>> >> >> >> > Any metadata you export can and will get copied to a new file when
>> >> >> >> > remuxing, therefor exporting arbitrary info that isn't actual stream
>> >> >> >> > metadata tags in metadata is problematic - it carries over to the
>> >> >> >> > destination file, in which it would be entirely meaningless.
>> >> >> >>
>> >> >> >> Sorry, I don't understand:
>> >> >> >> Which application do you mean?
>> >> >> >
>> >> >> > ffmpeg
>> >> >>
>> >> >> Didn't I ping yesterday a patch that avoids this?
>> >> >> And wasn't this the mail you answered?
>> >> >
>> >> > Sorry, I couldn't find it just now. What was the subject line?
>> >>
>> >> My mailer (gmail) shows it as the first mail of this thread, so
>> >> it (hopefully) uses the same subject.
>> >
>> > Sorry, I missed that. But this code is not called for remuxing,
>> > or is it?
>>
>> Afair, this behaviour (copying the vendor on remuxing) is
>> consistent with the Apple documentation.
>
> What if you remux to mkv?

Afair, copying the vendor tag on remuxing is consistent with
its documentation.

Carl Eugen
wm4 Jan. 27, 2017, 7:56 a.m.
On Fri, 27 Jan 2017 08:26:26 +0100
Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> 2017-01-27 7:04 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> > On Thu, 26 Jan 2017 18:06:39 +0100
> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >  
> >> 2017-01-26 9:26 GMT+01:00 wm4 <nfxjfg@googlemail.com>:  
> >> > On Thu, 26 Jan 2017 09:16:00 +0100
> >> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >> >  
> >> >> 2017-01-26 9:07 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> >> >>  
> >> >> >> >> > Any metadata you export can and will get copied to a new file when
> >> >> >> >> > remuxing, therefor exporting arbitrary info that isn't actual stream
> >> >> >> >> > metadata tags in metadata is problematic - it carries over to the
> >> >> >> >> > destination file, in which it would be entirely meaningless.  
> >> >> >> >>
> >> >> >> >> Sorry, I don't understand:
> >> >> >> >> Which application do you mean?  
> >> >> >> >
> >> >> >> > ffmpeg  
> >> >> >>
> >> >> >> Didn't I ping yesterday a patch that avoids this?
> >> >> >> And wasn't this the mail you answered?  
> >> >> >
> >> >> > Sorry, I couldn't find it just now. What was the subject line?  
> >> >>
> >> >> My mailer (gmail) shows it as the first mail of this thread, so
> >> >> it (hopefully) uses the same subject.  
> >> >
> >> > Sorry, I missed that. But this code is not called for remuxing,
> >> > or is it?  
> >>
> >> Afair, this behaviour (copying the vendor on remuxing) is
> >> consistent with the Apple documentation.  
> >
> > What if you remux to mkv?  
> 
> Afair, copying the vendor tag on remuxing is consistent with
> its documentation.

Well, the Apple docs don't have anything to do with mkv?
Carl Eugen Hoyos Jan. 27, 2017, 8:05 a.m.
2017-01-27 8:56 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> On Fri, 27 Jan 2017 08:26:26 +0100
> Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
>> 2017-01-27 7:04 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
>> > On Thu, 26 Jan 2017 18:06:39 +0100
>> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>> >
>> >> 2017-01-26 9:26 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
>> >> > On Thu, 26 Jan 2017 09:16:00 +0100
>> >> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>> >> >
>> >> >> 2017-01-26 9:07 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
>> >> >>
>> >> >> >> >> > Any metadata you export can and will get copied to a new file when
>> >> >> >> >> > remuxing, therefor exporting arbitrary info that isn't actual stream
>> >> >> >> >> > metadata tags in metadata is problematic - it carries over to the
>> >> >> >> >> > destination file, in which it would be entirely meaningless.
>> >> >> >> >>
>> >> >> >> >> Sorry, I don't understand:
>> >> >> >> >> Which application do you mean?
>> >> >> >> >
>> >> >> >> > ffmpeg
>> >> >> >>
>> >> >> >> Didn't I ping yesterday a patch that avoids this?
>> >> >> >> And wasn't this the mail you answered?
>> >> >> >
>> >> >> > Sorry, I couldn't find it just now. What was the subject line?
>> >> >>
>> >> >> My mailer (gmail) shows it as the first mail of this thread, so
>> >> >> it (hopefully) uses the same subject.
>> >> >
>> >> > Sorry, I missed that. But this code is not called for remuxing,
>> >> > or is it?
>> >>
>> >> Afair, this behaviour (copying the vendor on remuxing) is
>> >> consistent with the Apple documentation.
>> >
>> > What if you remux to mkv?
>>
>> Afair, copying the vendor tag on remuxing is consistent with
>> its documentation.
>
> Well, the Apple docs don't have anything to do with mkv?

So mkv has a specific definition of the vendor metadata?
I did not immediately find it, could you point me to it?

Carl Eugen
wm4 Jan. 27, 2017, 8:17 a.m.
On Fri, 27 Jan 2017 09:05:15 +0100
Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> 2017-01-27 8:56 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> > On Fri, 27 Jan 2017 08:26:26 +0100
> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >  
> >> 2017-01-27 7:04 GMT+01:00 wm4 <nfxjfg@googlemail.com>:  
> >> > On Thu, 26 Jan 2017 18:06:39 +0100
> >> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >> >  
> >> >> 2017-01-26 9:26 GMT+01:00 wm4 <nfxjfg@googlemail.com>:  
> >> >> > On Thu, 26 Jan 2017 09:16:00 +0100
> >> >> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >> >> >  
> >> >> >> 2017-01-26 9:07 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> >> >> >>  
> >> >> >> >> >> > Any metadata you export can and will get copied to a new file when
> >> >> >> >> >> > remuxing, therefor exporting arbitrary info that isn't actual stream
> >> >> >> >> >> > metadata tags in metadata is problematic - it carries over to the
> >> >> >> >> >> > destination file, in which it would be entirely meaningless.  
> >> >> >> >> >>
> >> >> >> >> >> Sorry, I don't understand:
> >> >> >> >> >> Which application do you mean?  
> >> >> >> >> >
> >> >> >> >> > ffmpeg  
> >> >> >> >>
> >> >> >> >> Didn't I ping yesterday a patch that avoids this?
> >> >> >> >> And wasn't this the mail you answered?  
> >> >> >> >
> >> >> >> > Sorry, I couldn't find it just now. What was the subject line?  
> >> >> >>
> >> >> >> My mailer (gmail) shows it as the first mail of this thread, so
> >> >> >> it (hopefully) uses the same subject.  
> >> >> >
> >> >> > Sorry, I missed that. But this code is not called for remuxing,
> >> >> > or is it?  
> >> >>
> >> >> Afair, this behaviour (copying the vendor on remuxing) is
> >> >> consistent with the Apple documentation.  
> >> >
> >> > What if you remux to mkv?  
> >>
> >> Afair, copying the vendor tag on remuxing is consistent with
> >> its documentation.  
> >
> > Well, the Apple docs don't have anything to do with mkv?  
> 
> So mkv has a specific definition of the vendor metadata?
> I did not immediately find it, could you point me to it?

You're completely misunderstanding.
Carl Eugen Hoyos Jan. 27, 2017, 8:39 a.m.
2017-01-27 9:17 GMT+01:00 wm4 <nfxjfg@googlemail.com>:

> You're completely misunderstanding.

Would you mind to elaborate?

Carl Eugen
wm4 Jan. 27, 2017, 9:09 a.m.
On Fri, 27 Jan 2017 09:39:03 +0100
Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> 2017-01-27 9:17 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> 
> > You're completely misunderstanding.  
> 
> Would you mind to elaborate?

FFmpeg shouldn't mux codec-specific tags into a different container.

Your ffmpeg.c patch works for transcoding only, not remuxing. Also, an
API user can't distinguish user tags and container-specific metadata
added by libavformat, which is a problem.

In theory, you have to add this information as side-data. Adding a new
side-data type is a bit roundabout, though.
Carl Eugen Hoyos Jan. 27, 2017, 9:19 a.m.
2017-01-27 10:09 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> On Fri, 27 Jan 2017 09:39:03 +0100
> Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
>> 2017-01-27 9:17 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
>>
>> > You're completely misunderstanding.
>>
>> Would you mind to elaborate?
>
> FFmpeg shouldn't mux codec-specific tags into a different
> container.

This is not codec-specific, at least not in the sense that
would make a difference for other containers.

> Your ffmpeg.c patch works for transcoding only, not remuxing.

That's because it makes sense to remux "vendor" metadata.

Carl Eugen
wm4 Jan. 27, 2017, 9:29 a.m.
On Fri, 27 Jan 2017 10:19:32 +0100
Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> 2017-01-27 10:09 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> > On Fri, 27 Jan 2017 09:39:03 +0100
> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >  
> >> 2017-01-27 9:17 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> >>  
> >> > You're completely misunderstanding.  
> >>
> >> Would you mind to elaborate?  
> >
> > FFmpeg shouldn't mux codec-specific tags into a different
> > container.  
> 
> This is not codec-specific, at least not in the sense that
> would make a difference for other containers.
> 
> > Your ffmpeg.c patch works for transcoding only, not remuxing.  
> 
> That's because it makes sense to remux "vendor" metadata.

No, technical metadata that is "hidden" is different from user-visible
tags that contain non-technical information about the media. Don't mix
them. FFmpeg does, and this is bad.
Carl Eugen Hoyos Jan. 27, 2017, 9:38 a.m.
2017-01-27 10:29 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> On Fri, 27 Jan 2017 10:19:32 +0100
> Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
>> 2017-01-27 10:09 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
>> > On Fri, 27 Jan 2017 09:39:03 +0100
>> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>> >
>> >> 2017-01-27 9:17 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
>> >>
>> >> > You're completely misunderstanding.
>> >>
>> >> Would you mind to elaborate?
>> >
>> > FFmpeg shouldn't mux codec-specific tags into a different
>> > container.
>>
>> This is not codec-specific, at least not in the sense that
>> would make a difference for other containers.
>>
>> > Your ffmpeg.c patch works for transcoding only, not remuxing.
>>
>> That's because it makes sense to remux "vendor" metadata.
>
> No, technical metadata that is "hidden" is different from user-visible
> tags that contain non-technical information about the media.

This is non-technical information that should be user-visible.

Carl Eugen
wm4 Jan. 27, 2017, 9:42 a.m.
On Fri, 27 Jan 2017 10:38:23 +0100
Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> 2017-01-27 10:29 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> > On Fri, 27 Jan 2017 10:19:32 +0100
> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >  
> >> 2017-01-27 10:09 GMT+01:00 wm4 <nfxjfg@googlemail.com>:  
> >> > On Fri, 27 Jan 2017 09:39:03 +0100
> >> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >> >  
> >> >> 2017-01-27 9:17 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> >> >>  
> >> >> > You're completely misunderstanding.  
> >> >>
> >> >> Would you mind to elaborate?  
> >> >
> >> > FFmpeg shouldn't mux codec-specific tags into a different
> >> > container.  
> >>
> >> This is not codec-specific, at least not in the sense that
> >> would make a difference for other containers.
> >>  
> >> > Your ffmpeg.c patch works for transcoding only, not remuxing.  
> >>
> >> That's because it makes sense to remux "vendor" metadata.  
> >
> > No, technical metadata that is "hidden" is different from user-visible
> > tags that contain non-technical information about the media.  
> 
> This is non-technical information that should be user-visible.

The vendor tag in your fate change has the value "appl". How should
that be user-visible? You can't seriously tell me that this should show
up in a non-technical UI view.
Paul B Mahol Jan. 27, 2017, 10 a.m.
On 12/12/16, Carl Eugen Hoyos <cehoyos@ag.or.at> wrote:
> Hi!
>
> I saw only after writing this patch that a Google employee has sent
> a more complicated variant of this patch in November (searching for
> a description of the vendor field showed the patch).
>
> Please comment, Carl Eugen
>

-1
Carl Eugen Hoyos Jan. 27, 2017, 10:21 a.m.
2017-01-27 10:42 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> On Fri, 27 Jan 2017 10:38:23 +0100
> Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
>> 2017-01-27 10:29 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
>> > On Fri, 27 Jan 2017 10:19:32 +0100
>> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>> >
>> >> 2017-01-27 10:09 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
>> >> > On Fri, 27 Jan 2017 09:39:03 +0100
>> >> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>> >> >
>> >> >> 2017-01-27 9:17 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
>> >> >>
>> >> >> > You're completely misunderstanding.
>> >> >>
>> >> >> Would you mind to elaborate?
>> >> >
>> >> > FFmpeg shouldn't mux codec-specific tags into a different
>> >> > container.
>> >>
>> >> This is not codec-specific, at least not in the sense that
>> >> would make a difference for other containers.
>> >>
>> >> > Your ffmpeg.c patch works for transcoding only, not remuxing.
>> >>
>> >> That's because it makes sense to remux "vendor" metadata.
>> >
>> > No, technical metadata that is "hidden" is different from user-visible
>> > tags that contain non-technical information about the media.
>>
>> This is non-technical information that should be user-visible.
>
> The vendor tag in your fate change has the value "appl". How should
> that be user-visible?

You mean the information is encoded in such a difficult manner
that no user will be able to decipher it?
I really wish you were a little more serious when trying to slow down
FFmpeg development.

> You can't seriously tell me that this should show
> up in a non-technical UI view.

Since you are blocking much more important patches, I should
probably not spend so much time discussing this one. But the
fact that I needed this patch for debugging and that I found  out
afterwards that other developers need it too sufficiently indicates
to me showing, exporting and remuxing the vendor tag makes sense.

Carl Eugen
wm4 Jan. 27, 2017, 10:55 a.m.
On Fri, 27 Jan 2017 11:21:08 +0100
Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> 2017-01-27 10:42 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> > On Fri, 27 Jan 2017 10:38:23 +0100
> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >  
> >> 2017-01-27 10:29 GMT+01:00 wm4 <nfxjfg@googlemail.com>:  
> >> > On Fri, 27 Jan 2017 10:19:32 +0100
> >> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >> >  
> >> >> 2017-01-27 10:09 GMT+01:00 wm4 <nfxjfg@googlemail.com>:  
> >> >> > On Fri, 27 Jan 2017 09:39:03 +0100
> >> >> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >> >> >  
> >> >> >> 2017-01-27 9:17 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> >> >> >>  
> >> >> >> > You're completely misunderstanding.  
> >> >> >>
> >> >> >> Would you mind to elaborate?  
> >> >> >
> >> >> > FFmpeg shouldn't mux codec-specific tags into a different
> >> >> > container.  
> >> >>
> >> >> This is not codec-specific, at least not in the sense that
> >> >> would make a difference for other containers.
> >> >>  
> >> >> > Your ffmpeg.c patch works for transcoding only, not remuxing.  
> >> >>
> >> >> That's because it makes sense to remux "vendor" metadata.  
> >> >
> >> > No, technical metadata that is "hidden" is different from user-visible
> >> > tags that contain non-technical information about the media.  
> >>
> >> This is non-technical information that should be user-visible.  
> >
> > The vendor tag in your fate change has the value "appl". How should
> > that be user-visible?  
> 
> You mean the information is encoded in such a difficult manner
> that no user will be able to decipher it?

That too. It's essentially a FourCC. Most people don't know what to do
with FourCC. It's something for developers or otherwise technically
inclined people.

While this strengthens my argument, it's not really the main point.

> I really wish you were a little more serious when trying to slow down
> FFmpeg development.

Please refrain from personal attacks. I'm not trying to slow down
development, I'm trying to improve the general quality of FFmpeg. In
fact, that's the main point of patch reviews: point out weak spots,
request improvements, and so on.

In this specific case, you're exporting a technical details as
user tags. This is a practice that should stop. Also, writing
essentially random garbage as tags or writing it to files in a manner
that doesn't make sense is not what I associate with quality. What
sense does it make to write a stringified FourCC as user-tag to, say,
a mkv file?

Last but not least, it's questionable whether this is a meaningful
feature at all. Why not use a tool that's more suited for the job, like
a mp4-specific file dumper? (Why did you not do the same for audio?)

To put this into perspective, mediainfo -f doesn't seem to export this
value.

> > You can't seriously tell me that this should show
> > up in a non-technical UI view.  
> 
> Since you are blocking much more important patches, I should
> probably not spend so much time discussing this one. But the
> fact that I needed this patch for debugging and that I found  out
> afterwards that other developers need it too sufficiently indicates
> to me showing, exporting and remuxing the vendor tag makes sense.

For debugging you probably want to use a tool like boxdumper. FFmpeg
can't and won't be able to do "everything" a more suited tool can do,
because it's simply out of scope, and would flood us with thousands of
normally useless details.

I'm also sad to see that you consider discussing patches an annoyance.

Besides, aren't you one of the main offenders when it comes to
stubbornly blocking certain changes?
Carl Eugen Hoyos Jan. 27, 2017, 11:35 a.m.
2017-01-27 11:55 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> On Fri, 27 Jan 2017 11:21:08 +0100
> Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
>> 2017-01-27 10:42 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
>> > On Fri, 27 Jan 2017 10:38:23 +0100
>> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>> >
>> >> 2017-01-27 10:29 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
>> >> > On Fri, 27 Jan 2017 10:19:32 +0100
>> >> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>> >> >
>> >> >> 2017-01-27 10:09 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
>> >> >> > On Fri, 27 Jan 2017 09:39:03 +0100
>> >> >> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>> >> >> >
>> >> >> >> 2017-01-27 9:17 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
>> >> >> >>
>> >> >> >> > You're completely misunderstanding.
>> >> >> >>
>> >> >> >> Would you mind to elaborate?
>> >> >> >
>> >> >> > FFmpeg shouldn't mux codec-specific tags into a different
>> >> >> > container.
>> >> >>
>> >> >> This is not codec-specific, at least not in the sense that
>> >> >> would make a difference for other containers.
>> >> >>
>> >> >> > Your ffmpeg.c patch works for transcoding only, not remuxing.
>> >> >>
>> >> >> That's because it makes sense to remux "vendor" metadata.
>> >> >
>> >> > No, technical metadata that is "hidden" is different from user-visible
>> >> > tags that contain non-technical information about the media.
>> >>
>> >> This is non-technical information that should be user-visible.
>> >
>> > The vendor tag in your fate change has the value "appl". How should
>> > that be user-visible?
>>
>> You mean the information is encoded in such a difficult manner
>> that no user will be able to decipher it?
>
> That too. It's essentially a FourCC. Most people don't know what to do
> with FourCC. It's something for developers or otherwise technically
> inclined people.

I disagree.
(And so do other developers it seems.)

> While this strengthens my argument, it's not really the main point.
>
>> I really wish you were a little more serious when trying to slow down
>> FFmpeg development.
>
> Please refrain from personal attacks.

I do not consider this a personal attack: People you seem to respect
have often requested (publically and in private) that FFmpeg development
has to slow down - I strongly disagree with them but you have said in the
past that you agree with them.

> I'm not trying to slow down
> development, I'm trying to improve the general quality of FFmpeg. In
> fact, that's the main point of patch reviews: point out weak spots,
> request improvements, and so on.

But that is not how you started this thread, or was it?

And that's apart from the fact that you have threatened contributors
away in the past.

> In this specific case, you're exporting a technical details as
> user tags. This is a practice that should stop. Also, writing
> essentially random garbage as tags or writing it to files in a manner
> that doesn't make sense is not what I associate with quality. What
> sense does it make to write a stringified FourCC as user-tag to, say,
> a mkv file?

I do not understand in which way the vendor tag "doesn't make sense"
in any container.

> Last but not least, it's questionable whether this is a meaningful
> feature at all. Why not use a tool that's more suited for the job, like
> a mp4-specific file dumper? (Why did you not do the same for audio?)

I would really, really prefer if all necessary debug information would be
printed by FFmpeg, not a third-party toold.

> To put this into perspective, mediainfo -f doesn't seem to export this
> value.

This does not sound like a useful argument to me.

>> > You can't seriously tell me that this should show
>> > up in a non-technical UI view.
>>
>> Since you are blocking much more important patches, I should
>> probably not spend so much time discussing this one. But the
>> fact that I needed this patch for debugging and that I found  out
>> afterwards that other developers need it too sufficiently indicates
>> to me showing, exporting and remuxing the vendor tag makes sense.
>
> For debugging you probably want to use a tool like boxdumper.

See above.

> FFmpeg can't and won't be able to do "everything" a more suited tool
> can do, because it's simply out of scope, and would flood us with
> thousands of normally useless details.

How is exporting properties of a media file out-of-scope for FFmpeg?

> I'm also sad to see that you consider discussing patches an annoyance.

I wish you would have commented earlier, the way you did it gave me
the impression you were only interested in annoying me.

> Besides, aren't you one of the main offenders when it comes to
> stubbornly blocking certain changes?

Compared to you?

Carl Eugen
wm4 Jan. 27, 2017, 11:58 a.m.
On Fri, 27 Jan 2017 12:35:25 +0100
Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> 2017-01-27 11:55 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> > On Fri, 27 Jan 2017 11:21:08 +0100
> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >  
> >> 2017-01-27 10:42 GMT+01:00 wm4 <nfxjfg@googlemail.com>:  
> >> > On Fri, 27 Jan 2017 10:38:23 +0100
> >> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >> >  
> >> >> 2017-01-27 10:29 GMT+01:00 wm4 <nfxjfg@googlemail.com>:  
> >> >> > On Fri, 27 Jan 2017 10:19:32 +0100
> >> >> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >> >> >  
> >> >> >> 2017-01-27 10:09 GMT+01:00 wm4 <nfxjfg@googlemail.com>:  
> >> >> >> > On Fri, 27 Jan 2017 09:39:03 +0100
> >> >> >> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >> >> >> >  
> >> >> >> >> 2017-01-27 9:17 GMT+01:00 wm4 <nfxjfg@googlemail.com>:
> >> >> >> >>  
> >> >> >> >> > You're completely misunderstanding.  
> >> >> >> >>
> >> >> >> >> Would you mind to elaborate?  
> >> >> >> >
> >> >> >> > FFmpeg shouldn't mux codec-specific tags into a different
> >> >> >> > container.  
> >> >> >>
> >> >> >> This is not codec-specific, at least not in the sense that
> >> >> >> would make a difference for other containers.
> >> >> >>  
> >> >> >> > Your ffmpeg.c patch works for transcoding only, not remuxing.  
> >> >> >>
> >> >> >> That's because it makes sense to remux "vendor" metadata.  
> >> >> >
> >> >> > No, technical metadata that is "hidden" is different from user-visible
> >> >> > tags that contain non-technical information about the media.  
> >> >>
> >> >> This is non-technical information that should be user-visible.  
> >> >
> >> > The vendor tag in your fate change has the value "appl". How should
> >> > that be user-visible?  
> >>
> >> You mean the information is encoded in such a difficult manner
> >> that no user will be able to decipher it?  
> >
> > That too. It's essentially a FourCC. Most people don't know what to do
> > with FourCC. It's something for developers or otherwise technically
> > inclined people.  
> 
> I disagree.
> (And so do other developers it seems.)

Which other developers?

> > While this strengthens my argument, it's not really the main point.
> >  
> >> I really wish you were a little more serious when trying to slow down
> >> FFmpeg development.  
> >
> > Please refrain from personal attacks.  
> 
> I do not consider this a personal attack: People you seem to respect
> have often requested (publically and in private) that FFmpeg development
> has to slow down - I strongly disagree with them but you have said in the
> past that you agree with them.

Where did they say this? Do you have a link or an example?

> > I'm not trying to slow down
> > development, I'm trying to improve the general quality of FFmpeg. In
> > fact, that's the main point of patch reviews: point out weak spots,
> > request improvements, and so on.  
> 
> But that is not how you started this thread, or was it?

Did you?

> And that's apart from the fact that you have threatened contributors
> away in the past.

How many people have you chased away from the bug tracker?

> > In this specific case, you're exporting a technical details as
> > user tags. This is a practice that should stop. Also, writing
> > essentially random garbage as tags or writing it to files in a manner
> > that doesn't make sense is not what I associate with quality. What
> > sense does it make to write a stringified FourCC as user-tag to, say,
> > a mkv file?  
> 
> I do not understand in which way the vendor tag "doesn't make sense"
> in any container.

See the paragraph you quoted.

Let me ask you again: how does an Apple-specific FourCC make sense in
mkv (except maybe the Qt muxing support)?

> > Last but not least, it's questionable whether this is a meaningful
> > feature at all. Why not use a tool that's more suited for the job, like
> > a mp4-specific file dumper? (Why did you not do the same for audio?)  
> 
> I would really, really prefer if all necessary debug information would be
> printed by FFmpeg, not a third-party toold.

Adding user-visible changes "for debugging" is usually a bad argument.

You could output it with an av_log with debug log level if you must.

> > To put this into perspective, mediainfo -f doesn't seem to export this
> > value.  
> 
> This does not sound like a useful argument to me.

It does to me.

> >> > You can't seriously tell me that this should show
> >> > up in a non-technical UI view.  
> >>
> >> Since you are blocking much more important patches, I should
> >> probably not spend so much time discussing this one. But the
> >> fact that I needed this patch for debugging and that I found  out
> >> afterwards that other developers need it too sufficiently indicates
> >> to me showing, exporting and remuxing the vendor tag makes sense.  
> >
> > For debugging you probably want to use a tool like boxdumper.  
> 
> See above.

See above.

> > FFmpeg can't and won't be able to do "everything" a more suited tool
> > can do, because it's simply out of scope, and would flood us with
> > thousands of normally useless details.  
> 
> How is exporting properties of a media file out-of-scope for FFmpeg?

Don't twist my words. I wasn't talking about "exporting properties",
but this specific case.

> > I'm also sad to see that you consider discussing patches an annoyance.  
> 
> I wish you would have commented earlier, the way you did it gave me
> the impression you were only interested in annoying me.

Oh look, a personal attack mixed with plenty of mind-bending bullshit
again.

> > Besides, aren't you one of the main offenders when it comes to
> > stubbornly blocking certain changes?  
> 
> Compared to you?

I'm less stubborn than you. Because I give in when I see that I'm
wrong, or if the opposition is too large.
Michael Niedermayer Jan. 27, 2017, 9:47 p.m.
On Mon, Dec 12, 2016 at 12:12:37PM +0100, Carl Eugen Hoyos wrote:
> Hi!
> 
> I saw only after writing this patch that a Google employee has sent 
> a more complicated variant of this patch in November (searching for 
> a description of the vendor field showed the patch).
> 
> Please comment, Carl Eugen

It seems some developers dislike exporting 4 byte vendor tags as
metadata.
Maybe using the table from:
http://www.sno.phy.queensu.ca/~phil/exiftool/TagNames/QuickTime.html#VendorID

And instead exporting the full vendor name as metadata from the demuxer
and converting it back to a 4byte value using the same table in the
muxer would be an option ?

If this doesnt result in a resolution of the disagreements then feel
free to ignore my suggestion, its not my intent to complicate this
by adding additional requests

thx

[...]

Patch hide | download patch | download mbox

diff --git a/ffmpeg.c b/ffmpeg.c
index e4890a4..f58e9b1 100644
--- a/ffmpeg.c
+++ b/ffmpeg.c
@@ -3014,6 +3014,7 @@  static void set_encoder_id(OutputFile *of, OutputStream *ost)
     av_strlcat(encoder_string, ost->enc->name, encoder_string_len);
     av_dict_set(&ost->st->metadata, "encoder",  encoder_string,
                 AV_DICT_DONT_STRDUP_VAL | AV_DICT_DONT_OVERWRITE);
+    av_dict_set(&ost->st->metadata, "vendor", NULL, 0);
 }
 
 static void parse_forced_key_frames(char *kf, OutputStream *ost,