Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
 help / color / mirror / Atom feed
* [FFmpeg-devel] [PATCH v4] avformat/dvdvideodec: Fix incorrect padding cell trim logic
@ 2024-07-02  6:41 Marth64
  2024-07-06 11:18 ` Stefano Sabatini
  0 siblings, 1 reply; 6+ messages in thread
From: Marth64 @ 2024-07-02  6:41 UTC (permalink / raw)
  To: ffmpeg-devel; +Cc: Marth64

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@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);
+
+                        i = 0;
+                        continue;
+                    }
+
                     av_log(s, AV_LOG_DEBUG, "navigation: locked to program stream\n");
 
                     state->in_ps = 1;
-- 
2.34.1

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [FFmpeg-devel] [PATCH v4] avformat/dvdvideodec: Fix incorrect padding cell trim logic
  2024-07-02  6:41 [FFmpeg-devel] [PATCH v4] avformat/dvdvideodec: Fix incorrect padding cell trim logic Marth64
@ 2024-07-06 11:18 ` Stefano Sabatini
  2024-07-06 17:53   ` Marth64
  0 siblings, 1 reply; 6+ messages in thread
From: Stefano Sabatini @ 2024-07-06 11:18 UTC (permalink / raw)
  To: FFmpeg development discussions and patches; +Cc: Marth64

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@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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [FFmpeg-devel] [PATCH v4] avformat/dvdvideodec: Fix incorrect padding cell trim logic
  2024-07-06 11:18 ` Stefano Sabatini
@ 2024-07-06 17:53   ` Marth64
  2024-07-12  3:30     ` Marth64
  0 siblings, 1 reply; 6+ messages in thread
From: Marth64 @ 2024-07-06 17:53 UTC (permalink / raw)
  To: FFmpeg development discussions and patches, Marth64

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

Thanks Stefano for the review. A further cleanup patch set is coming
shortly with a patch specifically on language in log messages, will include
this.

Cheers,
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [FFmpeg-devel] [PATCH v4] avformat/dvdvideodec: Fix incorrect padding cell trim logic
  2024-07-06 17:53   ` Marth64
@ 2024-07-12  3:30     ` Marth64
  2024-07-13 15:37       ` Stefano Sabatini
  0 siblings, 1 reply; 6+ messages in thread
From: Marth64 @ 2024-07-12  3:30 UTC (permalink / raw)
  To: Marth64; +Cc: FFmpeg development discussions and patches

Ping to apply on this set of 3, so I can send seeking and additional
cleanup set (as a collection). Thank you!
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [FFmpeg-devel] [PATCH v4] avformat/dvdvideodec: Fix incorrect padding cell trim logic
  2024-07-12  3:30     ` Marth64
@ 2024-07-13 15:37       ` Stefano Sabatini
  2024-07-13 15:52         ` Marth64
  0 siblings, 1 reply; 6+ messages in thread
From: Stefano Sabatini @ 2024-07-13 15:37 UTC (permalink / raw)
  To: FFmpeg development discussions and patches; +Cc: Marth64

On Fri, Jul 12, 2024 at 5:31 AM Marth64 <marth64@proxyid.net> wrote:
>
> Ping to apply on this set of 3, so I can send seeking and additional
> cleanup set (as a collection). Thank you!

Applied patches:
eb07a59 avformat/dvdvideodec: Don't add chapter markers for empty/dummy PTTs
f37f86a avformat/dvdvideodec: Remove redundant ret initializations
f1abb75 avformat/dvdvideodec: Fix incorrect padding cell trim logic

Thanks.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [FFmpeg-devel] [PATCH v4] avformat/dvdvideodec: Fix incorrect padding cell trim logic
  2024-07-13 15:37       ` Stefano Sabatini
@ 2024-07-13 15:52         ` Marth64
  0 siblings, 0 replies; 6+ messages in thread
From: Marth64 @ 2024-07-13 15:52 UTC (permalink / raw)
  To: Stefano Sabatini; +Cc: Marth64, FFmpeg development discussions and patches

Thank you!
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2024-07-13 15:52 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-07-02  6:41 [FFmpeg-devel] [PATCH v4] avformat/dvdvideodec: Fix incorrect padding cell trim logic Marth64
2024-07-06 11:18 ` Stefano Sabatini
2024-07-06 17:53   ` Marth64
2024-07-12  3:30     ` Marth64
2024-07-13 15:37       ` Stefano Sabatini
2024-07-13 15:52         ` Marth64

Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
		ffmpegdev@gitmailbox.com
	public-inbox-index ffmpegdev

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git