| Message ID | 20240301133923.1132924-2-nicolas.gaullier@cji.paris |
|---|---|
| State | New |
| Headers | show |
| Series | avcodec/parser: fix fetch_timestamp in a scenario with unaligned packets | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On Fri, Mar 01, 2024 at 02:39:19PM +0100, Nicolas Gaullier wrote: > The mpegts demuxer splits packets according to its max_packet_size. > This currently fills the AVCodecParserContext s->cur_frame_* arrays with > kind of 'empty' entries: no pts/dts. > This patch merges these entries, so the parser behaviour is independent > from the demuxer settings. > This patch is required for the following patch which will fetch 'past' > timestamps from past cur_frames. > > Signed-off-by: Nicolas Gaullier <nicolas.gaullier@cji.paris> > --- > libavcodec/parser.c | 4 ++++ > 1 file changed, 4 insertions(+) Breaks fate-seek-lavf-as --- ./tests/ref/seek/lavf-asf 2024-02-28 23:42:14.743496132 +0100 +++ tests/data/fate/seek-lavf-asf 2024-03-03 23:06:08.850893410 +0100 @@ -1,53 +0,0 @@ -ret: 0 st: 1 flags:1 dts: 0.000000 pts: 0.000000 pos: 693 size: 208 -ret: 0 st:-1 flags:0 ts:-1.000000 -ret: 0 st: 1 flags:1 dts: 0.000000 pts: 0.000000 pos: 693 size: 208 -ret: 0 st:-1 flags:1 ts: 1.894167 -ret: 0 st: 1 flags:1 dts: 0.470000 pts: 0.470000 pos: 147893 size: 209 -ret: 0 st: 0 flags:0 ts: 0.788000 -ret: 0 st: 1 flags:1 dts: 0.470000 pts: 0.470000 pos: 147893 size: 209 -ret: 0 st: 0 flags:1 ts:-0.317000 -ret: 0 st: 1 flags:1 dts: 0.000000 pts: 0.000000 pos: 693 size: 208 -ret: 0 st: 1 flags:0 ts: 2.577000 -ret: 0 st: 1 flags:1 dts: 0.993000 pts: 0.993000 pos: 330293 size: 209 -ret: 0 st: 1 flags:1 ts: 1.471000 -ret: 0 st: 1 flags:1 dts: 0.993000 pts: 0.993000 pos: 330293 size: 209 -ret: 0 st:-1 flags:0 ts: 0.365002 -ret: 0 st: 1 flags:1 dts: 0.470000 pts: 0.470000 pos: 147893 size: 209 -ret: 0 st:-1 flags:1 ts:-0.740831 -ret: 0 st: 1 flags:1 dts: 0.000000 pts: 0.000000 pos: 693 size: 208 -ret: 0 st: 0 flags:0 ts: 2.153000 -ret: 0 st: 1 flags:1 dts: 0.941000 pts: 0.941000 pos: 301493 size: 209 -ret: 0 st: 0 flags:1 ts: 1.048000 -ret: 0 st: 1 flags:1 dts: 0.941000 pts: 0.941000 pos: 301493 size: 209 -ret: 0 st: 1 flags:0 ts:-0.058000 -ret: 0 st: 1 flags:1 dts: 0.000000 pts: 0.000000 pos: 693 size: 208 -ret: 0 st: 1 flags:1 ts: 2.836000 -ret: 0 st: 1 flags:1 dts: 0.993000 pts: 0.993000 pos: 330293 size: 209 -ret: 0 st:-1 flags:0 ts: 1.730004 -ret: 0 st: 1 flags:1 dts: 0.941000 pts: 0.941000 pos: 301493 size: 209 -ret: 0 st:-1 flags:1 ts: 0.624171 -ret: 0 st: 1 flags:1 dts: 0.000000 pts: 0.000000 pos: 693 size: 208 -ret: 0 st: 0 flags:0 ts:-0.482000 -ret: 0 st: 1 flags:1 dts: 0.000000 pts: 0.000000 pos: 693 size: 208 -ret: 0 st: 0 flags:1 ts: 2.413000 -ret: 0 st: 1 flags:1 dts: 0.941000 pts: 0.941000 pos: 301493 size: 209 -ret: 0 st: 1 flags:0 ts: 1.307000 -ret: 0 st: 1 flags:1 dts: 0.993000 pts: 0.993000 pos: 330293 size: 209 -ret: 0 st: 1 flags:1 ts: 0.201000 -ret: 0 st: 1 flags:1 dts: 0.000000 pts: 0.000000 pos: 693 size: 208 -ret: 0 st:-1 flags:0 ts:-0.904994 -ret: 0 st: 1 flags:1 dts: 0.000000 pts: 0.000000 pos: 693 size: 208 -ret: 0 st:-1 flags:1 ts: 1.989173 -ret: 0 st: 1 flags:1 dts: 0.941000 pts: 0.941000 pos: 301493 size: 209 -ret: 0 st: 0 flags:0 ts: 0.883000 -ret: 0 st: 1 flags:1 dts: 0.470000 pts: 0.470000 pos: 147893 size: 209 -ret: 0 st: 0 flags:1 ts:-0.222000 -ret: 0 st: 1 flags:1 dts: 0.000000 pts: 0.000000 pos: 693 size: 208 -ret: 0 st: 1 flags:0 ts: 2.672000 -ret: 0 st: 1 flags:1 dts: 0.993000 pts: 0.993000 pos: 330293 size: 209 -ret: 0 st: 1 flags:1 ts: 1.566000 -ret: 0 st: 1 flags:1 dts: 0.993000 pts: 0.993000 pos: 330293 size: 209 -ret: 0 st:-1 flags:0 ts: 0.460008 -ret: 0 st: 1 flags:1 dts: 0.470000 pts: 0.470000 pos: 147893 size: 209 -ret: 0 st:-1 flags:1 ts:-0.645825 -ret: 0 st: 1 flags:1 dts: 0.000000 pts: 0.000000 pos: 693 size: 208 Test seek-lavf-asf failed. Look at tests/data/fate/seek-lavf-asf.err for details. [asf @ 0x5623c91f8280] asf_read_pts failed [asf @ 0x5623c91f8280] asf_read_pts failed [asf @ 0x5623c91f8280] asf_read_pts failed [asf @ 0x5623c91f8280] asf_read_pts failed [asf @ 0x5623c91f8280] asf_read_pts failed [asf @ 0x5623c91f8280] asf_read_pts failed [asf @ 0x5623c91f8280] asf_read_pts failed [asf @ 0x5623c91f8280] asf_read_pts failed Assertion pkt->pos == asf_st->packet_pos failed at libavformat/asfdec_f.c:1478 Aborted (core dumped) threads=1 tests/Makefile:318: recipe for target 'fate-seek-lavf-asf' failed make: *** [fate-seek-lavf-asf] Error 134 [...]
>De : ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> De la part de Michael Niedermayer >Envoyé : dimanche 3 mars 2024 23:07 >À : FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> >Objet : Re: [FFmpeg-devel] [PATCH v2 1/5] avcodec/parser: merge packets from the same frame > >On Fri, Mar 01, 2024 at 02:39:19PM +0100, Nicolas Gaullier wrote: >> The mpegts demuxer splits packets according to its max_packet_size. >> This currently fills the AVCodecParserContext s->cur_frame_* arrays >> with kind of 'empty' entries: no pts/dts. >> This patch merges these entries, so the parser behaviour is >> independent from the demuxer settings. >> This patch is required for the following patch which will fetch 'past' >> timestamps from past cur_frames. >> >> Signed-off-by: Nicolas Gaullier <nicolas.gaullier@cji.paris> >> --- >> libavcodec/parser.c | 4 ++++ >> 1 file changed, 4 insertions(+) > >Breaks fate-seek-lavf-as Really sorry about that, I was not aware that assert-level=2 had to be enabled to get full fate tests. Maybe this should be documented in the fate.texi. I also checked the green light on patchwork... It seems at least assert-level=1 is not supposed to consume too much cpu, so maybe it could be enabled for patchwork? Thank you very much for taking time for this review. Nicolas
Hi On Mon, Mar 04, 2024 at 05:38:48PM +0000, Nicolas Gaullier wrote: > >De : ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> De la part de Michael Niedermayer > >Envoyé : dimanche 3 mars 2024 23:07 > >À : FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > >Objet : Re: [FFmpeg-devel] [PATCH v2 1/5] avcodec/parser: merge packets from the same frame > > > >On Fri, Mar 01, 2024 at 02:39:19PM +0100, Nicolas Gaullier wrote: > >> The mpegts demuxer splits packets according to its max_packet_size. > >> This currently fills the AVCodecParserContext s->cur_frame_* arrays > >> with kind of 'empty' entries: no pts/dts. > >> This patch merges these entries, so the parser behaviour is > >> independent from the demuxer settings. > >> This patch is required for the following patch which will fetch 'past' > >> timestamps from past cur_frames. > >> > >> Signed-off-by: Nicolas Gaullier <nicolas.gaullier@cji.paris> > >> --- > >> libavcodec/parser.c | 4 ++++ > >> 1 file changed, 4 insertions(+) > > > >Breaks fate-seek-lavf-as > > Really sorry about that, I was not aware that assert-level=2 had to be enabled to get full fate tests. > Maybe this should be documented in the fate.texi. patch welcome > I also checked the green light on patchwork... It seems at least assert-level=1 is not supposed to consume too much cpu, so maybe it could be enabled for patchwork? CCing Andriy thx [...]
diff --git a/libavcodec/parser.c b/libavcodec/parser.c index efc28b8918..249f81d4bb 100644 --- a/libavcodec/parser.c +++ b/libavcodec/parser.c @@ -142,6 +142,7 @@ int av_parser_parse2(AVCodecParserContext *s, AVCodecContext *avctx, memset(dummy_buf, 0, sizeof(dummy_buf)); buf = dummy_buf; } else if (s->cur_offset + buf_size != s->cur_frame_end[s->cur_frame_start_index]) { /* skip remainder packets */ + if (pos != s->cur_frame_pos[s->cur_frame_start_index] || pos <= 0 || pts != AV_NOPTS_VALUE ) { /* add a new packet descriptor */ i = (s->cur_frame_start_index + 1) & (AV_PARSER_PTS_NB - 1); s->cur_frame_start_index = i; @@ -150,6 +151,9 @@ int av_parser_parse2(AVCodecParserContext *s, AVCodecContext *avctx, s->cur_frame_pts[i] = pts; s->cur_frame_dts[i] = dts; s->cur_frame_pos[i] = pos; + } else { + s->cur_frame_end[s->cur_frame_start_index] = s->cur_offset + buf_size; + } } if (s->fetch_timestamp) {
The mpegts demuxer splits packets according to its max_packet_size. This currently fills the AVCodecParserContext s->cur_frame_* arrays with kind of 'empty' entries: no pts/dts. This patch merges these entries, so the parser behaviour is independent from the demuxer settings. This patch is required for the following patch which will fetch 'past' timestamps from past cur_frames. Signed-off-by: Nicolas Gaullier <nicolas.gaullier@cji.paris> --- libavcodec/parser.c | 4 ++++ 1 file changed, 4 insertions(+)