[FFmpeg-devel] avformat/matroskaenc: do not write timebase as framerate

If the API user doesn't set avg_frame_rate, matroskaenc will write the
current timebase as "default duration" for the video track. This makes
no sense, because the "default duration" implies the framerate of the
video. Since the timebase is forced to 1/1000, this will make the
resulting file claim 1000fps.

Drop it and don't write the element. It's optional, so it's better not
to write it if the framerate is unknown.

Strangely does not require FATE changes.
---
 libavformat/matroskaenc.c | 2 --
 1 file changed, 2 deletions(-)

2018-04-28 19:24 GMT+02:00, wm4 <nfxjfg@googlemail.com>:
> If the API user doesn't set avg_frame_rate, matroskaenc will write the
> current timebase as "default duration" for the video track. This makes
> no sense, because the "default duration" implies the framerate of the
> video. Since the timebase is forced to 1/1000, this will make the
> resulting file claim 1000fps.
>
> Drop it and don't write the element. It's optional, so it's better not
> to write it if the framerate is unknown.

(Isn't it default frame duration?)

Please mention ticket #6386 if you commit.

Carl Eugen

On Sat, 28 Apr 2018 19:52:38 +0200
Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> 2018-04-28 19:24 GMT+02:00, wm4 <nfxjfg@googlemail.com>:
> > If the API user doesn't set avg_frame_rate, matroskaenc will write the
> > current timebase as "default duration" for the video track. This makes
> > no sense, because the "default duration" implies the framerate of the
> > video. Since the timebase is forced to 1/1000, this will make the
> > resulting file claim 1000fps.
> >
> > Drop it and don't write the element. It's optional, so it's better not
> > to write it if the framerate is unknown.  
> 
> (Isn't it default frame duration?)

The Matroska "spec" calls it DefaultDuration.

> Please mention ticket #6386 if you commit.

So if this was known to you, and you even made a patch, why did you
never send the patch to the list?

2018-04-28 20:05 GMT+02:00, wm4 <nfxjfg@googlemail.com>:
> On Sat, 28 Apr 2018 19:52:38 +0200
> Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>
>> 2018-04-28 19:24 GMT+02:00, wm4 <nfxjfg@googlemail.com>:
>> > If the API user doesn't set avg_frame_rate, matroskaenc will write the
>> > current timebase as "default duration" for the video track. This makes
>> > no sense, because the "default duration" implies the framerate of the
>> > video. Since the timebase is forced to 1/1000, this will make the
>> > resulting file claim 1000fps.
>> >
>> > Drop it and don't write the element. It's optional, so it's better not
>> > to write it if the framerate is unknown.
>>
>> (Isn't it default frame duration?)
>
> The Matroska "spec" calls it DefaultDuration.

Which sounds more similar to "default frame duration" than
"framerate"...

>> Please mention ticket #6386 if you commit.
>
> So if this was known to you, and you even made a patch, why
> did you never send the patch to the list?

I am not convinced the patch is correct and the OP claimed
that it did not fix the reported issue completely.

(And this was of course not known "to me" but to everybody.)

Carl Eugen

On Sat, 28 Apr 2018 21:18:47 +0200
Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> 2018-04-28 20:05 GMT+02:00, wm4 <nfxjfg@googlemail.com>:
> > On Sat, 28 Apr 2018 19:52:38 +0200
> > Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> >  
> >> 2018-04-28 19:24 GMT+02:00, wm4 <nfxjfg@googlemail.com>:  
> >> > If the API user doesn't set avg_frame_rate, matroskaenc will write the
> >> > current timebase as "default duration" for the video track. This makes
> >> > no sense, because the "default duration" implies the framerate of the
> >> > video. Since the timebase is forced to 1/1000, this will make the
> >> > resulting file claim 1000fps.
> >> >
> >> > Drop it and don't write the element. It's optional, so it's better not
> >> > to write it if the framerate is unknown.  
> >>
> >> (Isn't it default frame duration?)  
> >
> > The Matroska "spec" calls it DefaultDuration.  
> 
> Which sounds more similar to "default frame duration" than
> "framerate"...

It's the same thing, really. Technically the framerate is the inverse
of it.

> >> Please mention ticket #6386 if you commit.  
> >
> > So if this was known to you, and you even made a patch, why
> > did you never send the patch to the list?  
> 
> I am not convinced the patch is correct and the OP claimed
> that it did not fix the reported issue completely.
> 
> (And this was of course not known "to me" but to everybody.)

It definitely does fix the issue that ffmpeg writes files that mkv
demuxers, including ffmpeg's, detect as having video framerate of
1000hz.

I don't know what that issue is about and I don't care, it's just that
you posted a patch there that does exactly the same thing as mine. I'll
gladly leave that to you.

Will push tomorrow.

On Sat, 28 Apr 2018 19:24:21 +0200
wm4 <nfxjfg@googlemail.com> wrote:

> If the API user doesn't set avg_frame_rate, matroskaenc will write the
> current timebase as "default duration" for the video track. This makes
> no sense, because the "default duration" implies the framerate of the
> video. Since the timebase is forced to 1/1000, this will make the
> resulting file claim 1000fps.
> 
> Drop it and don't write the element. It's optional, so it's better not
> to write it if the framerate is unknown.
> 
> Strangely does not require FATE changes.
> ---
>  libavformat/matroskaenc.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index 5950b4de44..b7ff1950d3 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -1304,8 +1304,6 @@ static int mkv_write_track(AVFormatContext *s, MatroskaMuxContext *mkv,
>          if(   st->avg_frame_rate.num > 0 && st->avg_frame_rate.den > 0
>             && av_cmp_q(av_inv_q(st->avg_frame_rate), st->time_base) > 0)
>              put_ebml_uint(pb, MATROSKA_ID_TRACKDEFAULTDURATION, 1000000000LL * st->avg_frame_rate.den / st->avg_frame_rate.num);
> -        else
> -            put_ebml_uint(pb, MATROSKA_ID_TRACKDEFAULTDURATION, 1000000000LL * st->time_base.num / st->time_base.den);
>  
>          if (!native_id &&
>              ff_codec_get_tag(ff_codec_movvideo_tags, par->codec_id) &&

Pushed.

2018-05-04 15:00 GMT+02:00, wm4 <nfxjfg@googlemail.com>:
> On Sat, 28 Apr 2018 19:24:21 +0200
> wm4 <nfxjfg@googlemail.com> wrote:
>
>> If the API user doesn't set avg_frame_rate, matroskaenc will write the
>> current timebase as "default duration" for the video track. This makes
>> no sense, because the "default duration" implies the framerate of the
>> video. Since the timebase is forced to 1/1000, this will make the
>> resulting file claim 1000fps.
>>
>> Drop it and don't write the element. It's optional, so it's better not
>> to write it if the framerate is unknown.
>>
>> Strangely does not require FATE changes.
>> ---
>>  libavformat/matroskaenc.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
>> index 5950b4de44..b7ff1950d3 100644
>> --- a/libavformat/matroskaenc.c
>> +++ b/libavformat/matroskaenc.c
>> @@ -1304,8 +1304,6 @@ static int mkv_write_track(AVFormatContext *s,
>> MatroskaMuxContext *mkv,
>>          if(   st->avg_frame_rate.num > 0 && st->avg_frame_rate.den > 0
>>             && av_cmp_q(av_inv_q(st->avg_frame_rate), st->time_base) > 0)
>>              put_ebml_uint(pb, MATROSKA_ID_TRACKDEFAULTDURATION,
>> 1000000000LL * st->avg_frame_rate.den / st->avg_frame_rate.num);
>> -        else
>> -            put_ebml_uint(pb, MATROSKA_ID_TRACKDEFAULTDURATION,
>> 1000000000LL * st->time_base.num / st->time_base.den);
>>
>>          if (!native_id &&
>>              ff_codec_get_tag(ff_codec_movvideo_tags, par->codec_id) &&
>
> Pushed.

Good to know that your behaviour hasn't improved.

Carl Eugen

On Sat, 5 May 2018 02:35:52 +0200
Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> 2018-05-04 15:00 GMT+02:00, wm4 <nfxjfg@googlemail.com>:
> > On Sat, 28 Apr 2018 19:24:21 +0200
> > wm4 <nfxjfg@googlemail.com> wrote:
> >  
> >> If the API user doesn't set avg_frame_rate, matroskaenc will write the
> >> current timebase as "default duration" for the video track. This makes
> >> no sense, because the "default duration" implies the framerate of the
> >> video. Since the timebase is forced to 1/1000, this will make the
> >> resulting file claim 1000fps.
> >>
> >> Drop it and don't write the element. It's optional, so it's better not
> >> to write it if the framerate is unknown.
> >>
> >> Strangely does not require FATE changes.
> >> ---
> >>  libavformat/matroskaenc.c | 2 --
> >>  1 file changed, 2 deletions(-)
> >>
> >> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> >> index 5950b4de44..b7ff1950d3 100644
> >> --- a/libavformat/matroskaenc.c
> >> +++ b/libavformat/matroskaenc.c
> >> @@ -1304,8 +1304,6 @@ static int mkv_write_track(AVFormatContext *s,
> >> MatroskaMuxContext *mkv,
> >>          if(   st->avg_frame_rate.num > 0 && st->avg_frame_rate.den > 0
> >>             && av_cmp_q(av_inv_q(st->avg_frame_rate), st->time_base) > 0)
> >>              put_ebml_uint(pb, MATROSKA_ID_TRACKDEFAULTDURATION,
> >> 1000000000LL * st->avg_frame_rate.den / st->avg_frame_rate.num);
> >> -        else
> >> -            put_ebml_uint(pb, MATROSKA_ID_TRACKDEFAULTDURATION,
> >> 1000000000LL * st->time_base.num / st->time_base.den);
> >>
> >>          if (!native_id &&
> >>              ff_codec_get_tag(ff_codec_movvideo_tags, par->codec_id) &&  
> >
> > Pushed.  
> 
> Good to know that your behaviour hasn't improved.

Wow yet another passive aggressive attack against for no reason. I
think I'll just block all your mails so I have peace. Just because I
participate in this project it doesn't mean I have to tolerate your
obnoxious rudeness.