[FFmpeg-devel] [PATCH 1/7] avformat/dvdvideodec: Fix racy PTS calculation and frame drops

[Anton Khirnov]

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 <marth64 at 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