Quoting Marth64 (2024-07-28 09:34:39)
> DVDs naturally consist of segmented MPEG-PS blobs within a VOB
> (i.e. VOBs are not linear). NAV packs set the segment boundaries.
> When switching between segments, discontinuities occur and thus
> the subdemuxer needs to be reset. The current approach to manage
> this is by invoking ff_read_frame_flush() on the subdemuxer context,
> via a callback function which is invoked during the menu or dvdnav
> block functions. The same subdemuxer context is used throughout
> the demux, with a stretched PTS wrap bits value (64) + disabled
> overflow correction, and then flushed on each segment. Eventually,
> a play_end context variable is set to declare EOF.
>
> However, this approach is wrong and racy. The block read flushes the
> demuxer before the frame read is complete, causing frames to drop
> on discontinuity. The play_end signal likewise ends playback before
> the frame read is complete, causing frames to drop at end of the title.
> To compound the issue, the PTS wrap bits value of 64 is wrong;
> the VOBU limit is actually 32 and the overflow correction should work.
>
> Instead, EOF the MPEG-PS subdemuxer organically when each VOB segment
> ends, and re-open it if needed with the offset after the full frame read
> is complete. In doing so, correct the PTS wrap behavior to 32 bits,
> remove the racy play_end/segment_started signals and the callback pattern.
> The behavior is now more similar to the HLS/DASH demuxers.
>
> This commit fixes five intertwined issues, yielding an accurate demux:
> (1) Racy segment switching
> (2) Racy EOF signaling
> (3) Off-by-one leading to missed packets at start of menus
> (4) Incorrect PTS wrap behavior
> (5) Unnecessary frame discard workarounds removed
>
> Signed-off-by: Marth64 <mart...@proxyid.net>
> ---
> libavformat/dvdvideodec.c | 198 +++++++++++++++++++-------------------
> 1 file changed, 100 insertions(+), 98 deletions(-)
I wouldn't call it a 'race', since that implies concurrency, while all
this should be strictly single-threaded. IIUC the problem is more about
various demuxer pieces getting desynchronized.
Besides that, the commit message looks generally sensible, but the diff
itself seems like it mixes a bit too many changes that could be split
into separate patches to make it more reviewable. E.g. renaming
ts_offset to ptm_offset, moving some error messages around, renaming
found_stream to st_matched, changing p_nav_event to p_is_nav_packet,
etc. - are all cosmetics that obscure the actual functional changes.
> @@ -1469,7 +1466,9 @@ static void dvdvideo_subdemux_close(AVFormatContext *s)
> DVDVideoDemuxContext *c = s->priv_data;
>
> av_freep(&c->mpeg_pb.pub.buffer);
> + memset(&c->mpeg_pb, 0x00, sizeof(c->mpeg_pb));
Why this change?
> avformat_close_input(&c->mpeg_ctx);
> + c->mpeg_ctx = NULL;
Does this do anything? The pointer should be NULLed by
avformat_close_input(), that's why it takes a double pointer.
> }
>
> static int dvdvideo_subdemux_open(AVFormatContext *s)
> @@ -1501,12 +1500,23 @@ static int dvdvideo_subdemux_open(AVFormatContext *s)
> c->mpeg_ctx->max_analyze_duration = 0;
> c->mpeg_ctx->interrupt_callback = s->interrupt_callback;
> c->mpeg_ctx->pb = &c->mpeg_pb.pub;
> - c->mpeg_ctx->correct_ts_overflow = 0;
> - c->mpeg_ctx->io_open = NULL;
Why?
> @@ -1604,72 +1614,64 @@ static int dvdvideo_read_packet(AVFormatContext *s,
> AVPacket *pkt)
> DVDVideoDemuxContext *c = s->priv_data;
>
> int ret;
> - enum AVMediaType st_type;
> - int found_stream = 0;
> -
> - if (c->play_end)
> - return AVERROR_EOF;
> + int st_matched = 0;
> + AVStream *st_subdemux;
>
> ret = av_read_frame(c->mpeg_ctx, pkt);
> + if (ret < 0) {
> + if (c->subdemux_reset) {
> + c->subdemux_reset = 0;
> + c->pts_offset = c->play_state.ptm_offset;
>
> - if (ret < 0)
> - return ret;
> + if ((ret = dvdvideo_subdemux_reset(s)) < 0)
> + return ret;
> +
> + return FFERROR_REDO;
> + }
>
> - if (!c->segment_started)
> - c->segment_started = 1;
> + return ret;
> + }
>
> - st_type = c->mpeg_ctx->streams[pkt->stream_index]->codecpar->codec_type;
> + st_subdemux = c->mpeg_ctx->streams[pkt->stream_index];
>
> /* map the subdemuxer stream to the parent demuxer's stream (by
> startcode) */
> for (int i = 0; i < s->nb_streams; i++) {
> - if (s->streams[i]->id ==
> c->mpeg_ctx->streams[pkt->stream_index]->id) {
> + if (s->streams[i]->id == st_subdemux->id) {
> pkt->stream_index = s->streams[i]->index;
> - found_stream = 1;
> + st_matched = 1;
> break;
> }
> }
>
> - if (!found_stream) {
> - av_log(s, AV_LOG_DEBUG, "discarding frame with stream that was not
> in IFO headers "
> - "(stream id=%d)\n",
> c->mpeg_ctx->streams[pkt->stream_index]->id);
> -
> - return FFERROR_REDO;
> - }
> + if (!st_matched)
> + goto discard;
>
> if (pkt->pts != AV_NOPTS_VALUE && pkt->dts != AV_NOPTS_VALUE) {
> if (!c->play_started) {
> - /* try to start at the beginning of a GOP */
> - if (st_type != AVMEDIA_TYPE_VIDEO || !(pkt->flags &
> AV_PKT_FLAG_KEY)) {
> - av_log(s, AV_LOG_VERBOSE, "Discarding packet which is not a
> video keyframe or "
> - "with unset PTS/DTS at start\n");
> - return FFERROR_REDO;
> - }
> -
> c->first_pts = pkt->pts;
> c->play_started = 1;
> }
>
> - pkt->pts += c->play_state.ts_offset - c->first_pts;
> - pkt->dts += c->play_state.ts_offset - c->first_pts;
> -
> - if (pkt->pts < 0) {
> - av_log(s, AV_LOG_VERBOSE, "Discarding packet with negative PTS
> (st=%d pts=%" PRId64 "), "
> - "this is OK at start of playback\n",
> - pkt->stream_index, pkt->pts);
> -
> - return FFERROR_REDO;
> - }
Any reason you're not dropping pts<0 packets anymore?
--
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
