| Message ID | 20240822165533.476916-1-nicolas.gaullier@cji.paris |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avformat/mxfenc: Fix guess frame_rate | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| yinshiyou/make_loongarch64 | warning | New warnings during build |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_x86 | warning | New warnings during build |
Quoting Nicolas Gaullier (2024-08-22 18:55:32) > The input time_base was a bad guess. > > Currently, fate-time_base test data assumed that overriding the input > time_base would affect the frame_rate, but this behaviour is not > documented, so just fix the fate data now that this is fixed. > > Fix regression since 10185e2d4c1e9839bc58a1d6a63c861677b13fd0: > previously, when streamcopying, the time_base was guessed from the > frame_rate considering it is often constant, so guessing the frame_rate > back from the time_base was often not a problem. > > To reproduce: > ffmpeg -i fate-suite/mpeg2/dvd_still_frame.vob -an -c copy out.mxf > > Signed-off-by: Nicolas Gaullier <nicolas.gaullier@cji.paris> > --- > libavformat/mxfenc.c | 9 +++++++-- > tests/ref/fate/time_base | 2 +- > 2 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c > index 4ac6a2d715..a814f15609 100644 > --- a/libavformat/mxfenc.c > +++ b/libavformat/mxfenc.c > @@ -2894,8 +2894,13 @@ static int mxf_init(AVFormatContext *s) > > if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO) { > const AVPixFmtDescriptor *pix_desc = av_pix_fmt_desc_get(st->codecpar->format); > - // TODO: should be avg_frame_rate > - AVRational tbc = st->time_base; > + AVRational frame_rate = (AVRational){ 0, 1 }; > + if (st->avg_frame_rate.num > 0 && st->avg_frame_rate.den > 0) > + frame_rate = st->avg_frame_rate; > + else if (st->r_frame_rate.num > 0 && st->r_frame_rate.den > 0) > + frame_rate = st->r_frame_rate; > + AVRational tbc = av_inv_q(frame_rate); Should it not still fall back to time_base when neither avg_frame_rate nor r_frame_rate is set?
sön 2024-09-01 klockan 12:53 +0200 skrev Anton Khirnov: > Quoting Nicolas Gaullier (2024-08-22 18:55:32) > > The input time_base was a bad guess. > > > > Currently, fate-time_base test data assumed that overriding the > > input > > time_base would affect the frame_rate, but this behaviour is not > > documented, so just fix the fate data now that this is fixed. > > > > Fix regression since 10185e2d4c1e9839bc58a1d6a63c861677b13fd0: > > previously, when streamcopying, the time_base was guessed from the > > frame_rate considering it is often constant, so guessing the > > frame_rate > > back from the time_base was often not a problem. > > > > To reproduce: > > ffmpeg -i fate-suite/mpeg2/dvd_still_frame.vob -an -c copy out.mxf > > > > Signed-off-by: Nicolas Gaullier <nicolas.gaullier@cji.paris> > > --- > > libavformat/mxfenc.c | 9 +++++++-- > > tests/ref/fate/time_base | 2 +- > > 2 files changed, 8 insertions(+), 3 deletions(-) > > > > diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c > > index 4ac6a2d715..a814f15609 100644 > > --- a/libavformat/mxfenc.c > > +++ b/libavformat/mxfenc.c > > @@ -2894,8 +2894,13 @@ static int mxf_init(AVFormatContext *s) > > > > if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO) { > > const AVPixFmtDescriptor *pix_desc = > > av_pix_fmt_desc_get(st->codecpar->format); > > - // TODO: should be avg_frame_rate > > - AVRational tbc = st->time_base; > > + AVRational frame_rate = (AVRational){ 0, 1 }; > > + if (st->avg_frame_rate.num > 0 && st- > > >avg_frame_rate.den > 0) > > + frame_rate = st->avg_frame_rate; > > + else if (st->r_frame_rate.num > 0 && st- > > >r_frame_rate.den > 0) > > + frame_rate = st->r_frame_rate; > > + AVRational tbc = av_inv_q(frame_rate); > > Should it not still fall back to time_base when neither > avg_frame_rate > nor r_frame_rate is set? Might be better to error out since time_base can be wildly off what is intended.. /Tomas
diff --git a/libavformat/mxfenc.c b/libavformat/mxfenc.c index 4ac6a2d715..a814f15609 100644 --- a/libavformat/mxfenc.c +++ b/libavformat/mxfenc.c @@ -2894,8 +2894,13 @@ static int mxf_init(AVFormatContext *s) if (st->codecpar->codec_type == AVMEDIA_TYPE_VIDEO) { const AVPixFmtDescriptor *pix_desc = av_pix_fmt_desc_get(st->codecpar->format); - // TODO: should be avg_frame_rate - AVRational tbc = st->time_base; + AVRational frame_rate = (AVRational){ 0, 1 }; + if (st->avg_frame_rate.num > 0 && st->avg_frame_rate.den > 0) + frame_rate = st->avg_frame_rate; + else if (st->r_frame_rate.num > 0 && st->r_frame_rate.den > 0) + frame_rate = st->r_frame_rate; + AVRational tbc = av_inv_q(frame_rate); + // Default component depth to 8 sc->component_depth = 8; sc->h_chroma_sub_sample = 2; diff --git a/tests/ref/fate/time_base b/tests/ref/fate/time_base index fd6cac53fc..23875d1fb8 100644 --- a/tests/ref/fate/time_base +++ b/tests/ref/fate/time_base @@ -1 +1 @@ -d408aba82d62a90ed7f46a1999b014f1 +b28d4ca13029fdc80a114b56467be9d7
The input time_base was a bad guess. Currently, fate-time_base test data assumed that overriding the input time_base would affect the frame_rate, but this behaviour is not documented, so just fix the fate data now that this is fixed. Fix regression since 10185e2d4c1e9839bc58a1d6a63c861677b13fd0: previously, when streamcopying, the time_base was guessed from the frame_rate considering it is often constant, so guessing the frame_rate back from the time_base was often not a problem. To reproduce: ffmpeg -i fate-suite/mpeg2/dvd_still_frame.vob -an -c copy out.mxf Signed-off-by: Nicolas Gaullier <nicolas.gaullier@cji.paris> --- libavformat/mxfenc.c | 9 +++++++-- tests/ref/fate/time_base | 2 +- 2 files changed, 8 insertions(+), 3 deletions(-)