[FFmpeg-devel] movenc: Tag files generated with strict experimental with a warning

Submitted by Vittorio Giovara on Dec. 2, 2016, 8 p.m.

Details

Message ID 20161202200057.6685-1-vittorio.giovara@gmail.com
State New
Headers show

Commit Message

Vittorio Giovara Dec. 2, 2016, 8 p.m.
This will simplify identifying files that were generated with
unfinished/incomplete/non-standard specifications.

Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com>
---
 libavformat/movenc.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

James Almer Dec. 2, 2016, 8:17 p.m.
On 12/2/2016 5:00 PM, Vittorio Giovara wrote:
> This will simplify identifying files that were generated with
> unfinished/incomplete/non-standard specifications.
> 
> Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com>
> ---
>  libavformat/movenc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> index dc19838..c46bea9 100644
> --- a/libavformat/movenc.c
> +++ b/libavformat/movenc.c
> @@ -5756,6 +5756,7 @@ static int mov_init(AVFormatContext *s)
>                             FF_COMPLIANCE_EXPERIMENTAL);
>                      return AVERROR_EXPERIMENTAL;
>                  }
> +                av_dict_set(&s->metadata, "WARNING", "This file was generated using an unfinished specification, please don't modify your demuxer to support it, should it not work", 0);
>              }
>          } else if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO) {
>              track->timescale = st->codecpar->sample_rate;
> @@ -5802,6 +5803,7 @@ static int mov_init(AVFormatContext *s)
>                             FF_COMPLIANCE_EXPERIMENTAL);
>                      return AVERROR_EXPERIMENTAL;
>                  }
> +                av_dict_set(&s->metadata, "WARNING", "This file was generated using an unfinished specification, please don't modify your demuxer to support it, should it not work", 0);
>              }
>          } else if (st->codecpar->codec_type == AVMEDIA_TYPE_SUBTITLE) {
>              track->timescale = st->time_base.den;

Maybe lavf and lavc should add "experimental" to the container and
stream's metadata alongside the library version, much like how we're
adding the name of the encoder alongside the lavc version to the
stream's metadata.

A warning printed by the muxer not going to help once such a file
is in the wild.
wm4 Dec. 2, 2016, 11:17 p.m.
On Fri, 2 Dec 2016 17:17:23 -0300
James Almer <jamrial@gmail.com> wrote:

> On 12/2/2016 5:00 PM, Vittorio Giovara wrote:
> > This will simplify identifying files that were generated with
> > unfinished/incomplete/non-standard specifications.
> > 
> > Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com>
> > ---
> >  libavformat/movenc.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> > index dc19838..c46bea9 100644
> > --- a/libavformat/movenc.c
> > +++ b/libavformat/movenc.c
> > @@ -5756,6 +5756,7 @@ static int mov_init(AVFormatContext *s)
> >                             FF_COMPLIANCE_EXPERIMENTAL);
> >                      return AVERROR_EXPERIMENTAL;
> >                  }
> > +                av_dict_set(&s->metadata, "WARNING", "This file was generated using an unfinished specification, please don't modify your demuxer to support it, should it not work", 0);
> >              }
> >          } else if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO) {
> >              track->timescale = st->codecpar->sample_rate;
> > @@ -5802,6 +5803,7 @@ static int mov_init(AVFormatContext *s)
> >                             FF_COMPLIANCE_EXPERIMENTAL);
> >                      return AVERROR_EXPERIMENTAL;
> >                  }
> > +                av_dict_set(&s->metadata, "WARNING", "This file was generated using an unfinished specification, please don't modify your demuxer to support it, should it not work", 0);
> >              }
> >          } else if (st->codecpar->codec_type == AVMEDIA_TYPE_SUBTITLE) {
> >              track->timescale = st->time_base.den;  
> 
> Maybe lavf and lavc should add "experimental" to the container and
> stream's metadata alongside the library version, much like how we're
> adding the name of the encoder alongside the lavc version to the
> stream's metadata.
> 
> A warning printed by the muxer not going to help once such a file
> is in the wild.

This patch or the suggestion above sounds like a good idea. I wish it'd
actually be possible to encode a watermark with the warning into the
video stream.
Vittorio Giovara Dec. 6, 2016, 3:48 p.m.
On Fri, Dec 2, 2016 at 3:00 PM, Vittorio Giovara
<vittorio.giovara@gmail.com> wrote:
> This will simplify identifying files that were generated with
> unfinished/incomplete/non-standard specifications.
>
> Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com>
> ---
>  libavformat/movenc.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
> index dc19838..c46bea9 100644
> --- a/libavformat/movenc.c
> +++ b/libavformat/movenc.c
> @@ -5756,6 +5756,7 @@ static int mov_init(AVFormatContext *s)
>                             FF_COMPLIANCE_EXPERIMENTAL);
>                      return AVERROR_EXPERIMENTAL;
>                  }
> +                av_dict_set(&s->metadata, "WARNING", "This file was generated using an unfinished specification, please don't modify your demuxer to support it, should it not work", 0);
>              }
>          } else if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO) {
>              track->timescale = st->codecpar->sample_rate;
> @@ -5802,6 +5803,7 @@ static int mov_init(AVFormatContext *s)
>                             FF_COMPLIANCE_EXPERIMENTAL);
>                      return AVERROR_EXPERIMENTAL;
>                  }
> +                av_dict_set(&s->metadata, "WARNING", "This file was generated using an unfinished specification, please don't modify your demuxer to support it, should it not work", 0);
>              }
>          } else if (st->codecpar->codec_type == AVMEDIA_TYPE_SUBTITLE) {
>              track->timescale = st->time_base.den;
> --

Any opinion about this? Should I assume it is fine as is?
Please CC.
James Almer Dec. 6, 2016, 3:50 p.m.
On 12/2/2016 5:17 PM, James Almer wrote:
> On 12/2/2016 5:00 PM, Vittorio Giovara wrote:
>> This will simplify identifying files that were generated with
>> unfinished/incomplete/non-standard specifications.
>>
>> Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com>
>> ---
>>  libavformat/movenc.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
>> index dc19838..c46bea9 100644
>> --- a/libavformat/movenc.c
>> +++ b/libavformat/movenc.c
>> @@ -5756,6 +5756,7 @@ static int mov_init(AVFormatContext *s)
>>                             FF_COMPLIANCE_EXPERIMENTAL);
>>                      return AVERROR_EXPERIMENTAL;
>>                  }
>> +                av_dict_set(&s->metadata, "WARNING", "This file was generated using an unfinished specification, please don't modify your demuxer to support it, should it not work", 0);
>>              }
>>          } else if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO) {
>>              track->timescale = st->codecpar->sample_rate;
>> @@ -5802,6 +5803,7 @@ static int mov_init(AVFormatContext *s)
>>                             FF_COMPLIANCE_EXPERIMENTAL);
>>                      return AVERROR_EXPERIMENTAL;
>>                  }
>> +                av_dict_set(&s->metadata, "WARNING", "This file was generated using an unfinished specification, please don't modify your demuxer to support it, should it not work", 0);
>>              }
>>          } else if (st->codecpar->codec_type == AVMEDIA_TYPE_SUBTITLE) {
>>              track->timescale = st->time_base.den;
> 
> Maybe lavf and lavc should add "experimental" to the container and
> stream's metadata alongside the library version, much like how we're
> adding the name of the encoder alongside the lavc version to the
> stream's metadata.
> 
> A warning printed by the muxer not going to help once such a file
> is in the wild.

Resending, CCing Vittorio this time.
Vittorio Giovara Dec. 6, 2016, 5:31 p.m.
On Tue, Dec 6, 2016 at 10:50 AM, James Almer <jamrial@gmail.com> wrote:
> On 12/2/2016 5:17 PM, James Almer wrote:
>> On 12/2/2016 5:00 PM, Vittorio Giovara wrote:
>>> This will simplify identifying files that were generated with
>>> unfinished/incomplete/non-standard specifications.
>>>
>>> Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com>
>>> ---
>>>  libavformat/movenc.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>
>> Maybe lavf and lavc should add "experimental" to the container and
>> stream's metadata alongside the library version, much like how we're
>> adding the name of the encoder alongside the lavc version to the
>> stream's metadata.
>>
>> A warning printed by the muxer not going to help once such a file
>> is in the wild.

Yeah I was unsure about that -- are you saying that whenever -strict
experimental is used we should add this tag regardless?

The idea behind this is that in the future, when the standard is
consolidated, if there is a file is in the wild encoded with an
"experimental" codec/container combination and it is not playing
correctly, this tag will make evident that the bug lies within the
file and the playback issues are probably due the unfinished state of
the specification, rather than in in the modern implementation of the
demuxer. In other words, people will be less inclined to hack around
their own demuxer to accommodate for broken files.
James Almer Dec. 6, 2016, 5:59 p.m.
On 12/6/2016 2:31 PM, Vittorio Giovara wrote:
> On Tue, Dec 6, 2016 at 10:50 AM, James Almer <jamrial@gmail.com> wrote:
>> On 12/2/2016 5:17 PM, James Almer wrote:
>>> On 12/2/2016 5:00 PM, Vittorio Giovara wrote:
>>>> This will simplify identifying files that were generated with
>>>> unfinished/incomplete/non-standard specifications.
>>>>
>>>> Signed-off-by: Vittorio Giovara <vittorio.giovara@gmail.com>
>>>> ---
>>>>  libavformat/movenc.c | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>
>>> Maybe lavf and lavc should add "experimental" to the container and
>>> stream's metadata alongside the library version, much like how we're
>>> adding the name of the encoder alongside the lavc version to the
>>> stream's metadata.
>>>
>>> A warning printed by the muxer not going to help once such a file
>>> is in the wild.
> 
> Yeah I was unsure about that -- are you saying that whenever -strict
> experimental is used we should add this tag regardless?
> 
> The idea behind this is that in the future, when the standard is
> consolidated, if there is a file is in the wild encoded with an
> "experimental" codec/container combination and it is not playing
> correctly, this tag will make evident that the bug lies within the
> file and the playback issues are probably due the unfinished state of
> the specification, rather than in in the modern implementation of the
> demuxer. In other words, people will be less inclined to hack around
> their own demuxer to accommodate for broken files.

Oh, i misread the patch. I thought it was printing a warning, not adding
a "WARNING" stream metadata tag.

This patch is ok as is, i guess, but maybe a more general solution like
what i suggested is better.
Having the standard "encoder" tags read something like
"Lavf5x.xx.xx (experimental)" and "Lavc5x.xx.xx vorbis (experimental)"
if -strict experimental is used regardless of format, instead of a non
standard "WARNING" tag in one or two specific containers, which may or
may not be ignored by players and parsers.
Carl Eugen Hoyos Dec. 10, 2016, 2:06 p.m.
2016-12-02 21:00 GMT+01:00 Vittorio Giovara <vittorio.giovara@gmail.com>:
> This will simplify identifying files that were generated with
> unfinished/incomplete/non-standard specifications.

Doesn't the muxer version information already provide this information?
Are random metadata tags valid?

Carl Eugen

Patch hide | download patch | download mbox

diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index dc19838..c46bea9 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -5756,6 +5756,7 @@  static int mov_init(AVFormatContext *s)
                            FF_COMPLIANCE_EXPERIMENTAL);
                     return AVERROR_EXPERIMENTAL;
                 }
+                av_dict_set(&s->metadata, "WARNING", "This file was generated using an unfinished specification, please don't modify your demuxer to support it, should it not work", 0);
             }
         } else if (st->codecpar->codec_type == AVMEDIA_TYPE_AUDIO) {
             track->timescale = st->codecpar->sample_rate;
@@ -5802,6 +5803,7 @@  static int mov_init(AVFormatContext *s)
                            FF_COMPLIANCE_EXPERIMENTAL);
                     return AVERROR_EXPERIMENTAL;
                 }
+                av_dict_set(&s->metadata, "WARNING", "This file was generated using an unfinished specification, please don't modify your demuxer to support it, should it not work", 0);
             }
         } else if (st->codecpar->codec_type == AVMEDIA_TYPE_SUBTITLE) {
             track->timescale = st->time_base.den;