[vlc-devel] [PATCH] Added textStyleInit. Cleaned modules
basos g
noxelia at gmail.com
Sat Jan 10 21:49:03 CET 2009
2009/1/10 Laurent Aimar <fenrir at via.ecp.fr>:
> 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.
>
I didn't find a std style
in 098 i used module_GetObjName
in 100 module_get_object
Anyway this is logical
> 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).
>
nice. I though of it but decided not to add xtra symbols.
> 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 ?
>
yes
>> - 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).
>
to investigate. But i though it has been a leftover from past and that
vlc_memcpy is used from now and on.
>> @@ -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.
good point (avoided crashes )
>
>
>> > + 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.
As far as i can tell yes. I had made a thorough search.
Wait for a re-patch next days.
Thanks for review
>
> --
> fenrir
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> http://mailman.videolan.org/listinfo/vlc-devel
>
More information about the vlc-devel
mailing list