<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<meta http-equiv="Content-Style-Type" content="text/css" />
<meta name="generator" content="pandoc" />
<title></title>
<style type="text/css">code{white-space: pre;}</style>
</head>
<body>
<p>Hi Salah-Eddin,</p>
<p>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).</p>
<p>On 2017-03-11 10:45, Salah-Eddin Shaban wrote:</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> ---
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 );</code></pre>
</blockquote>
<p>The above check of <code>*psz_val</code> is redundant as <code>atoi</code> will return <code>0</code> on an empty (or ill-formed) string.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + else if( !strcasecmp( "style", psz_attr ) && psz_val && *psz_val )
+ if( !strcasecmp( "italic", psz_val ) )
+ b_italic = true;</code></pre>
</blockquote>
<p>Same thing about <code>*psz_val</code> in the above, <code>strcasecmp</code> will not return <code>0</code> for an empty string.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + }
+
+ if( i_weight == 700 )
+ b_bold = true;</code></pre>
</blockquote>
<p>As a font can be either <em>bold</em> or <em>not-bold</em> through <code>NewFont</code>, it might make sense to check whether <code>i_weight</code> is <em>greater-than-or-equal</em> to 700 (as <code>701</code> would not result in a <em>non-bold</em> font).</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +
+ 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;</code></pre>
</blockquote>
<p>Leak of the <code>asprintf</code>-allocated resource referred to by <code>psz_fontfile</code>.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> +}
+
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;</code></pre>
</blockquote>
<p>There is the possibility that the above call to <code>Android_ParseFont</code> consumes the <code>XML_READER_ENDELEM</code> for <code></family></code> if the file contains unexpected data (such as the below, contrived example).</p>
<pre><code><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></code></pre>
<p>As <code>familyset.family.font[weight="700"]</code> does not contain a <code>XML_READER_TEXT</code> node, <code>Android_ParseFont</code> would return <code>VLC_EGENERIC</code>, but as you only check for <code>VLC_ENOMEM</code> the affected loop would keep running, eventually adding <code>DroidSansMono.ttf</code> to the family named <code>sans-serif</code> instead of <code>monospace</code>.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + 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 );</code></pre>
</blockquote>
<ul>
<li>See previous remarks regarding <code>*psz_val</code>.</li>
<li>If a node, for some reason, contains attributes such as <code>to="..."</code> <strong>and</strong> <code>To="..."</code> the above would leak acquired resources (as xml-attributes are <em>case-sensitive</em>).</li>
</ul>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + }
+
+ 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;
+ }</code></pre>
</blockquote>
<p>Should this function really return <code>VLC_SUCCESS</code> if all calls to <code>Android_ParseSystemFonts</code> fail (for other reason then <code>VLC_ENOMEM</code>)?</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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",</code></pre>
</blockquote>
<p>I am not familiar with the inner workings of <code>text_renderer/freetype</code>, but is there a particular reason for the padding?</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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</code></pre>
</blockquote>
</body>
</html>