[vlc-devel] [vlc-commits] decoder: add Destroy helper function for decoder owners

[Rémi Denis-Courmont]

Le maanantaina 18. helmikuuta 2019, 17.52.32 EET Steve Lhomme a écrit :
> On 18/02/2019 16:32, Thomas Guillem wrote:
> > On Mon, Feb 18, 2019, at 16:11, Steve Lhomme wrote:
> >> vlc | branch: master | Steve Lhomme <robux4 at ycbcr.xyz> | Mon Feb 18
> >> 14:50:21 2019 +0100| [4a79e6a4929085cd78024e0c0367c2bc01ab11b4] |
> >> committer: Steve Lhomme
> >> 
> >> decoder: add Destroy helper function for decoder owners
> >> 
> >>> http://git.videolan.org/gitweb.cgi/vlc.git/?a=commit;h=4a79e6a4929085cd7
> >>> 8024e0c0367c2bc01ab11b4>> 
> >> ---
> >> 
> >>   include/vlc_codec.h         |  7 +++++++
> >>   src/input/decoder_helpers.c | 29 +++++++++++++++++++++++++++++
> >>   src/libvlccore.sym          |  1 +
> >>   3 files changed, 37 insertions(+)
> >> 
> >> diff --git a/include/vlc_codec.h b/include/vlc_codec.h
> >> index c287953ba7..2042ad2424 100644
> >> --- a/include/vlc_codec.h
> >> +++ b/include/vlc_codec.h
> >> @@ -320,6 +320,13 @@ VLC_API void decoder_AbortPictures( decoder_t
> >> *dec, bool b_abort );
> >> 
> >>   VLC_API void decoder_Init( decoder_t *dec, const es_format_t * );
> >>   
> >>   /**
> >> 
> >> + * Destroy a decoder and reset the structure.
> >> + *
> >> + * To be used by decoder owners.
> >> + */
> >> +VLC_API void decoder_Destroy( decoder_t **p_dec );
> >> +
> >> +/**
> >> 
> >>    * This function queues a single picture to the video output.
> >>    *
> >>    * \note
> >> 
> >> diff --git a/src/input/decoder_helpers.c b/src/input/decoder_helpers.c
> >> index 51f7ce8f6d..b60b830123 100644
> >> --- a/src/input/decoder_helpers.c
> >> +++ b/src/input/decoder_helpers.c
> >> @@ -28,6 +28,8 @@
> >> 
> >>   #include <vlc_common.h>
> >>   #include <vlc_codec.h>
> >> 
> >> +#include <vlc_meta.h>
> >> +#include <vlc_modules.h>
> >> 
> >>   void decoder_Init( decoder_t *p_dec, const es_format_t *restrict p_fmt
> >> 
> >> )
> >> 
> >>   {
> >> 
> >> @@ -43,3 +45,30 @@ void decoder_Init( decoder_t *p_dec, const
> >> es_format_t *restrict p_fmt )
> >> 
> >>       es_format_Copy( &p_dec->fmt_in, p_fmt );
> >>       es_format_Init( &p_dec->fmt_out, p_fmt->i_cat, 0 );
> >>   
> >>   }
> >> 
> >> +
> >> +static void decoder_Clean( decoder_t *p_dec )
> >> +{
> >> +    es_format_Clean( &p_dec->fmt_in );
> >> +    es_format_Clean( &p_dec->fmt_out );
> >> +
> >> +    if ( p_dec->p_description )
> >> +    {
> >> +        vlc_meta_Delete(p_dec->p_description);
> >> +        p_dec->p_description = NULL;
> >> +    }
> >> +    if ( p_dec->p_module != NULL )
> >> +    {
> >> +        module_unneed(p_dec, p_dec->p_module);
> >> +        p_dec->p_module = NULL;
> >> +    }
> >> +}
> >> +
> >> +void decoder_Destroy( decoder_t **p_dec )
> >> +{
> >> +    if (*p_dec != NULL)
> >> +    {
> >> +        decoder_Clean( *p_dec );
> >> +        vlc_object_release( *p_dec );
> >> +        *p_dec = NULL;
> >> +    }
> > 
> > I really prefer to use a decoder_t * here.
> > There is not many functions in VLC that set the pointer to NULL. This will
> > add more confusion.
> > 
> > I really prefer having a crash (via assert/ASAN) when a client is misusing
> > an API (calling Destroy with a freed pointer) instead of ignoring it.

> A double free doesn't always crash on Windows and there's no ASAN.

So your point is that because the bug is hard to observe on Windows, it should 
be made hard to observe everywhere? This is punishing everybody for your 
personal selection of development environment. What the hell...

-- 
雷米‧德尼-库尔蒙
http://www.remlab.net/