Message ID | 20240627053746.189095-1-marth64@proxyid.net |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel] avformat/dvdvideodec: Simplify padding cell trim logic | 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 |
Please, ignore this particular patch. I have found a formatting mistake (opportunity to remove braces). I will send a v2. I apologize for the inconvenience. On Thu, Jun 27, 2024 at 12:38 AM Marth64 <marth64@proxyid.net> 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 when we > attempt to lock on to the PGC navigation event stream, > but this is a wrong assumption because we have already > entered the PGC. This causes false positives 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 MPEG-PS block > reading event handling, in turn also fixing the trim > function for these weirdly authored discs. > > Signed-off-by: Marth64 <marth64@proxyid.net> > --- > libavformat/dvdvideodec.c | 20 +++++++++----------- > 1 file changed, 9 insertions(+), 11 deletions(-) > > diff --git a/libavformat/dvdvideodec.c b/libavformat/dvdvideodec.c > index e7132725b7..b0fb5f0de2 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; > } > @@ -785,8 +778,13 @@ static int dvdvideo_play_next_ps_block(AVFormatContext *s, DVDVideoPlaybackState > return nav_len; > case DVDNAV_BLOCK_OK: > if (!state->in_ps) { > - if (state->in_pgc) > - i = 0; /* necessary in case we are skipping junk cells at the beginning */ > + continue; > + } > + > + if (c->opt_trim && !dvdvideo_is_cell_promising(s, state->pgc, state->celln)) { > + av_log(s, AV_LOG_DEBUG, "skipping packet in padding cell #%d\n", state->celln); > + > + i = 0; > continue; > } > > -- > 2.34.1 >
diff --git a/libavformat/dvdvideodec.c b/libavformat/dvdvideodec.c index e7132725b7..b0fb5f0de2 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; } @@ -785,8 +778,13 @@ static int dvdvideo_play_next_ps_block(AVFormatContext *s, DVDVideoPlaybackState return nav_len; case DVDNAV_BLOCK_OK: if (!state->in_ps) { - if (state->in_pgc) - i = 0; /* necessary in case we are skipping junk cells at the beginning */ + continue; + } + + if (c->opt_trim && !dvdvideo_is_cell_promising(s, state->pgc, state->celln)) { + av_log(s, AV_LOG_DEBUG, "skipping packet in padding cell #%d\n", state->celln); + + i = 0; continue; }
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 when we attempt to lock on to the PGC navigation event stream, but this is a wrong assumption because we have already entered the PGC. This causes false positives 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 MPEG-PS block reading event handling, in turn also fixing the trim function for these weirdly authored discs. Signed-off-by: Marth64 <marth64@proxyid.net> --- libavformat/dvdvideodec.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-)