[vlc-devel] [PATCH] [RFC] decoder: add a Flush callback to decoders/packetizers
Rémi Denis-Courmont
remi at remlab.net
Wed Nov 18 19:45:25 CET 2015
On Wednesday 18 November 2015 09:33:12 Steve Lhomme wrote:
> The flush handling is not passed to the DecoderProcessXXX functions anymore
>
> All decoder & packetizer, aout/vout/spu flushing is done in one place.
> The sout flushing will be part of another patch.
> --
> deprecates https://patches.videolan.org/patch/10776/
> ---
> include/vlc_codec.h | 1 +
> src/input/decoder.c | 145
> ++++++++++++++++++++++++++++++---------------------- 2 files changed, 85
> insertions(+), 61 deletions(-)
>
> diff --git a/include/vlc_codec.h b/include/vlc_codec.h
> index 1a2baa2..1399d26 100644
> --- a/include/vlc_codec.h
> +++ b/include/vlc_codec.h
> @@ -70,6 +70,7 @@ struct decoder_t
> block_t * ( * pf_decode_audio )( decoder_t *, block_t ** );
> subpicture_t * ( * pf_decode_sub) ( decoder_t *, block_t ** );
> block_t * ( * pf_packetize ) ( decoder_t *, block_t ** );
> + int ( * pf_flush ) ( decoder_t * );
Don´t add a return error code that you don´t/can´t handle.
Traditionally in I/O code, drain/flush failure means that some unexpected
error occurred during or before processing, but that the object state is reset
and resources freed regardless. In this case, the return value is useless: an
error message is probably better.
Those are the semantics apparently used in this patch.
But then the next patch uses the return value as a fatal error that is not
handled correctly. If you did not even do it correctly, nobody else will.
>
> /* Closed Caption (CEA 608/708) extraction.
> * If set, it *may* be called after pf_decode_video/pf_packetize
> diff --git a/src/input/decoder.c b/src/input/decoder.c
> index cacf7f2..b1adac3 100644
> --- a/src/input/decoder.c
> +++ b/src/input/decoder.c
> @@ -134,6 +134,8 @@ struct decoder_owner_sys_t
> /* */
> #define DECODER_SPU_VOUT_WAIT_DURATION ((int)(0.200*CLOCK_FREQ))
>
> +static int DecoderFlush( decoder_t *p_dec );
I´d avoid forward declarations in new code.
> +
> /**
> * Load a decoder module
> */
> @@ -148,6 +150,7 @@ static int LoadDecoder( decoder_t *p_dec, bool
> b_packetizer, p_dec->pf_decode_sub = NULL;
> p_dec->pf_get_cc = NULL;
> p_dec->pf_packetize = NULL;
> + p_dec->pf_flush = DecoderFlush;
NULL please. There are other decoder_t sites.
>
> es_format_Copy( &p_dec->fmt_in, p_fmt );
> es_format_Init( &p_dec->fmt_out, UNKNOWN_ES, 0 );
> @@ -224,6 +227,25 @@ static block_t *DecoderBlockFlushNew()
> return p_null;
> }
>
> +int DecoderFlush( decoder_t *p_dec )
> +{
Check if pf_flush is NULL here and dispatch accordingly.
> + block_t *p_flush = DecoderBlockFlushNew();
> + if( p_flush )
> + {
> + if (p_dec->pf_decode_video)
> + p_dec->pf_decode_video( p_dec, &p_flush );
> + else if (p_dec->pf_decode_audio)
> + p_dec->pf_decode_audio( p_dec, &p_flush );
> + else if (p_dec->pf_decode_sub)
> + p_dec->pf_decode_sub( p_dec, &p_flush );
> + else if (p_dec->pf_packetize)
> + p_dec->pf_packetize( p_dec, &p_flush );
> + else
> + block_Release( p_flush );
> + }
> + return VLC_SUCCESS;
> +}
> +
> /**************************************************************************
> *** * Buffers allocation callbacks for the decoders
>
> ***************************************************************************
> **/ @@ -945,6 +967,13 @@ static void DecoderProcessVideo( decoder_t *p_dec,
> block_t *p_block, bool b_flus {
> decoder_owner_sys_t *p_owner = (decoder_owner_sys_t *)p_dec->p_owner;
>
> + if ( b_flush )
> + {
> + if( p_owner->p_vout )
> + vout_Flush( p_owner->p_vout, VLC_TS_0 );
I believe that the Grand Plan from the Great Master was to use zero for
VLC_TS_0 , and INT64_MIN for VLC_TS_INVALID. Please don´t unfix this.
> + return;
> + }
Why is this reordered? Flushing in flow order seems more logical.
> +
> if( p_owner->p_packetizer )
> {
> block_t *p_packetized_block;
> @@ -982,22 +1011,11 @@ static void DecoderProcessVideo( decoder_t *p_dec,
> block_t *p_block, bool b_flus p_packetized_block = p_next;
> }
> }
> - /* The packetizer does not output a block that tell the decoder to
> flush - * do it ourself */
> - if( b_flush )
> - {
> - block_t *p_null = DecoderBlockFlushNew();
> - if( p_null )
> - DecoderDecodeVideo( p_dec, p_null );
> - }
> }
> else
> {
> DecoderDecodeVideo( p_dec, p_block );
> }
> -
> - if( b_flush && p_owner->p_vout )
> - vout_Flush( p_owner->p_vout, VLC_TS_INVALID+1 );
> }
>
> static void DecoderPlayAudio( decoder_t *p_dec, block_t *p_audio,
> @@ -1096,7 +1114,7 @@ static void DecoderDecodeAudio( decoder_t *p_dec,
> block_t *p_block )
>
> /* This function process a audio block
> */
> -static void DecoderProcessAudio( decoder_t *p_dec, block_t *p_block, bool
> b_flush ) +static void DecoderProcessAudio( decoder_t *p_dec, block_t
> *p_block ) {
> decoder_owner_sys_t *p_owner = (decoder_owner_sys_t *)p_dec->p_owner;
>
> @@ -1134,22 +1152,11 @@ static void DecoderProcessAudio( decoder_t *p_dec,
> block_t *p_block, bool b_flus p_packetized_block = p_next;
> }
> }
> - /* The packetizer does not output a block that tell the decoder to
> flush - * do it ourself */
> - if( b_flush )
> - {
> - block_t *p_null = DecoderBlockFlushNew();
> - if( p_null )
> - DecoderDecodeAudio( p_dec, p_null );
> - }
> }
> else
> {
> DecoderDecodeAudio( p_dec, p_block );
> }
> -
> - if( b_flush && p_owner->p_aout )
> - aout_DecFlush( p_owner->p_aout, false );
> }
>
> static void DecoderPlaySpu( decoder_t *p_dec, subpicture_t *p_subpic )
> @@ -1191,7 +1198,7 @@ static void DecoderPlaySpu( decoder_t *p_dec,
> subpicture_t *p_subpic )
>
> /* This function process a subtitle block
> */
> -static void DecoderProcessSpu( decoder_t *p_dec, block_t *p_block, bool
> b_flush )
> +static void DecoderProcessSpu( decoder_t *p_dec, block_t
> *p_block ) {
> decoder_owner_sys_t *p_owner = p_dec->p_owner;
>
> @@ -1230,17 +1237,6 @@ static void DecoderProcessSpu( decoder_t *p_dec,
> block_t *p_block, bool b_flush if( p_vout )
> vlc_object_release( p_vout );
> }
> -
> - if( b_flush && p_owner->p_spu_vout )
> - {
> - p_vout = input_resource_HoldVout( p_owner->p_resource );
> -
> - if( p_vout && p_owner->p_spu_vout == p_vout )
> - vout_FlushSubpictureChannel( p_vout, p_owner->i_spu_channel );
> -
> - if( p_vout )
> - vlc_object_release( p_vout );
> - }
Why remove this?
> }
>
> /**
> @@ -1269,45 +1265,72 @@ static void DecoderProcess( decoder_t *p_dec,
> block_t *p_block ) return;
> }
>
> -#ifdef ENABLE_SOUT
> - if( p_owner->b_packetizer )
> - {
> - if( p_block )
> - p_block->i_flags &= ~BLOCK_FLAG_CORE_PRIVATE_MASK;
> + bool b_flush = false;
>
> - DecoderProcessSout( p_dec, p_block );
> - }
> - else
> -#endif
> + if( p_block )
> {
> - bool b_flush = false;
> -
> - if( p_block )
> - {
> - const bool b_flushing = p_owner->i_preroll_end == INT64_MAX;
> - DecoderUpdatePreroll( &p_owner->i_preroll_end, p_block );
> + const bool b_flushing = p_owner->i_preroll_end == INT64_MAX;
> + DecoderUpdatePreroll( &p_owner->i_preroll_end, p_block );
>
> - b_flush = !b_flushing && b_flush_request;
> + b_flush = !b_flushing && b_flush_request;
>
> - p_block->i_flags &= ~BLOCK_FLAG_CORE_PRIVATE_MASK;
> - }
> + p_block->i_flags &= ~BLOCK_FLAG_CORE_PRIVATE_MASK;
> + }
>
> - if( p_dec->fmt_out.i_cat == AUDIO_ES )
> + if (b_flush)
> + {
> + if ( p_block )
> {
> - DecoderProcessAudio( p_dec, p_block, b_flush );
> + block_Release( p_block );
> + p_block = NULL;
> }
> - else if( p_dec->fmt_out.i_cat == VIDEO_ES )
> +
> + if ( p_owner->p_packetizer )
> + p_owner->p_packetizer->pf_flush( p_owner->p_packetizer );
> + p_dec->pf_flush( p_dec );
> +
> + if( p_owner->p_vout )
> + vout_Flush( p_owner->p_vout, VLC_TS_0 );
> + if( p_owner->p_aout )
> + aout_DecFlush( p_owner->p_aout, false );
> + if( p_owner->p_spu_vout )
> {
> - DecoderProcessVideo( p_dec, p_block, b_flush );
> + vout_thread_t *p_vout = input_resource_HoldVout(
> p_owner->p_resource ); +
> + if( p_vout && p_owner->p_spu_vout == p_vout )
> + vout_FlushSubpictureChannel( p_vout, p_owner->i_spu_channel
> ); +
> + if( p_vout )
> + vlc_object_release( p_vout );
> }
> - else if( p_dec->fmt_out.i_cat == SPU_ES )
> + }
> + else
> + {
> +#ifdef ENABLE_SOUT
> + if( p_owner->b_packetizer )
> {
> - DecoderProcessSpu( p_dec, p_block, b_flush );
> + DecoderProcessSout( p_dec, p_block );
> }
> else
> +#endif
> {
> - msg_Err( p_dec, "unknown ES format" );
> - p_dec->b_error = true;
> + if( p_dec->fmt_out.i_cat == AUDIO_ES )
> + {
> + DecoderProcessAudio( p_dec, p_block );
> + }
> + else if( p_dec->fmt_out.i_cat == VIDEO_ES )
> + {
> + DecoderProcessVideo( p_dec, p_block );
> + }
> + else if( p_dec->fmt_out.i_cat == SPU_ES )
> + {
> + DecoderProcessSpu( p_dec, p_block );
> + }
> + else
> + {
> + msg_Err( p_dec, "unknown ES format" );
> + p_dec->b_error = true;
> + }
> }
> }
> }
The idea was to no longer need blocks, and simplify things here...
--
Rémi Denis-Courmont
http://www.remlab.net/
More information about the vlc-devel
mailing list