Message ID | 20180428172421.29763-1-nfxjfg@googlemail.com |
---|---|
State | Accepted |
Commit | 022d4a2114d2586d4807f5810160f0f565ab80d7 |
Headers | show |
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.
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) &&