[vlc-devel] [PATCH] Added textStyleInit. Cleaned modules
Laurent Aimar
fenrir at via.ecp.fr
Sat Jan 10 20:24:11 CET 2009
Hi,
Thanks for your work.
Here are some remarks I have on your patch:
On Thu, Jan 08, 2009, basos g wrote:
> Added a method to initialize or copy text style struct
> taking care for memmory management. Please mind that on
> deletion you have to free the p_style->psz_fontname
> before p_style itself.. The vout_ShowText* functions
> will take a p_style and copy it internally so you have
> to free in the same function. Other modules where p_style
> was malloced there are now fixed to copy before calling
> the sout* functions.
> From b8f5a3587b8a0a5b8380793e66926cf1dea042ca Mon Sep 17 00:00:00 2001
> From: basOS G <noxelia 4t gmail , com>
> Date: Mon, 29 Dec 2008 23:11:05 +0200
> Subject: [PATCH] Added textStyleInit. Cleaned modules
>
> Added a method to initialize or copy text style struct
> taking care for memmory management. Please mind that on
> deletion you have to free the p_style->psz_fontname
> before p_style itself.. The vout_ShowText* functions
> will take a p_style and copy it internally so you have
> to free in the same function. Other modules where p_style
> was malloced there are now fixed to copy before calling
> the sout* functions.
> ---
> include/vlc_osd.h | 8 ++++-
> modules/codec/subtitles/subsass.c | 2 +-
> modules/codec/subtitles/subsusf.c | 10 ++++--
> modules/gui/fbosd.c | 5 ++-
> .../video_filter/dynamicoverlay/dynamicoverlay.c | 4 +--
> .../video_filter/dynamicoverlay/dynamicoverlay.h | 2 +-
> .../dynamicoverlay/dynamicoverlay_commands.c | 16 +++++----
> modules/video_filter/marq.c | 8 +++--
> modules/video_filter/rss.c | 10 ++++--
> src/libvlccore.sym | 1 +
> src/video_output/video_text.c | 4 +-
> src/video_output/vout_subpictures.c | 32 +++++++++++++++++++-
> 12 files changed, 74 insertions(+), 28 deletions(-)
>
> diff --git a/include/vlc_osd.h b/include/vlc_osd.h
> index df855f6..aef1f4b 100644
> --- a/include/vlc_osd.h
> +++ b/include/vlc_osd.h
> @@ -265,8 +265,12 @@ struct text_style_t
> #define STYLE_UNDERLINE 32
> #define STYLE_STRIKEOUT 64
>
> -static const text_style_t default_text_style = { NULL, 22, 0xffffff, 0xff, STYLE_OUTLINE,
> - 0x000000, 0xff, 0x000000, 0xff, 0xffffff, 0x80, 0xffffff, 0xff, 1, 0, -1 };
> +/**
> + * Initialize a text_style_t struct from another or default when arg is NULL
> + *
> + * Use this whenever a new text_style_t is wanted to properly manage memory
> + */
> +VLC_EXPORT( text_style_t* , textStyleInit, ( text_style_t* ) );
Could you use a more coherent 'vlc' style :
text_style_Duplicate would be better.
Also adding:
text_style_New: to create a new style, avoid text_style_Duplicate(NULL)
text_style_Delete: to avoid the double free everywhere
text_style_Copy (like duplicate, but does not allocate the text_style_t).
would be nice :)
> /**
> * OSD menu button states
> diff --git a/modules/codec/subtitles/subsass.c b/modules/codec/subtitles/subsass.c
> index e642673..b712988 100644
> --- a/modules/codec/subtitles/subsass.c
> +++ b/modules/codec/subtitles/subsass.c
> @@ -132,7 +132,7 @@ void ParseSSAString( decoder_t *p_dec,
> else
> {
> msg_Dbg( p_dec, "style is: %s", p_style->psz_stylename);
> - p_spu->p_region->p_style = &p_style->font_style;
> + p_spu->p_region->p_style = textStyleInit( &p_style->font_style ) ;
> p_spu->p_region->i_align = p_style->i_align;
> if( p_style->i_align & SUBPICTURE_ALIGN_LEFT )
> {
> diff --git a/modules/codec/subtitles/subsusf.c b/modules/codec/subtitles/subsusf.c
> index ab7756b..fceebd0 100644
> --- a/modules/codec/subtitles/subsusf.c
> +++ b/modules/codec/subtitles/subsusf.c
> @@ -414,7 +414,11 @@ static subpicture_region_t *CreateTextRegion( decoder_t *p_dec,
> {
> msg_Dbg( p_dec, "style is: %s", p_style->psz_stylename );
>
> - p_text_region->p_style = &p_style->font_style;
> + /* basos g: region->p_style is freed by the vout. Decoder may live shorter.
> + cmnt from spu_DestroyRegion :"fenrir plugin does not allocate the memory for it.
> + I think it might lead to segfault, video renderer can live longer than the decoder"
> + */
I think the comment "I think it might lead to segfault.." is useless now, no ?
> - memcpy( p_block->p_buffer, p_attach->p_data, p_attach->i_data );
> + vlc_memcpy( p_block->p_buffer, p_attach->p_data, p_attach->i_data );
vlc_memcpy must be used for large block only. For small block it will
be really slower than default memcpy (the compilo can optimise it).
Is this buffer large ? (multiple kbytes).
> @@ -657,7 +661,7 @@ static void ParseUSFHeaderTags( decoder_t *p_dec, xml_reader_t *p_xml_reader )
> {
> ssa_style_t *p_default_style = p_sys->pp_ssa_styles[i];
>
> - memcpy( p_style, p_default_style, sizeof( ssa_style_t ) );
> + vlc_memcpy( p_style, p_default_style, sizeof( ssa_style_t ) );
> p_style->font_style.psz_fontname = strdup( p_style->font_style.psz_fontname );
Adding and using text_style_Copy will simplify it.
Also, here, vlc_memcpy is to be avoided.
> + free( p_intf->p_sys->p_style->psz_fontname ) ;
> free( p_intf->p_sys->p_style );
text_style_Delete would simplify it.
> free( p_intf->p_sys );
> return VLC_ENOMEM;
> @@ -521,6 +521,7 @@ static void Destroy( vlc_object_t *p_this )
> if( p_sys->p_overlay )
> picture_Release( p_sys->p_overlay );
>
> + free( p_sys->p_style->psz_fontname ) ;
> free( p_sys->p_style );
Same here (it applies for all following alike cases).
> /**
> + * Initialize a text style struct managing all the memory stuff
> + * when p_style_in is NULL initialize with the default text style
> + */
> +text_style_t* textStyleInit( text_style_t* p_style_in )
> +{
> + text_style_t* p_style_out = NULL ;
> + p_style_out = malloc( sizeof( text_style_t ) ) ;
> + if ( !p_style_out)
Useless space.
> + return NULL ;
> + if ( p_style_in )
Idem.
> + vlc_memcpy( p_style_out, p_style_in, sizeof( text_style_t ) );
> + else
> + vlc_memcpy( p_style_out, &default_text_style, sizeof( text_style_t ) ) ;
No vlc_memcpy here.
> + if ( p_style_out->psz_fontname )
> + p_style_out->psz_fontname = strdup( p_style_out->psz_fontname ) ;
Either set p_style_out->psz_fontname to NULL or use calloc iof malloc.
> > + if ( p_region->p_style ) {
> + free (p_region->p_style->psz_fontname ) ;
> + free( p_region->p_style );
> + }
text_style_Delete.
I have supposed you have look for everywhere the text_style_t allocation is
needed.
--
fenrir
More information about the vlc-devel
mailing list