[FFmpeg-devel] [PATCH v4] avformat/dvdvideodec: Fix incorrect padding cell trim logic

[Stefano Sabatini]

On date Tuesday 2024-07-02 01:41:31 -0500, Marth64 wrote:
> When -trim option is used (by default), padding cells
> at the beginning of the title are supposed to be ignored.
> The current implementation does the ignoring after we
> have locked on to the PGC navigation event stream,
> but does not set the PGC/PG state properly.
> 
> This causes false positives and errors on some discs
> due to a search for a program stream cell that
> never succeeds. User would have to know to disable
> the -trim option to work around the issue.
> 
> Simplify the logic and move it to the NAV packet
> event handling, in turn implementing the behaviour
> correctly and fixing the trim function for impacted discs.
> 
> Signed-off-by: Marth64 <marth64 at proxyid.net>
> ---
>  libavformat/dvdvideodec.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/libavformat/dvdvideodec.c b/libavformat/dvdvideodec.c
> index e7132725b7..c694e8d00f 100644
> --- a/libavformat/dvdvideodec.c
> +++ b/libavformat/dvdvideodec.c
> @@ -624,7 +624,6 @@ static int dvdvideo_play_next_ps_block(AVFormatContext *s, DVDVideoPlaybackState
>      dvdnav_vts_change_event_t *e_vts;
>      dvdnav_cell_change_event_t *e_cell;
>      int cur_title, cur_pgcn, cur_pgn, cur_angle, cur_title_unused, cur_ptt, cur_nb_angles;
> -    int is_cell_promising = 0;
>      pci_t *e_pci;
>      dsi_t *e_dsi;
>  
> @@ -706,23 +705,17 @@ static int dvdvideo_play_next_ps_block(AVFormatContext *s, DVDVideoPlaybackState
>                      continue;
>  
>                  e_cell = (dvdnav_cell_change_event_t *) nav_buf;
> -                is_cell_promising = !c->opt_trim || dvdvideo_is_cell_promising(s, state->pgc, e_cell->cellN);
>  
> -                av_log(s, AV_LOG_DEBUG, "new cell: prev=%d new=%d promising=%d\n",
> -                                        state->celln, e_cell->cellN, is_cell_promising);
> +                av_log(s, AV_LOG_DEBUG, "new cell: prev=%d new=%d\n", state->celln, e_cell->cellN);
>  
>                  if (!state->in_ps && !state->in_pgc) {
>                      if (cur_title == c->opt_title                        &&
>                          (c->opt_pgc || cur_ptt == c->opt_chapter_start)  &&
>                          cur_pgcn == state->pgcn                          &&
> -                        cur_pgn == state->entry_pgn                      &&
> -                        is_cell_promising) {
> +                        cur_pgn == state->entry_pgn) {
>  
>                          state->in_pgc = 1;
>                      }
> -
> -                    if (c->opt_trim && !is_cell_promising)
> -                        av_log(s, AV_LOG_INFO, "Skipping padding cell #%d\n", e_cell->cellN);
>                  } else if (state->celln >= e_cell->cellN || state->pgn > cur_pgn) {
>                      return AVERROR_EOF;
>                  }
> @@ -766,6 +759,13 @@ static int dvdvideo_play_next_ps_block(AVFormatContext *s, DVDVideoPlaybackState
>                         e_pci->pci_gi.nv_pck_lbn, state->vobu_duration, state->nav_pts);
>  

>                  if (!state->in_ps) {
> +                    if (c->opt_trim && !dvdvideo_is_cell_promising(s, state->pgc, state->celln)) {
> +                        av_log(s, AV_LOG_INFO, "Skipping padding cell #%d\n", state->celln);

nit++: possibly add some more information to aid debugging, such as
"Trimming enabled, skipping padding cell for non promising cell ..."

[...]

Looks good to me anyway, thanks.