[vlc-devel] [PATCH 1/1] Freetype: support Android's new fonts.xml
Filip Roséen
filip at atch.se
Sat Mar 11 21:34:23 CET 2017
Hi Salah-Eddin,
This reply contains my immediate reactions to the patch in question,
and some thoughts expressed might be unnecessary nit-picky (or safe to
ignore). I am currently very tired and therefore apologize in advance
if a comment is missing some crucial piece of information (rendering
it as false/unnecessary).
On 2017-03-11 10:45, Salah-Eddin Shaban wrote:
> ---
> modules/text_renderer/freetype/fonts/android.c | 207 ++++++++++++++++++++++--
> modules/text_renderer/freetype/freetype.c | 5 +-
> modules/text_renderer/freetype/platform_fonts.c | 2 +-
> modules/text_renderer/freetype/platform_fonts.h | 10 +-
> 4 files changed, 206 insertions(+), 18 deletions(-)
>
> diff --git a/modules/text_renderer/freetype/fonts/android.c b/modules/text_renderer/freetype/fonts/android.c
> index b07f70b..b02b70e 100644
> --- a/modules/text_renderer/freetype/fonts/android.c
> +++ b/modules/text_renderer/freetype/fonts/android.c
> @@ -42,16 +42,189 @@
>
> #include "../platform_fonts.h"
>
> +#define ANDROID_NEW_FONTS "file:///system/etc/fonts.xml"
> #define ANDROID_SYSTEM_FONTS "file:///system/etc/system_fonts.xml"
> #define ANDROID_FALLBACK_FONTS "file:///system/etc/fallback_fonts.xml"
> #define ANDROID_VENDOR_FONTS "file:///vendor/etc/fallback_fonts.xml"
> #define ANDROID_FONT_PATH "/system/fonts"
>
> +static int Android_ParseFont( filter_t *p_filter, xml_reader_t *p_xml,
> + vlc_family_t *p_family )
> +{
> + bool b_bold = false;
> + bool b_italic = false;
> + const char *psz_val = NULL;
> + const char *psz_attr = NULL;
> + int i_type = 0;
> + int i_weight = 0;
> +
> + while( ( psz_attr = xml_ReaderNextAttr( p_xml, &psz_val ) ) )
> + {
> + if( !strcasecmp( "weight", psz_attr ) && psz_val && *psz_val )
> + i_weight = atoi( psz_val );
The above check of `*psz_val` is redundant as `atoi` will return `0`
on an empty (or ill-formed) string.
> + else if( !strcasecmp( "style", psz_attr ) && psz_val && *psz_val )
> + if( !strcasecmp( "italic", psz_val ) )
> + b_italic = true;
Same thing about `*psz_val` in the above, `strcasecmp` will not return
`0` for an empty string.
> + }
> +
> + if( i_weight == 700 )
> + b_bold = true;
As a font can be either *bold* or *not-bold* through `NewFont`, it
might make sense to check whether `i_weight` is
*greater-than-or-equal* to 700 (as `701` would not result in a
*non-bold* font).
> +
> + i_type = xml_ReaderNextNode( p_xml, &psz_val );
> +
> + if( i_type != XML_READER_TEXT || !psz_val || !*psz_val )
> + {
> + msg_Warn( p_filter, "Android_ParseFont: no file name" );
> + return VLC_EGENERIC;
> + }
> +
> + char *psz_fontfile;
> +
> + /*
> + * We don't need all font weights. Only 400 (regular) and 700 (bold)
> + */
> + if( i_weight == 400 || i_weight == 700 )
> + if( asprintf( &psz_fontfile, "%s/%s", ANDROID_FONT_PATH, psz_val ) < 0
> + || !NewFont( psz_fontfile, 0, b_bold, b_italic, p_family ) )
> + return VLC_ENOMEM;
> +
> + return VLC_SUCCESS;
Leak of the `asprintf`-allocated resource referred to by `psz_fontfile`.
> +}
> +
> static int Android_ParseFamily( filter_t *p_filter, xml_reader_t *p_xml )
> {
> filter_sys_t *p_sys = p_filter->p_sys;
> vlc_dictionary_t *p_dict = &p_sys->family_map;
> vlc_family_t *p_family = NULL;
> + const char *psz_val = NULL;
> + const char *psz_attr = NULL;
> + const char *psz_name = NULL;
> + int i_type = 0;
> +
> + while( ( psz_attr = xml_ReaderNextAttr( p_xml, &psz_val ) ) )
> + {
> + if( !strcasecmp( "name", psz_attr ) && psz_val && *psz_val )
> + {
> + psz_name = psz_val;
> + break;
> + }
> + }
> +
> + if( psz_name && *psz_name )
> + {
> + /*
> + * Family has a name. See if we have that name already.
> + * If the name already exists, it's one of the font attachments.
> + */
> + char *psz_lc = ToLower( psz_name );
> + if( unlikely( !psz_lc ) )
> + return VLC_ENOMEM;
> +
> + p_family = vlc_dictionary_value_for_key( p_dict, psz_lc );
> +
> + free( psz_lc );
> + }
> +
> + if( p_family == NULL )
> + {
> + /*
> + * We are either parsing a nameless family, or a named family that
> + * was not previously added to p_sys->family_map.
> + *
> + * Create a new family with the given name or, if psz_name is NULL,
> + * with the name fallback-xxxx
> + */
> + p_family = NewFamily( p_filter, psz_name, &p_sys->p_families,
> + &p_sys->family_map, NULL );
> +
> + if( unlikely( !p_family ) )
> + return VLC_ENOMEM;
> + }
> +
> + while( ( i_type = xml_ReaderNextNode( p_xml, &psz_val ) ) > 0 )
> + {
> + switch( i_type )
> + {
> + case XML_READER_STARTELEM:
> + if( !strcasecmp( "font", psz_val ) )
> + if( Android_ParseFont( p_filter, p_xml, p_family ) == VLC_ENOMEM )
> + return VLC_ENOMEM;
There is the possibility that the above call to `Android_ParseFont`
consumes the `XML_READER_ENDELEM` for `</family>` if the file contains
unexpected data (such as the below, contrived example).
<familyset version="42">
<family name="sans-serif">
<font weight="700" style="normal"></font>
</family>
<family name="monospace>
<font weight="400" style="normal">DroidSansMono.ttf</font>
</family>
</familyset>
As `familyset.family.font[weight="700"]` does not contain a
`XML_READER_TEXT` node, `Android_ParseFont` would return
`VLC_EGENERIC`, but as you only check for `VLC_ENOMEM` the affected
loop would keep running, eventually adding `DroidSansMono.ttf` to the
family named `sans-serif` instead of `monospace`.
> + break;
> +
> + case XML_READER_ENDELEM:
> + if( !strcasecmp( "family", psz_val ) )
> + {
> + if( strcasestr( p_family->psz_name, FB_NAME ) )
> + {
> + /*
> + * If the family name has "fallback" in it add it to the
> + * default fallback list.
> + */
> + vlc_family_t *p_fallback =
> + NewFamily( p_filter, p_family->psz_name,
> + NULL, &p_sys->fallback_map, FB_LIST_DEFAULT );
> +
> + if( unlikely( !p_fallback ) )
> + return VLC_ENOMEM;
> +
> + p_fallback->p_fonts = p_family->p_fonts;
> + }
> +
> + return VLC_SUCCESS;
> + }
> + break;
> + }
> + }
> +
> + msg_Warn( p_filter, "Android_ParseFamily: Corrupt font configuration file" );
> + return VLC_EGENERIC;
> +}
> +
> +static int Android_ParseAlias( filter_t *p_filter, xml_reader_t *p_xml )
> +{
> + filter_sys_t *p_sys = p_filter->p_sys;
> + vlc_dictionary_t *p_dict = &p_sys->family_map;
> + vlc_family_t *p_dest = NULL;
> + char *psz_name = NULL;
> + char *psz_dest = NULL;
> + const char *psz_val = NULL;
> + const char *psz_attr = NULL;
> + int i_weight = 0;
> + int i_ret = VLC_SUCCESS;
> +
> + while( ( psz_attr = xml_ReaderNextAttr( p_xml, &psz_val ) ) )
> + {
> + if( !strcasecmp( "weight", psz_attr ) && psz_val && *psz_val )
> + i_weight = atoi( psz_val );
> + else if( !strcasecmp( "to", psz_attr ) && psz_val && *psz_val )
> + psz_dest = ToLower( psz_val );
> + else if( !strcasecmp( "name", psz_attr ) && psz_val && *psz_val )
> + psz_name = ToLower( psz_val );
- See previous remarks regarding `*psz_val`.
- If a node, for some reason, contains attributes such as
`to="..."` **and** `To="..."` the above would leak acquired
resources (as xml-attributes are *case-sensitive*).
> + }
> +
> + if( !psz_dest || !psz_name )
> + {
> + i_ret = VLC_EGENERIC;
> + goto done;
> + }
> +
> + p_dest = vlc_dictionary_value_for_key( p_dict, psz_dest );
> +
> + if( p_dest && i_weight == 0 )
> + if( vlc_dictionary_value_for_key( p_dict, psz_name ) == NULL )
> + vlc_dictionary_insert( p_dict, psz_name, p_dest );
> +
> +done:
> + free( psz_dest );
> + free( psz_name );
> + return i_ret;
> +}
> +
> +static int Android_ParseOldFamily( filter_t *p_filter, xml_reader_t *p_xml )
> +{
> + filter_sys_t *p_sys = p_filter->p_sys;
> + vlc_dictionary_t *p_dict = &p_sys->family_map;
> + vlc_family_t *p_family = NULL;
> char *psz_lc = NULL;
> int i_counter = 0;
> bool b_bold = false;
> @@ -111,7 +284,7 @@ static int Android_ParseFamily( filter_t *p_filter, xml_reader_t *p_xml )
> /*
> * If p_family has not been set by the time we encounter the first file,
> * it means this family has no name, and should be used only as a fallback.
> - * We create a new family for it in the master list with the name "fallback-xx"
> + * We create a new family for it in the master list with the name "fallback-xxxx"
> * and later add it to the "default" fallback list.
> */
> else if( !strcasecmp( "file", p_node ) )
> @@ -194,11 +367,12 @@ static int Android_ParseFamily( filter_t *p_filter, xml_reader_t *p_xml )
> }
> }
>
> - msg_Warn( p_filter, "Android_ParseFamily: Corrupt font configuration file" );
> + msg_Warn( p_filter, "Android_ParseOldFamily: Corrupt font configuration file" );
> return VLC_EGENERIC;
> }
>
> -static int Android_ParseSystemFonts( filter_t *p_filter, const char *psz_path )
> +static int Android_ParseSystemFonts( filter_t *p_filter, const char *psz_path,
> + bool b_new_format )
> {
> int i_ret = VLC_SUCCESS;
> stream_t *p_stream = vlc_stream_NewURL( p_filter, psz_path );
> @@ -218,11 +392,21 @@ static int Android_ParseSystemFonts( filter_t *p_filter, const char *psz_path )
> int i_type;
> while( ( i_type = xml_ReaderNextNode( p_xml, &p_node ) ) > 0 )
> {
> - if( i_type == XML_READER_STARTELEM && !strcasecmp( "family", p_node ) )
> + if( i_type == XML_READER_STARTELEM && !strcasecmp( "family", p_node ) && b_new_format )
> {
> if( ( i_ret = Android_ParseFamily( p_filter, p_xml ) ) )
> break;
> }
> + else if( i_type == XML_READER_STARTELEM && !strcasecmp( "family", p_node ) && !b_new_format )
> + {
> + if( ( i_ret = Android_ParseOldFamily( p_filter, p_xml ) ) )
> + break;
> + }
> + else if( i_type == XML_READER_STARTELEM && !strcasecmp( "alias", p_node ) && b_new_format )
> + {
> + if( ( i_ret = Android_ParseAlias( p_filter, p_xml ) ) )
> + break;
> + }
> }
>
> xml_ReaderDelete( p_xml );
> @@ -232,12 +416,15 @@ static int Android_ParseSystemFonts( filter_t *p_filter, const char *psz_path )
>
> int Android_Prepare( filter_t *p_filter )
> {
> - if( Android_ParseSystemFonts( p_filter, ANDROID_SYSTEM_FONTS ) == VLC_ENOMEM )
> - return VLC_EGENERIC;
> - if( Android_ParseSystemFonts( p_filter, ANDROID_FALLBACK_FONTS ) == VLC_ENOMEM )
> - return VLC_EGENERIC;
> - if( Android_ParseSystemFonts( p_filter, ANDROID_VENDOR_FONTS ) == VLC_ENOMEM )
> - return VLC_EGENERIC;
> + if( Android_ParseSystemFonts( p_filter, ANDROID_NEW_FONTS, true ) != VLC_SUCCESS )
> + {
> + if( Android_ParseSystemFonts( p_filter, ANDROID_SYSTEM_FONTS, false ) == VLC_ENOMEM )
> + return VLC_ENOMEM;
> + if( Android_ParseSystemFonts( p_filter, ANDROID_FALLBACK_FONTS, false ) == VLC_ENOMEM )
> + return VLC_ENOMEM;
> + if( Android_ParseSystemFonts( p_filter, ANDROID_VENDOR_FONTS, false ) == VLC_ENOMEM )
> + return VLC_ENOMEM;
> + }
Should this function really return `VLC_SUCCESS` if all calls to
`Android_ParseSystemFonts` fail (for other reason then `VLC_ENOMEM`)?
>
> return VLC_SUCCESS;
> }
> diff --git a/modules/text_renderer/freetype/freetype.c b/modules/text_renderer/freetype/freetype.c
> index 76a60fe..29d2def 100644
> --- a/modules/text_renderer/freetype/freetype.c
> +++ b/modules/text_renderer/freetype/freetype.c
> @@ -319,7 +319,7 @@ static int LoadFontsFromAttachments( filter_t *p_filter )
> if( p_face->family_name )
> psz_lc = ToLower( p_face->family_name );
> else
> - if( asprintf( &psz_lc, FB_NAME"-%02d",
> + if( asprintf( &psz_lc, FB_NAME"-%04d",
> p_sys->i_fallback_counter++ ) < 0 )
> psz_lc = NULL;
>
> @@ -1299,7 +1299,8 @@ static int Create( vlc_object_t *p_this )
> p_sys->pf_get_fallbacks = Android_GetFallbacks;
> p_sys->pf_select = Generic_Select;
>
> - Android_Prepare( p_filter );
> + if( Android_Prepare( p_filter ) == VLC_ENOMEM )
> + goto error;
> #else
> p_sys->pf_select = Dummy_Select;
> #endif
> diff --git a/modules/text_renderer/freetype/platform_fonts.c b/modules/text_renderer/freetype/platform_fonts.c
> index 4ea648f..f4af71e 100644
> --- a/modules/text_renderer/freetype/platform_fonts.c
> +++ b/modules/text_renderer/freetype/platform_fonts.c
> @@ -249,7 +249,7 @@ vlc_family_t *NewFamily( filter_t *p_filter, const char *psz_family,
> if( psz_family && *psz_family )
> psz_name = ToLower( psz_family );
> else
> - if( asprintf( &psz_name, FB_NAME"-%02d",
> + if( asprintf( &psz_name, FB_NAME"-%04d",
I am not familiar with the inner workings of `text_renderer/freetype`,
but is there a particular reason for the padding?
> p_sys->i_fallback_counter++ ) < 0 )
> psz_name = NULL;
>
> diff --git a/modules/text_renderer/freetype/platform_fonts.h b/modules/text_renderer/freetype/platform_fonts.h
> index 83af800..e4a2ac2 100644
> --- a/modules/text_renderer/freetype/platform_fonts.h
> +++ b/modules/text_renderer/freetype/platform_fonts.h
> @@ -68,8 +68,8 @@ extern "C" {
> # define SYSTEM_DEFAULT_MONOSPACE_FONT_FILE "/psfonts/mtsansdk.ttf"
> # define SYSTEM_DEFAULT_MONOSPACE_FAMILY "Monotype Sans Duospace WT K"
> #elif defined( __ANDROID__ )
> -# define SYSTEM_DEFAULT_FONT_FILE "/system/fonts/DroidSans-Bold.ttf"
> -# define SYSTEM_DEFAULT_FAMILY "Droid Sans"
> +# define SYSTEM_DEFAULT_FONT_FILE "/system/fonts/Roboto-Regular.ttf"
> +# define SYSTEM_DEFAULT_FAMILY "sans-serif"
> # define SYSTEM_DEFAULT_MONOSPACE_FONT_FILE "/system/fonts/DroidSansMono.ttf"
> # define SYSTEM_DEFAULT_MONOSPACE_FAMILY "Monospace"
> #else
> @@ -121,7 +121,7 @@ struct vlc_family_t
> vlc_family_t *p_next; /**< next family in the chain */
> /**
> * Human-readable name, usually requested.
> - * Can be fallback-xx for font attachments with no family name, and for fallback
> + * Can be fallback-xxxx for font attachments with no family name, and for fallback
> * fonts in Android.
> * This field is used only for loading the family fonts, and for debugging.
> * Apart from that, families are accessed either through the
> @@ -193,7 +193,7 @@ char* Generic_Select( filter_t *p_filter, const char* family,
> * Creates a new family.
> *
> * \param psz_family the usual font family name, human-readable;
> - * if NULL, will use "fallback-xx"[IN]
> + * if NULL, will use "fallback-xxxx"[IN]
> * \param pp_list the family list where to append the new family;
> * can be NULL if not in a list, or if the family is to be appended to a fallback list
> * within \ref filter_sys_t::fallback_map [IN]
> @@ -203,7 +203,7 @@ char* Generic_Select( filter_t *p_filter, const char* family,
> * appended there [IN]
> * \param psz_key specific key for the dictionary.
> * If NULL will use whatever is used for the family name, whether it is the specified \p psz_family
> - * or "fallback-xx" [IN]
> + * or "fallback-xxxx" [IN]
> *
> * \return the new family representation
> */
> --
> 2.10.2
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170311/c186fa81/attachment.html>
More information about the vlc-devel
mailing list