Issue 5797066: Add CreateFallbackForScript to SkTypeface for Android. (Closed)
DescriptionAdd CreateFallbackForScript to SkTypeface for Android.
WebKit uses HarfBuzz directly to do Complex Text Layout, so it needs to get the
proper SkTypeface to pass it to HarfBuzz. However, on Android, fallback scripts
have no name, and we can only get them by file name each time (CreateFromFile).
This actually breaks the semantics of SkTypeface, which states 'The ID should
be unique for the underlying font file/data, not unique per typeface instance.'
And add 2 helper function to convert between FallbackScripts enum and font file
name. These are useful for WebKit's FontCache, which needs string as key.
BUG=525
Patch Set 1 #
Total comments: 9
Patch Set 2 : #Patch Set 3 : #
Total comments: 1
Patch Set 4 : #
Total comments: 7
Patch Set 5 : #MessagesTotal messages: 27
Looks fine to me. Since this is definitely android-specific (no other port even uses the fallback mechanism) I think you can consider make the API completely private to Android: just create a new header for this method, with no need to modify either SkTypeface.h or SkFontHost.h. We have done this for nearly every other port, to expose port-specific ways to create fonts. See SkTypeface_mac.h and SkTypeface_win.h https://codereview.appspot.com/5797066/diff/1/include/core/SkTypeface.h File include/core/SkTypeface.h (right): https://codereview.appspot.com/5797066/diff/1/include/core/SkTypeface.h#newco... include/core/SkTypeface.h:123: enum FallbackScript { Lets add a suffix to these enums, and make the enum name plural (FallbackScripts) kArabic_FallbackScript, kEthiopic_FallbackScript, ... https://codereview.appspot.com/5797066/diff/1/src/ports/SkFontHost_android.cpp File src/ports/SkFontHost_android.cpp (right): https://codereview.appspot.com/5797066/diff/1/src/ports/SkFontHost_android.cp... src/ports/SkFontHost_android.cpp:494: Skia never marks a POD parameter as const. https://codereview.appspot.com/5797066/diff/1/src/ports/SkFontHost_android.cp... src/ports/SkFontHost_android.cpp:495: static void map_fallback_scripts(const char fileName[], const uint32_t uniqueID) { 1. Can this be written as a table instead. Might make it easier to understand and modify in the future (or extend at runtime perhaps via xml). e.g. static const struct { const char* fFileName; FallbackScript fScript; } gFallbackTable[] = { { "DroidNaskh-Regular.ttf", SkTypeface::kArabic }, { "DroidSansEthiopic-Regular.ttf", SkTypeface::kEthiopic }, ... }; Now the code just spins in a loop, returning the script associated with the matching name. 2. Are these filenames already stored somewhere else? Any chance we can reuse those, so we don't have to manually keep the two lists in sync? https://codereview.appspot.com/5797066/diff/1/src/ports/SkFontHost_android.cp... src/ports/SkFontHost_android.cpp:723: SkTypeface* SkFontHost::CreateFallbackTypefaceForScript( Do we need to call load_system_fonts() first, before we look at gFallbackScriptsMap? Will it always be initialized?
Sign in to reply to this message.
https://codereview.appspot.com/5797066/diff/1/src/ports/SkFontHost_android.cpp File src/ports/SkFontHost_android.cpp (right): https://codereview.appspot.com/5797066/diff/1/src/ports/SkFontHost_android.cp... src/ports/SkFontHost_android.cpp:495: static void map_fallback_scripts(const char fileName[], const uint32_t uniqueID) { I would check with russellbrenner who has done a lot of work on the Android browser that involves this code. He may have a suggestion for how to store and pull this information from the same place that we pull the location of the fonts. On 2012/03/12 12:27:50, reed1 wrote: > 1. Can this be written as a table instead. Might make it easier to understand > and modify in the future (or extend at runtime perhaps via xml). > > e.g. > > static const struct { > const char* fFileName; > FallbackScript fScript; > } gFallbackTable[] = { > { "DroidNaskh-Regular.ttf", SkTypeface::kArabic }, > { "DroidSansEthiopic-Regular.ttf", SkTypeface::kEthiopic }, > ... > }; > > Now the code just spins in a loop, returning the script associated with the > matching name. > > 2. Are these filenames already stored somewhere else? Any chance we can reuse > those, so we don't have to manually keep the two lists in sync? >
Sign in to reply to this message.
PTAL https://codereview.appspot.com/5797066/diff/1/include/core/SkTypeface.h File include/core/SkTypeface.h (right): https://codereview.appspot.com/5797066/diff/1/include/core/SkTypeface.h#newco... include/core/SkTypeface.h:123: enum FallbackScript { On 2012/03/12 12:27:50, reed1 wrote: > Lets add a suffix to these enums, and make the enum name plural > (FallbackScripts) > > kArabic_FallbackScript, > kEthiopic_FallbackScript, > ... Done. https://codereview.appspot.com/5797066/diff/1/src/ports/SkFontHost_android.cpp File src/ports/SkFontHost_android.cpp (right): https://codereview.appspot.com/5797066/diff/1/src/ports/SkFontHost_android.cp... src/ports/SkFontHost_android.cpp:494: On 2012/03/12 12:27:50, reed1 wrote: > Skia never marks a POD parameter as const. Done. https://codereview.appspot.com/5797066/diff/1/src/ports/SkFontHost_android.cp... src/ports/SkFontHost_android.cpp:495: static void map_fallback_scripts(const char fileName[], const uint32_t uniqueID) { On 2012/03/12 12:27:50, reed1 wrote: > 1. Can this be written as a table instead. Might make it easier to understand > and modify in the future (or extend at runtime perhaps via xml). > > e.g. > > static const struct { > const char* fFileName; > FallbackScript fScript; > } gFallbackTable[] = { > { "DroidNaskh-Regular.ttf", SkTypeface::kArabic }, > { "DroidSansEthiopic-Regular.ttf", SkTypeface::kEthiopic }, > ... > }; > > Now the code just spins in a loop, returning the script associated with the > matching name. Good idea. Done. > > 2. Are these filenames already stored somewhere else? Any chance we can reuse > those, so we don't have to manually keep the two lists in sync? > Filenames is stored in /system/etc/fallback_fonts.xml. However, there is no mapping information, so we have to maintain the mapping here. <family> <fileset> <file>DroidSansThai.ttf</file> </fileset> </family> https://codereview.appspot.com/5797066/diff/1/src/ports/SkFontHost_android.cp... src/ports/SkFontHost_android.cpp:723: SkTypeface* SkFontHost::CreateFallbackTypefaceForScript( On 2012/03/12 12:27:50, reed1 wrote: > Do we need to call load_system_fonts() first, before we look at > gFallbackScriptsMap? Will it always be initialized? Good catch. Done.
Sign in to reply to this message.
Added 2 helper function to convert between FallbackScripts enum and font file name. These are useful for WebKit's FontCache, which needs string as key. Please look patch set 3.
Sign in to reply to this message.
lgtm perhaps in the future, we can extend the existing xml file to include an optional field indicated that a particular font-file is the "default" for a given script...
Sign in to reply to this message.
On 2012/03/13 13:48:26, reed1 wrote: > lgtm > > perhaps in the future, we can extend the existing xml file to include an > optional field indicated that a particular font-file is the "default" for a > given script... Thanks for reviewing. Can I submit the change myself? I use 'gcl commit'. It prompt me to enter realm, but I don't know which account to use. Authentication realm: <https://skia.googlecode.com:443> Google Code Subversion Repository zhenghao@chromium.org Password for 'zhenghao@chromium.org':
Sign in to reply to this message.
https://codereview.appspot.com/5797066/diff/1007/src/ports/SkFontHost_android... File src/ports/SkFontHost_android.cpp (right): https://codereview.appspot.com/5797066/diff/1007/src/ports/SkFontHost_android... src/ports/SkFontHost_android.cpp:497: memset(gFallbackScriptsMap, 0, sizeof(gFallbackScriptsMap)); Sorry, sizeof(gFallbackScriptsMap) is 4 here... Change to use calloc in change set 4.
Sign in to reply to this message.
https://codereview.appspot.com/5797066/diff/5003/src/ports/SkFontHost_android... File src/ports/SkFontHost_android.cpp (right): https://codereview.appspot.com/5797066/diff/5003/src/ports/SkFontHost_android... src/ports/SkFontHost_android.cpp:433: }; I'm reluctant to put a hard-coded list of font files back into the source code. The switch to an xml-based configuration was, in large part, intended to reduce the need to recompile Skia when new fonts were added to the platform. I'm in agreement with reed on moving to xml, but would rather see that done at the outset.
Sign in to reply to this message.
On 2012/03/13 22:17:48, russell wrote: > https://codereview.appspot.com/5797066/diff/5003/src/ports/SkFontHost_android... > File src/ports/SkFontHost_android.cpp (right): > > https://codereview.appspot.com/5797066/diff/5003/src/ports/SkFontHost_android... > src/ports/SkFontHost_android.cpp:433: }; > I'm reluctant to put a hard-coded list of font files back into the source code. > The switch to an xml-based configuration was, in large part, intended to reduce > the need to recompile Skia when new fonts were added to the platform. > > I'm in agreement with reed on moving to xml, but would rather see that done at > the outset. I agree hardcoded list is not good. But I think we need this short term solution, as pushing Android to add some 'script description' to font_fallback.xml is a big deal. And we have to wait for next Android update to get the change. And I think Android system also use HarfBuzz to do CTL, right? How do they map font file to script? I guess if they don't code it in xml, they also have to hard code the mapping somewhere? Anyway, if you think it's worthy, I can file a bug for Android.
Sign in to reply to this message.
I already have a bug (b/5956665) to address a problem similar to this for fonts. The big issue is that Skia has Android users who want to use Skia across different versions of Android but right now we only support the latest version. Here is my suggestion... (1) let me fix the issue that Skia does not support looking up fonts anything other than the current Android release. (2) I'll extend that solution to allow clients (e.g. Chrome) to provide an optional config header that stores the mapping between fonts and the enum. Then, if the client doesn't provide the header then the functions will just return null. In the meantime I suggest we let this CL in and I'll implement both suggestions 1-2 above before we merge this code into Android. Russell, is that ok with you? https://codereview.appspot.com/5797066/diff/5003/src/ports/SkFontHost_android... File src/ports/SkFontHost_android.cpp (right): https://codereview.appspot.com/5797066/diff/5003/src/ports/SkFontHost_android... src/ports/SkFontHost_android.cpp:433: }; From the sound of it we will always need something at least semi-hardcoded as we need to match the enum constants with a particular font On 2012/03/13 22:17:48, russell wrote: > I'm reluctant to put a hard-coded list of font files back into the source code. > The switch to an xml-based configuration was, in large part, intended to reduce > the need to recompile Skia when new fonts were added to the platform. > > I'm in agreement with reed on moving to xml, but would rather see that done at > the outset.
Sign in to reply to this message.
Would the following API additions be sufficient for clank's needs? 1. Iterator, which returns each SkTypeface associated with the fallback chain 2. Typeface API which returns true if a given Unichar is available in a given Typeface (and only this typeface, no fallbacks considered). It seems (to me) that with these two, clank could, at startup, construct its own table of which fonts to use for each script that it cares about...
Sign in to reply to this message.
https://codereview.appspot.com/5797066/diff/5003/src/ports/SkFontHost_android... File src/ports/SkFontHost_android.cpp (right): https://codereview.appspot.com/5797066/diff/5003/src/ports/SkFontHost_android... src/ports/SkFontHost_android.cpp:433: }; Instead of an enum, we could use something like a 4-byte tag with language/region pairs. <family locale="jaJP"> It's still necessary for Skia to have some identifying mark for scripts so that callers can know which script is which, but this would remove the need for Skia to be hardcoded with any of the font filenames, which is a maintenance headache. On 2012/03/14 13:00:10, djsollen wrote: > From the sound of it we will always need something at least semi-hardcoded as we > need to match the enum constants with a particular font > > On 2012/03/13 22:17:48, russell wrote: > > I'm reluctant to put a hard-coded list of font files back into the source > code. > > The switch to an xml-based configuration was, in large part, intended to > reduce > > the need to recompile Skia when new fonts were added to the platform. > > > > I'm in agreement with reed on moving to xml, but would rather see that done at > > the outset. >
Sign in to reply to this message.
I think that these additions would allow webkit to stop mapping scripts to a hardcoded list of font files in order to properly setup Harfbuzz. I'm all in favor! On 2012/03/14 16:20:50, reed1 wrote: > Would the following API additions be sufficient for clank's needs? > > 1. Iterator, which returns each SkTypeface associated with the fallback chain > 2. Typeface API which returns true if a given Unichar is available in a given > Typeface (and only this typeface, no fallbacks considered). > > It seems (to me) that with these two, clank could, at startup, construct its own > table of which fonts to use for each script that it cares about...
Sign in to reply to this message.
On 2012/03/14 16:20:50, reed1 wrote: > Would the following API additions be sufficient for clank's needs? > > 1. Iterator, which returns each SkTypeface associated with the fallback chain > 2. Typeface API which returns true if a given Unichar is available in a given > Typeface (and only this typeface, no fallbacks considered). > > It seems (to me) that with these two, clank could, at startup, construct its own > table of which fonts to use for each script that it cares about... Sounds like a solution, although I haven't considered it clearly how to integrate with WebKit. I have an idea following yours. We can construct the script to file mapping by detecting some typical Unichar for each script. Then chrome can still get SkTypeface by script enum. This seems to me more reasonable, because we can hide all low level detain in Skia.
Sign in to reply to this message.
On 2012/03/15 04:29:40, zhenghao wrote: > On 2012/03/14 16:20:50, reed1 wrote: > > Would the following API additions be sufficient for clank's needs? > > > > 1. Iterator, which returns each SkTypeface associated with the fallback chain > > 2. Typeface API which returns true if a given Unichar is available in a given > > Typeface (and only this typeface, no fallbacks considered). > > > > It seems (to me) that with these two, clank could, at startup, construct its > own > > table of which fonts to use for each script that it cares about... > > Sounds like a solution, although I haven't considered it clearly how to > integrate with WebKit. I have an idea following yours. We can construct the > script to file mapping by detecting some typical Unichar for each script. Then > chrome can still get SkTypeface by script enum. This seems to me more > reasonable, because we can hide all low level detain in Skia. Sorry for typo. low level detail
Sign in to reply to this message.
Indeed. My proposal is that webkit is then free to create whatever mapping structure it wants, once skia will identify each typeface by the unichars it supports.
Sign in to reply to this message.
On 2012/03/15 12:09:00, reed1 wrote: > Indeed. My proposal is that webkit is then free to create whatever mapping > structure it wants, once skia will identify each typeface by the unichars it > supports. Sorry that I'm not making myself clear. I mean we can still keep the mapping structure in Skia, but when creating the mapping structure, we can use your proposed technique. Then we no longer need to hard code the file names in SkFontHost_android.cpp. The Skia api in SkTypeface_android.h still remains now, i.e. taking an enum as an argument. I think keeping this low level detail in Skia is more reasonable than moving the mapping into WebKit.
Sign in to reply to this message.
After talking with Mike I think he just wanted to have his proposed API made available to all Skia ports and is fine if the enum and mapping table in an android specific file within the Skia tree. On Thu, Mar 15, 2012 at 8:15 AM, <zhenghao@chromium.org> wrote: > On 2012/03/15 12:09:00, reed1 wrote: > >> Indeed. My proposal is that webkit is then free to create whatever >> > mapping > >> structure it wants, once skia will identify each typeface by the >> > unichars it > >> supports. >> > > Sorry that I'm not making myself clear. I mean we can still keep the > mapping structure in Skia, but when creating the mapping structure, we > can use your proposed technique. Then we no longer need to hard code the > file names in SkFontHost_android.cpp. The Skia api in > SkTypeface_android.h still remains now, i.e. taking an enum as an > argument. I think keeping this low level detail in Skia is more > reasonable than moving the mapping into WebKit. > > https://codereview.appspot.**com/5797066/<https://codereview.appspot.com/5797... >
Sign in to reply to this message.
On 2012/03/15 14:05:32, djsollen wrote: > After talking with Mike I think he just wanted to have his proposed API > made available to all Skia ports and is fine if the enum and mapping table > in an android specific file within the Skia tree. > Ah, that sounds great to me! Thanks for both of you! In that case, can you help me submit this change, as it seems I cannot submit myself...
Sign in to reply to this message.
Mike is working on the new API now and I'll be glad to change the internal implementation to use that new function instead of hardcoding the files. However before submitting this CL I want to make sure the SK_API's are correct so that we don't break your code when we change the implementation. Additionally, do you have a list of character codes that are representative of each fallback script class that we can use with Mike's new API? https://codereview.appspot.com/5797066/diff/5003/include/ports/SkTypeface_and... File include/ports/SkTypeface_android.h (right): https://codereview.appspot.com/5797066/diff/5003/include/ports/SkTypeface_and... include/ports/SkTypeface_android.h:46: SK_API const char* SkGetFileNameFromScript(FallbackScripts script); using the new API we won't have access to the filename just the typeface object. So we won't be able to support this API anymore. We could have Skia maintain a mapping to a unique fallback script ID, but not the filename. If you need this function in Skia I would rename it to SkGetFallbackScriptID(...). Then update the comment to state that we just promise to return a unique string ID. https://codereview.appspot.com/5797066/diff/5003/include/ports/SkTypeface_and... include/ports/SkTypeface_android.h:53: SK_API FallbackScripts SkGetScriptFromFileName(const char* fileName); we will be unable to support this API for the same reason as the one above.
Sign in to reply to this message.
Sorry, I don't have a list of character codes to detect the script. I learned to use fontmatrix to view font files this time. You can use it to open each Android fallback font file, and pick some unique unichars. For example, uni0E01 for DroidSansThai.ttf. https://codereview.appspot.com/5797066/diff/5003/include/ports/SkTypeface_and... File include/ports/SkTypeface_android.h (right): https://codereview.appspot.com/5797066/diff/5003/include/ports/SkTypeface_and... include/ports/SkTypeface_android.h:46: SK_API const char* SkGetFileNameFromScript(FallbackScripts script); On 2012/03/15 14:58:22, djsollen wrote: > using the new API we won't have access to the filename just the typeface object. > So we won't be able to support this API anymore. > > We could have Skia maintain a mapping to a unique fallback script ID, but not > the filename. If you need this function in Skia I would rename it to > SkGetFallbackScriptID(...). Then update the comment to state that we just > promise to return a unique string ID. Got it. Can you return some differentiable string like 'FallbackScript_Arabic' or something like that? It should be unlikely used by other font family names. That is, I need some string representation of the enum to be used as the key of WebKit's FontCache. https://codereview.appspot.com/5797066/diff/5003/include/ports/SkTypeface_and... include/ports/SkTypeface_android.h:53: SK_API FallbackScripts SkGetScriptFromFileName(const char* fileName); On 2012/03/15 14:58:22, djsollen wrote: > we will be unable to support this API for the same reason as the one above. Same here. I need to convert back to enum from the string representation.
Sign in to reply to this message.
I can make sure the string is unique when I rework the implementation. I can also submit the CL if you can update the API names on your side first. On Thu, Mar 15, 2012 at 11:09 AM, <zhenghao@chromium.org> wrote: > Sorry, I don't have a list of character codes to detect the script. I > learned to use fontmatrix to view font files this time. You can use it > to open each Android fallback font file, and pick some unique unichars. > For example, uni0E01 for DroidSansThai.ttf. > > > > https://codereview.appspot.**com/5797066/diff/5003/include/** > ports/SkTypeface_android.h<https://codereview.appspot.com/5797066/diff/5003/include/ports/SkTypeface_android.h> > File include/ports/SkTypeface_**android.h (right): > > https://codereview.appspot.**com/5797066/diff/5003/include/** > ports/SkTypeface_android.h#**newcode46<https://codereview.appspot.com/5797066/diff/5003/include/ports/SkTypeface_android.h#newcode46> > include/ports/SkTypeface_**android.h:46: SK_API const char* > SkGetFileNameFromScript(**FallbackScripts script); > On 2012/03/15 14:58:22, djsollen wrote: > >> using the new API we won't have access to the filename just the >> > typeface object. > >> So we won't be able to support this API anymore. >> > > We could have Skia maintain a mapping to a unique fallback script ID, >> > but not > >> the filename. If you need this function in Skia I would rename it to >> SkGetFallbackScriptID(...). Then update the comment to state that we >> > just > >> promise to return a unique string ID. >> > > Got it. Can you return some differentiable string like > 'FallbackScript_Arabic' or something like that? It should be unlikely > used by other font family names. That is, I need some string > representation of the enum to be used as the key of WebKit's FontCache. > > > https://codereview.appspot.**com/5797066/diff/5003/include/** > ports/SkTypeface_android.h#**newcode53<https://codereview.appspot.com/5797066/diff/5003/include/ports/SkTypeface_android.h#newcode53> > include/ports/SkTypeface_**android.h:53: SK_API FallbackScripts > SkGetScriptFromFileName(const char* fileName); > On 2012/03/15 14:58:22, djsollen wrote: > >> we will be unable to support this API for the same reason as the one >> > above. > > Same here. I need to convert back to enum from the string > representation. > > https://codereview.appspot.**com/5797066/<https://codereview.appspot.com/5797... >
Sign in to reply to this message.
Done. I removed some constraints on return value of SkGetFallbackScriptID, because I'm not sure how you would implement it. I guess if we are probing the scripts, we will no longer maintain invalid script mappings. I found Android has different font for NexusS and Xoom. Feel free to change the comment in your implementation.
Sign in to reply to this message.
On 2012/03/15 15:25:17, zhenghao wrote: > Done. I removed some constraints on return value of SkGetFallbackScriptID, > because I'm not sure how you would implement it. I guess if we are probing the > scripts, we will no longer maintain invalid script mappings. I found Android has > different font for NexusS and Xoom. Feel free to change the comment in your > implementation. Committed revision 3403. Please go ahead and close this issue. Thanks!
Sign in to reply to this message.
On 2012/03/15 15:50:23, djsollen wrote: > On 2012/03/15 15:25:17, zhenghao wrote: > > Done. I removed some constraints on return value of SkGetFallbackScriptID, > > because I'm not sure how you would implement it. I guess if we are probing the > > scripts, we will no longer maintain invalid script mappings. I found Android > has > > different font for NexusS and Xoom. Feel free to change the comment in your > > implementation. > > Committed revision 3403. > > Please go ahead and close this issue. Thanks! Thanks, Derek. I got a new idea of detecting the script from font files. Font files have glyphs block, and we can use the block name to detect the script. For example, DroidSansThai.ttf has 4 blocks in fontmatrix: 'Basic Latin', 'Thai', 'General Punctuation', 'Geometric Shapes'.
Sign in to reply to this message.
I like this new idea, a lot. After a quick scan of font files using FontForge, it looks like all of the scripts can be determined from their block signatures. On 2012/03/16 04:01:44, zhenghao wrote: > Thanks, Derek. I got a new idea of detecting the script from font files. Font > files have glyphs block, and we can use the block name to detect the script. For > example, DroidSansThai.ttf has 4 blocks in fontmatrix: 'Basic Latin', 'Thai', > 'General Punctuation', 'Geometric Shapes'.
Sign in to reply to this message.
|
