| Message ID | 20160906122112.GM4692@nb4 |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Sep 6, 2016 at 2:21 PM, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Tue, Sep 06, 2016 at 02:18:35PM +0200, Michael Niedermayer wrote: >> On Tue, Sep 06, 2016 at 04:57:06AM +0200, Michael Niedermayer wrote: >> > On Mon, Sep 05, 2016 at 10:04:35PM -0300, James Almer wrote: >> > > On 9/5/2016 12:41 PM, Michael Niedermayer wrote: >> > > > On Mon, Sep 05, 2016 at 04:41:52PM +0200, Clément Bœsch wrote: >> > > >> From: Clément Bœsch <clement@stupeflix.com> >> > > >> >> > > >> These adjusted codec fields do not seem to be in use anymore and prevent >> > > >> the convert of ffmpeg*.c to codecpar. >> > > > >> > > > ./ffmpeg -i ~/tickets/4914/xdcam8mp2-1s_small.ts -c:v copy out.mxf >> > > > fails, no output anymore >> > > > >> > > > ./ffmpeg -i matrixbench_mpeg2.mpg -c:v copy -t 1 test.avi >> > > > the output now has 600fps >> > > >> > > Even with this code in place the resulting stream in the avi is reported >> > > as 100 fps. >> > >> > that seems to be a regression since >> > 6f69f7a8bf6a0d013985578df2ef42ee6b1c7994 >> > >> > IIRC the intended timebase is 1/50 for this kind of content >> > (allowing the support of interlaced and field duplicated content to >> > appear later) >> > >> > >> > > And with or without the code, the resulting files play the >> > > same with the players i tried. >> > >> > Higher framerates / finer timebases need noticably more space to >> > be stored in avi, thats not the case for other formats and thats >> > one reason why avi is treated as a special case. >> > >> > ill try to look tomorrow why its 100fps since the previous >> > codecpar patches. Though 100fps is not nearly as bad as 600fps >> > 600 has ~6 times the overhead >> >> This regression is caused by ticks_per_frame beiing incorrect >> >> Ill send a patch to fix this > > patch attached > We don't have time_base in codecpar, so why do we need ticks per frame in it? Which time_base does it modify the interpretation of? The field should be bundled with that, then. - Hendrik
On Tue, Sep 06, 2016 at 02:39:11PM +0200, Hendrik Leppkes wrote: > On Tue, Sep 6, 2016 at 2:21 PM, Michael Niedermayer > <michael@niedermayer.cc> wrote: > > On Tue, Sep 06, 2016 at 02:18:35PM +0200, Michael Niedermayer wrote: > >> On Tue, Sep 06, 2016 at 04:57:06AM +0200, Michael Niedermayer wrote: > >> > On Mon, Sep 05, 2016 at 10:04:35PM -0300, James Almer wrote: > >> > > On 9/5/2016 12:41 PM, Michael Niedermayer wrote: > >> > > > On Mon, Sep 05, 2016 at 04:41:52PM +0200, Clément Bœsch wrote: > >> > > >> From: Clément Bœsch <clement@stupeflix.com> > >> > > >> > >> > > >> These adjusted codec fields do not seem to be in use anymore and prevent > >> > > >> the convert of ffmpeg*.c to codecpar. > >> > > > > >> > > > ./ffmpeg -i ~/tickets/4914/xdcam8mp2-1s_small.ts -c:v copy out.mxf > >> > > > fails, no output anymore > >> > > > > >> > > > ./ffmpeg -i matrixbench_mpeg2.mpg -c:v copy -t 1 test.avi > >> > > > the output now has 600fps > >> > > > >> > > Even with this code in place the resulting stream in the avi is reported > >> > > as 100 fps. > >> > > >> > that seems to be a regression since > >> > 6f69f7a8bf6a0d013985578df2ef42ee6b1c7994 > >> > > >> > IIRC the intended timebase is 1/50 for this kind of content > >> > (allowing the support of interlaced and field duplicated content to > >> > appear later) > >> > > >> > > >> > > And with or without the code, the resulting files play the > >> > > same with the players i tried. > >> > > >> > Higher framerates / finer timebases need noticably more space to > >> > be stored in avi, thats not the case for other formats and thats > >> > one reason why avi is treated as a special case. > >> > > >> > ill try to look tomorrow why its 100fps since the previous > >> > codecpar patches. Though 100fps is not nearly as bad as 600fps > >> > 600 has ~6 times the overhead > >> > >> This regression is caused by ticks_per_frame beiing incorrect > >> > >> Ill send a patch to fix this > > > > patch attached > > > > We don't have time_base in codecpar, so why do we need ticks per frame in it? > > Which time_base does it modify the interpretation of? The field should > be bundled with that, then. When do we have a mismatch of st->time_base and that "codec time base"? What if the ticks_per_frame was at AVStream level?
On 9/6/2016 9:57 AM, Clément Bœsch wrote: >> We don't have time_base in codecpar, so why do we need ticks per frame in it? >> > >> > Which time_base does it modify the interpretation of? The field should >> > be bundled with that, then. > When do we have a mismatch of st->time_base and that "codec time base"? st->time_base can be anything. Matroska for example is always 1/1000, and mpegps 1/90000. "Codec time_base" seems to be codec frame_rate * ticks_per_frame, so usually the same as frame_rate, except for codecs like h264 and mpeg2 where it's twice that.
On Tue, Sep 06, 2016 at 11:32:24AM -0300, James Almer wrote: > On 9/6/2016 9:57 AM, Clément Bœsch wrote: > >> We don't have time_base in codecpar, so why do we need ticks per frame in it? > >> > > >> > Which time_base does it modify the interpretation of? The field should > >> > be bundled with that, then. > > When do we have a mismatch of st->time_base and that "codec time base"? > > st->time_base can be anything. Matroska for example is always 1/1000, > and mpegps 1/90000. > "Codec time_base" seems to be codec frame_rate * ticks_per_frame, so > usually the same as frame_rate, except for codecs like h264 and mpeg2 > where it's twice that. But you could deduce one from another if you have ticks per frame? That means we could have only AVStream.{time_base,ticks_per_frame} and we will be able to deal with all the situations were lavf/mux* need such info, and thus not need to transmit anything in AVCodecParameters?
On Tue, Sep 06, 2016 at 02:39:11PM +0200, Hendrik Leppkes wrote: > On Tue, Sep 6, 2016 at 2:21 PM, Michael Niedermayer > <michael@niedermayer.cc> wrote: > > On Tue, Sep 06, 2016 at 02:18:35PM +0200, Michael Niedermayer wrote: > >> On Tue, Sep 06, 2016 at 04:57:06AM +0200, Michael Niedermayer wrote: > >> > On Mon, Sep 05, 2016 at 10:04:35PM -0300, James Almer wrote: > >> > > On 9/5/2016 12:41 PM, Michael Niedermayer wrote: > >> > > > On Mon, Sep 05, 2016 at 04:41:52PM +0200, Clément Bœsch wrote: > >> > > >> From: Clément Bœsch <clement@stupeflix.com> > >> > > >> > >> > > >> These adjusted codec fields do not seem to be in use anymore and prevent > >> > > >> the convert of ffmpeg*.c to codecpar. > >> > > > > >> > > > ./ffmpeg -i ~/tickets/4914/xdcam8mp2-1s_small.ts -c:v copy out.mxf > >> > > > fails, no output anymore > >> > > > > >> > > > ./ffmpeg -i matrixbench_mpeg2.mpg -c:v copy -t 1 test.avi > >> > > > the output now has 600fps > >> > > > >> > > Even with this code in place the resulting stream in the avi is reported > >> > > as 100 fps. > >> > > >> > that seems to be a regression since > >> > 6f69f7a8bf6a0d013985578df2ef42ee6b1c7994 > >> > > >> > IIRC the intended timebase is 1/50 for this kind of content > >> > (allowing the support of interlaced and field duplicated content to > >> > appear later) > >> > > >> > > >> > > And with or without the code, the resulting files play the > >> > > same with the players i tried. > >> > > >> > Higher framerates / finer timebases need noticably more space to > >> > be stored in avi, thats not the case for other formats and thats > >> > one reason why avi is treated as a special case. > >> > > >> > ill try to look tomorrow why its 100fps since the previous > >> > codecpar patches. Though 100fps is not nearly as bad as 600fps > >> > 600 has ~6 times the overhead > >> > >> This regression is caused by ticks_per_frame beiing incorrect > >> > >> Ill send a patch to fix this > > > > patch attached > > > > We don't have time_base in codecpar, so why do we need ticks per frame in it? We need both in some form probably For this regression ticks_per_frame ATM is enough. For time_base theres code to copy/access it bypassing codecpar The way it seems to be currently is that there are fields which are needed and either they are in codecpar or theres some tricks to access them from code sheduled to be removed (which will work only as long as the code isnt removed) or things just dont work. > > Which time_base does it modify the interpretation of? The field should > be bundled with that, then. the AVCodecContext one, ticks_per_frame is already in AVCodecContext the AVCodecContext ticks_per_frame though is not exported after codecpar while the codec timebase still is just not vissibly through codecpar [...]
On Tue, Sep 06, 2016 at 11:32:24AM -0300, James Almer wrote: > On 9/6/2016 9:57 AM, Clément Bœsch wrote: > >> We don't have time_base in codecpar, so why do we need ticks per frame in it? > >> > > >> > Which time_base does it modify the interpretation of? The field should > >> > be bundled with that, then. > > When do we have a mismatch of st->time_base and that "codec time base"? > > st->time_base can be anything. Matroska for example is always 1/1000, > and mpegps 1/90000. > "Codec time_base" seems to be codec frame_rate * ticks_per_frame, so > usually the same as frame_rate, except for codecs like h264 and mpeg2 > where it's twice that. this only works when codecs use the frame or field rate as basis for their timebase With some codecs that is syntactically not even possible MPEG4 (ISO/IEC 14496-2) stores its timebase as 1/N not M/N for vfr with for example 30000/1001 that would be different (1/30000) than the frame rate (30000/1001) (which may be variable) [...]
On Tue, Sep 6, 2016 at 5:10 PM, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Tue, Sep 06, 2016 at 02:39:11PM +0200, Hendrik Leppkes wrote: >> On Tue, Sep 6, 2016 at 2:21 PM, Michael Niedermayer >> <michael@niedermayer.cc> wrote: >> > On Tue, Sep 06, 2016 at 02:18:35PM +0200, Michael Niedermayer wrote: >> >> On Tue, Sep 06, 2016 at 04:57:06AM +0200, Michael Niedermayer wrote: >> >> > On Mon, Sep 05, 2016 at 10:04:35PM -0300, James Almer wrote: >> >> > > On 9/5/2016 12:41 PM, Michael Niedermayer wrote: >> >> > > > On Mon, Sep 05, 2016 at 04:41:52PM +0200, Clément Bœsch wrote: >> >> > > >> From: Clément Bœsch <clement@stupeflix.com> >> >> > > >> >> >> > > >> These adjusted codec fields do not seem to be in use anymore and prevent >> >> > > >> the convert of ffmpeg*.c to codecpar. >> >> > > > >> >> > > > ./ffmpeg -i ~/tickets/4914/xdcam8mp2-1s_small.ts -c:v copy out.mxf >> >> > > > fails, no output anymore >> >> > > > >> >> > > > ./ffmpeg -i matrixbench_mpeg2.mpg -c:v copy -t 1 test.avi >> >> > > > the output now has 600fps >> >> > > >> >> > > Even with this code in place the resulting stream in the avi is reported >> >> > > as 100 fps. >> >> > >> >> > that seems to be a regression since >> >> > 6f69f7a8bf6a0d013985578df2ef42ee6b1c7994 >> >> > >> >> > IIRC the intended timebase is 1/50 for this kind of content >> >> > (allowing the support of interlaced and field duplicated content to >> >> > appear later) >> >> > >> >> > >> >> > > And with or without the code, the resulting files play the >> >> > > same with the players i tried. >> >> > >> >> > Higher framerates / finer timebases need noticably more space to >> >> > be stored in avi, thats not the case for other formats and thats >> >> > one reason why avi is treated as a special case. >> >> > >> >> > ill try to look tomorrow why its 100fps since the previous >> >> > codecpar patches. Though 100fps is not nearly as bad as 600fps >> >> > 600 has ~6 times the overhead >> >> >> >> This regression is caused by ticks_per_frame beiing incorrect >> >> >> >> Ill send a patch to fix this >> > >> > patch attached >> > >> >> We don't have time_base in codecpar, so why do we need ticks per frame in it? > > We need both in some form probably > For this regression ticks_per_frame ATM is enough. > For time_base theres code to copy/access it bypassing codecpar > > The way it seems to be currently is that there are fields which > are needed and either they are > in codecpar or > theres some tricks to access them from code sheduled to be removed > (which will work only as long as the code isnt removed) > or things just dont work. > > >> >> Which time_base does it modify the interpretation of? The field should >> be bundled with that, then. > > the AVCodecContext one, ticks_per_frame is already in AVCodecContext > the AVCodecContext ticks_per_frame though is not exported after codecpar > while the codec timebase still is just not vissibly through codecpar > Maybe that part should be fixed then, wherever we export that to AVCodecContext, also set its ticks_per_frame properly? It just feels bad to export a property here that standing alone doesn't mean anything. So fix the export of ticks_per_frame for AVCodecContext, and if later on we decide we really do need time_base in AVCodecParameters, and can't fix whatever needs it differently, we can then include both in there. - Hendrik
From 66b4f6da72ca7867b9d1c6c7936161cf1482a73b Mon Sep 17 00:00:00 2001 From: Michael Niedermayer <michael@niedermayer.cc> Date: Tue, 6 Sep 2016 13:39:36 +0200 Subject: [PATCH] avcodec: Add ticks_per_frame to AVCodecParameters Fixes regressions in stream copy timebase handling (for example with AVI output being incorrectly 100fps) Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/avcodec.h | 8 ++++++++ libavcodec/utils.c | 3 +++ tests/ref/fate/concat-demuxer-extended-lavf-mxf | 2 +- tests/ref/fate/concat-demuxer-extended-lavf-mxf_d10 | 2 +- tests/ref/fate/concat-demuxer-simple1-lavf-mxf | 2 +- tests/ref/fate/concat-demuxer-simple1-lavf-mxf_d10 | 2 +- tests/ref/fate/concat-demuxer-simple2-lavf-ts | 2 +- 7 files changed, 16 insertions(+), 5 deletions(-) diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h index e2dad5d..3ec8814 100644 --- a/libavcodec/avcodec.h +++ b/libavcodec/avcodec.h @@ -4091,6 +4091,14 @@ typedef struct AVCodecParameters { * Audio only. Number of samples to skip after a discontinuity. */ int seek_preroll; + /** + * For some codecs, the time base is closer to the field rate than the frame rate. + * Most notably, H.264 and MPEG-2 specify time_base as half of frame duration + * if no telecine is used ... + * + * Set to time_base ticks per frame. Default 1, e.g., H.264/MPEG-2 set it to 2. + */ + int ticks_per_frame; } AVCodecParameters; /** diff --git a/libavcodec/utils.c b/libavcodec/utils.c index 0f6d0e7..da24f92 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -4043,6 +4043,7 @@ static void codec_parameters_reset(AVCodecParameters *par) par->sample_aspect_ratio = (AVRational){ 0, 1 }; par->profile = FF_PROFILE_UNKNOWN; par->level = FF_LEVEL_UNKNOWN; + par->ticks_per_frame = 1; } AVCodecParameters *avcodec_parameters_alloc(void) @@ -4098,6 +4099,7 @@ int avcodec_parameters_from_context(AVCodecParameters *par, par->bits_per_raw_sample = codec->bits_per_raw_sample; par->profile = codec->profile; par->level = codec->level; + par->ticks_per_frame = codec->ticks_per_frame; switch (par->codec_type) { case AVMEDIA_TYPE_VIDEO: @@ -4153,6 +4155,7 @@ int avcodec_parameters_to_context(AVCodecContext *codec, codec->bits_per_raw_sample = par->bits_per_raw_sample; codec->profile = par->profile; codec->level = par->level; + codec->ticks_per_frame = par->ticks_per_frame; switch (par->codec_type) { case AVMEDIA_TYPE_VIDEO: diff --git a/tests/ref/fate/concat-demuxer-extended-lavf-mxf b/tests/ref/fate/concat-demuxer-extended-lavf-mxf index b894938..677a354 100644 --- a/tests/ref/fate/concat-demuxer-extended-lavf-mxf +++ b/tests/ref/fate/concat-demuxer-extended-lavf-mxf @@ -1 +1 @@ -0aa1ca6ff6e2e5aa926454d22fdaecd5 *tests/data/fate/concat-demuxer-extended-lavf-mxf.ffprobe +7038d2a9b336799a9173463ab91a152f *tests/data/fate/concat-demuxer-extended-lavf-mxf.ffprobe diff --git a/tests/ref/fate/concat-demuxer-extended-lavf-mxf_d10 b/tests/ref/fate/concat-demuxer-extended-lavf-mxf_d10 index b378a2d..5ea0ce2 100644 --- a/tests/ref/fate/concat-demuxer-extended-lavf-mxf_d10 +++ b/tests/ref/fate/concat-demuxer-extended-lavf-mxf_d10 @@ -1 +1 @@ -14c2b8d8f82f261c9627b33c481c0e8c *tests/data/fate/concat-demuxer-extended-lavf-mxf_d10.ffprobe +999e165a53d2d15f5f5b8f1ee649320a *tests/data/fate/concat-demuxer-extended-lavf-mxf_d10.ffprobe diff --git a/tests/ref/fate/concat-demuxer-simple1-lavf-mxf b/tests/ref/fate/concat-demuxer-simple1-lavf-mxf index 3fc7957..f173b10 100644 --- a/tests/ref/fate/concat-demuxer-simple1-lavf-mxf +++ b/tests/ref/fate/concat-demuxer-simple1-lavf-mxf @@ -120,5 +120,5 @@ audio|1|65280|1.360000|65280|1.360000|1920|0.040000|N/A|N/A|3840|206848|K|1 Strings Metadata|8 video|0|37|1.480000|34|1.360000|1|0.040000|N/A|N/A|24786|211456|K|1 Strings Metadata|8 -0|mpeg2video|4|video|1/25|[0][0][0][0]|0x0000|352|288|0|0|1|1:1|11:9|yuv420p|8|tv|unknown|unknown|unknown|left|N/A|1|N/A|25/1|25/1|1/25|N/A|N/A|N/A|N/A|N/A|N/A|N/A|N/A|N/A|51|0|0|0|0|0|0|0|0|0|0|0|0x060A2B340101010501010D001300000000000000000000000000000000000001 +0|mpeg2video|4|video|1/50|[0][0][0][0]|0x0000|352|288|0|0|1|1:1|11:9|yuv420p|8|tv|unknown|unknown|unknown|left|N/A|1|N/A|25/1|25/1|1/25|N/A|N/A|N/A|N/A|N/A|N/A|N/A|N/A|N/A|51|0|0|0|0|0|0|0|0|0|0|0|0x060A2B340101010501010D001300000000000000000000000000000000000001 1|pcm_s16le|unknown|audio|1/48000|[0][0][0][0]|0x0000|s16|48000|1|unknown|16|N/A|0/0|0/0|1/48000|0|0.000000|N/A|N/A|768000|N/A|N/A|N/A|N/A|50|0|0|0|0|0|0|0|0|0|0|0|0x060A2B340101010501010D001300000000000000000000000000000000000001 diff --git a/tests/ref/fate/concat-demuxer-simple1-lavf-mxf_d10 b/tests/ref/fate/concat-demuxer-simple1-lavf-mxf_d10 index 0ff1b04..f0a3461 100644 --- a/tests/ref/fate/concat-demuxer-simple1-lavf-mxf_d10 +++ b/tests/ref/fate/concat-demuxer-simple1-lavf-mxf_d10 @@ -78,5 +78,5 @@ video|0|34|1.360000|34|1.360000|1|0.040000|N/A|N/A|150000|1923072|K|1 Strings Metadata|8 audio|1|65280|1.360000|65280|1.360000|1920|0.040000|N/A|N/A|7680|2073600|K|1 Strings Metadata|8 -0|mpeg2video|0|video|1/25|[0][0][0][0]|0x0000|720|608|0|0|0|1:1|45:38|yuv422p|5|tv|unknown|unknown|unknown|topleft|N/A|1|N/A|25/1|25/1|1/25|0|0.000000|N/A|N/A|30000000|N/A|N/A|N/A|N/A|35|0|0|0|0|0|0|0|0|0|0|0|0x060A2B340101010501010D001300000000000000000000000000000000000001 +0|mpeg2video|0|video|1/50|[0][0][0][0]|0x0000|720|608|0|0|0|1:1|45:38|yuv422p|5|tv|unknown|unknown|unknown|topleft|N/A|1|N/A|25/1|25/1|1/25|0|0.000000|N/A|N/A|30000000|N/A|N/A|N/A|N/A|35|0|0|0|0|0|0|0|0|0|0|0|0x060A2B340101010501010D001300000000000000000000000000000000000001 1|pcm_s16le|unknown|audio|1/48000|[0][0][0][0]|0x0000|s16|48000|2|unknown|16|N/A|0/0|0/0|1/48000|0|0.000000|N/A|N/A|1536000|N/A|N/A|N/A|N/A|35|0|0|0|0|0|0|0|0|0|0|0|0x060A2B340101010501010D001300000000000000000000000000000000000001 diff --git a/tests/ref/fate/concat-demuxer-simple2-lavf-ts b/tests/ref/fate/concat-demuxer-simple2-lavf-ts index 33337cf..ea614bf 100644 --- a/tests/ref/fate/concat-demuxer-simple2-lavf-ts +++ b/tests/ref/fate/concat-demuxer-simple2-lavf-ts @@ -148,4 +148,4 @@ video|1|168382|1.870911|164782|1.830911|3600|0.040000|N/A|N/A|24800|189692|K video|1|171982|1.910911|168382|1.870911|3600|0.040000|N/A|N/A|17454|216388|_ video|1|175582|1.950911|171982|1.910911|3600|0.040000|N/A|N/A|15033|235000|_ 0|mp2|unknown|audio|1/44100|[3][0][0][0]|0x0003|s16p|44100|1|mono|0|N/A|0/0|0/0|1/90000|0|0.000000|N/A|N/A|64000|N/A|N/A|N/A|N/A|89|0|0|0|0|0|0|0|0|0|0|0 -1|mpeg2video|4|video|1/25|[2][0][0][0]|0x0002|352|288|0|0|1|1:1|11:9|yuv420p|8|tv|unknown|unknown|unknown|left|N/A|1|N/A|25/1|25/1|1/90000|N/A|N/A|N/A|N/A|N/A|N/A|N/A|N/A|N/A|60|0|0|0|0|0|0|0|0|0|0|0 +1|mpeg2video|4|video|1/50|[2][0][0][0]|0x0002|352|288|0|0|1|1:1|11:9|yuv420p|8|tv|unknown|unknown|unknown|left|N/A|1|N/A|25/1|25/1|1/90000|N/A|N/A|N/A|N/A|N/A|N/A|N/A|N/A|N/A|60|0|0|0|0|0|0|0|0|0|0|0 -- 2.9.3