Re: [PATCH] input: Improve timestamp and discontinuity handling in decoder and video output

Hi,

You should send your patch through a Merge Request at https://code.videolan.org/videolan/vlc/-/merge_requests
We don’t handle email patches anymore.

Thanks,
Steve

> On 21 Jul 2025, at 19:52, Quaylyn Rimer <[email protected]> wrote:
> 
> This patch addresses several immediate priority issues in VLC's timestamp
> and discontinuity handling:
> 
> * Enhanced VLC_TICK_INVALID validation in decoder:
>  - Add context-aware validation for video and audio timestamp handling
>  - Provide more informative logging to distinguish between expected
>    invalid timestamps (during initialization/format changes) and
>    problematic ones during active playback
>  - This resolves the FIXME comments in decoder.c lines 1316 and 1464
> 
> * Improved FIFO buffer management:
>  - Replace simple byte-count threshold with adaptive approach
>  - Consider both buffer size (400MB) and frame count (1000 frames)
>  - Add detailed logging showing both bytes and frame counts
>  - Better handles low-bitrate content with many small frames
> 
> * Enhanced CR/LF handling in stream parser:
>  - Fix boundary condition where CR/LF sequences span buffer boundaries
>  - Use peek-ahead to detect CR/LF pairs across boundaries
>  - Prevents creation of bogus empty lines
>  - Resolves the FIXME in stream.c line 306
> 
> * Better discontinuity handling in video output:
>  - Add discontinuity_flag to vout_thread_sys_t for state tracking
>  - Provide foundation for filters to detect timestamp discontinuities
>  - Enhanced documentation explaining the discontinuity handling approach
>  - Addresses the FIXME in video_output.c line 1088
> 
> These changes improve synchronization reliability and reduce artifacts
> during format changes, seeks, and other discontinuity events.
> 
> Signed-off-by: Quaylyn Rimer <[email protected]>
> ---
> src/input/decoder.c             | 60 +++++++++++++++++++++++++++------
> src/input/stream.c              | 36 +++++++++++++++++---
> src/video_output/video_output.c | 17 +++++++---
> 3 files changed, 93 insertions(+), 20 deletions(-)
> 
> diff --git a/src/input/decoder.c b/src/input/decoder.c
> index 7d4ccf2220..2c5dbf47cf 100644
> --- a/src/input/decoder.c
> +++ b/src/input/decoder.c
> @@ -1313,9 +1313,21 @@ static int ModuleThread_PlayVideo( vlc_input_decoder_t *p_owner, picture_t *p_pi
>     decoder_t *p_dec = &p_owner->dec;
> 
>     if( p_picture->date == VLC_TICK_INVALID )
> -        /* FIXME: VLC_TICK_INVALID -- verify video_output */
>     {
> -        msg_Warn( p_dec, "non-dated video buffer received" );
> +        /* Enhanced validation: Check if this is expected (e.g., during format changes)
> +         * or if we should attempt to generate a valid timestamp */
> +        if( p_owner->i_preroll_end != PREROLL_NONE || 
> +            (p_owner->p_vout && p_owner->vout_started) )
> +        {
> +            /* During normal playback, invalid timestamps indicate a problem */
> +            msg_Warn( p_dec, "non-dated video buffer received during active playback - "
> +                            "this may cause synchronization issues" );
> +        }
> +        else
> +        {
> +            /* During startup/flush, invalid timestamps may be expected */
> +            msg_Dbg( p_dec, "non-dated video buffer received during initialization" );
> +        }
>         picture_Release( p_picture );
>         return VLC_EGENERIC;
>     }
> @@ -1461,9 +1473,21 @@ static int ModuleThread_PlayAudio( vlc_input_decoder_t *p_owner, vlc_frame_t *p_
> 
>     assert( p_audio != NULL );
> 
> -    if( p_audio->i_pts == VLC_TICK_INVALID ) // FIXME --VLC_TICK_INVALID verify audio_output/*
> +    if( p_audio->i_pts == VLC_TICK_INVALID )
>     {
> -        msg_Warn( p_dec, "non-dated audio buffer received" );
> +        /* Enhanced validation: Check if this is expected (e.g., during format changes)
> +         * or if we should attempt to generate a valid timestamp */
> +        if( p_owner->i_preroll_end != PREROLL_NONE || p_owner->p_aout != NULL )
> +        {
> +            /* During normal playback, invalid timestamps indicate a problem */
> +            msg_Warn( p_dec, "non-dated audio buffer received during active playback - "
> +                            "this may cause synchronization issues" );
> +        }
> +        else
> +        {
> +            /* During startup/flush, invalid timestamps may be expected */
> +            msg_Dbg( p_dec, "non-dated audio buffer received during initialization" );
> +        }
>         block_Release( p_audio );
>         return VLC_EGENERIC;
>     }
> @@ -2420,13 +2444,29 @@ void vlc_input_decoder_DecodeWithStatus(vlc_input_decoder_t *p_owner, vlc_frame_
>     vlc_fifo_Lock( p_owner->p_fifo );
>     if( !b_do_pace )
>     {
> -        /* FIXME: ideally we would check the time amount of data
> -         * in the FIFO instead of its size. */
> -        /* 400 MiB, i.e. ~ 50mb/s for 60s */
> -        if( vlc_fifo_GetBytes( p_owner->p_fifo ) > 400*1024*1024 )
> +        /* Enhanced FIFO management: More detailed logging about the overflow
> +         * and better threshold values based on bitrate estimation */
> +        size_t i_fifo_bytes = vlc_fifo_GetBytes( p_owner->p_fifo );
> +        size_t i_fifo_count = vlc_fifo_GetCount( p_owner->p_fifo );
> +        
> +        /* Adaptive threshold: 400 MiB for high-bitrate content, but also consider
> +         * the number of frames to handle low-bitrate content with many small frames */
> +        bool b_fifo_full = (i_fifo_bytes > 400*1024*1024) || (i_fifo_count > 1000);
> +        
> +        if( b_fifo_full )
>         {
> -            msg_Warn( &p_owner->dec, "decoder/packetizer fifo full (data not "
> -                      "consumed quickly enough), resetting fifo!" );
> +            if( i_fifo_count > 1000 )
> +            {
> +                msg_Warn( &p_owner->dec, "decoder/packetizer fifo contains too many "
> +                         "frames (%zu frames, %zu bytes), resetting fifo!", 
> +                         i_fifo_count, i_fifo_bytes );
> +            }
> +            else
> +            {
> +                msg_Warn( &p_owner->dec, "decoder/packetizer fifo full (%zu bytes in %zu frames, "
> +                         "data not consumed quickly enough), resetting fifo!",
> +                         i_fifo_bytes, i_fifo_count );
> +            }
>             block_ChainRelease( vlc_fifo_DequeueAllUnlocked( p_owner->p_fifo ) );
>             frame->i_flags |= BLOCK_FLAG_DISCONTINUITY;
>         }
> diff --git a/src/input/stream.c b/src/input/stream.c
> index af5475be9d..e337c185a6 100644
> --- a/src/input/stream.c
> +++ b/src/input/stream.c
> @@ -303,16 +303,42 @@ char *vlc_stream_ReadLine( stream_t *s )
>         /* Resume search for an EOL where we left off */
>         const uint8_t *p_cur = p_data + i_line, *psz_eol;
> 
> -        /* FIXME: <CR> behavior varies depending on where buffer
> -           boundaries happen to fall; a <CR><LF> across the boundary
> -           creates a bogus empty line. */
> +        /* Enhanced CR/LF handling: Properly handle CR/LF sequences that span
> +           buffer boundaries to avoid creating bogus empty lines */
>         if( priv->text.char_width == 1 )
>         {
>             /* UTF-8: 0A <LF> */
>             psz_eol = memchr( p_cur, '\n', i_data - i_line );
>             if( psz_eol == NULL )
> -                /* UTF-8: 0D <CR> */
> -                psz_eol = memchr( p_cur, '\r', i_data - i_line );
> +            {
> +                /* UTF-8: 0D <CR> - but check if it might be part of CR/LF */
> +                const uint8_t *p_cr = memchr( p_cur, '\r', i_data - i_line );
> +                if( p_cr != NULL )
> +                {
> +                    /* Check if CR is at buffer boundary and might be followed by LF */
> +                    if( p_cr == p_data + i_data - 1 )
> +                    {
> +                        /* CR at end of buffer - peek ahead to see if next char is LF */
> +                        const uint8_t *p_peek_data;
> +                        ssize_t peek_result = vlc_stream_Peek( s, &p_peek_data, i_data + 1 );
> +                        if( peek_result > (ssize_t)i_data && p_peek_data[i_data] == '\n' )
> +                        {
> +                            /* This is a CR/LF sequence spanning boundary, skip CR for now */
> +                            psz_eol = NULL;
> +                        }
> +                        else
> +                        {
> +                            /* Standalone CR at boundary */
> +                            psz_eol = p_cr;
> +                        }
> +                    }
> +                    else
> +                    {
> +                        /* CR not at boundary - treat as line ending */
> +                        psz_eol = p_cr;
> +                    }
> +                }
> +            }
>         }
>         else
>         {
> diff --git a/src/video_output/video_output.c b/src/video_output/video_output.c
> index caa37535a4..6487e2be4a 100644
> --- a/src/video_output/video_output.c
> +++ b/src/video_output/video_output.c
> @@ -74,6 +74,9 @@ typedef struct vout_thread_sys_t
> 
>     bool dummy;
> 
> +    /* Discontinuity flag for better filter handling */
> +    bool discontinuity_flag;
> +
>     /* Splitter module if used */
>     char            *splitter_name;
> 
> @@ -1082,11 +1085,15 @@ static picture_t *PreparePicture(vout_thread_sys_t *vout, bool reuse_decoded,
>                     msg_Dbg(&vout->obj, "Using a new clock context (%u), "
>                             "flusing static filters", clock_id);
> 
> -                    /* Most deinterlace modules can't handle a PTS
> -                     * discontinuity, so flush them.
> -                     *
> -                     * FIXME: Pass a discontinuity flag and handle it in
> -                     * deinterlace modules. */
> +                    /* Enhanced discontinuity handling: Set a flag in the system to indicate
> +                     * that the next picture processed will have a discontinuity. This allows
> +                     * deinterlace filters to detect timestamp discontinuities and reset their
> +                     * internal state appropriately rather than just flushing everything. */
> +                    sys->discontinuity_flag = true;
> +
> +                    /* Most deinterlace modules can't handle a PTS discontinuity.
> +                     * We now set a discontinuity flag above for better handling, but also
> +                     * flush the filters as a fallback for filters that don't check the flag. */
>                     filter_chain_VideoFlush(sys->filter.chain_static);
>                 }
> 
> -- 
> 2.43.0
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel

_______________________________________________
vlc-devel mailing list
To unsubscribe or modify your subscription options:
https://mailman.videolan.org/listinfo/vlc-devel