Issue 6197074: Update the implementation of SkTypeface_android.h functions. (Closed)
DescriptionUpdate the implementation of SkTypeface_android.h functions.
This CL removes the hardcoded mappings of FallbackScripts to
a particular font file and replaces it with a dynamic mechanism
that takes a style and representative character code for a given
FallbackScript class and returns the most appropriate SkTypeface
that contains that character code.
Committed: https://code.google.com/p/skia/source/detail?r=4111
Patch Set 1 #Patch Set 2 : #
Total comments: 4
Patch Set 3 : #
Total comments: 8
Patch Set 4 : #
Total comments: 5
Patch Set 5 : #Patch Set 6 : #
Total comments: 12
Patch Set 7 : #
Total comments: 1
Patch Set 8 : #
Total comments: 1
MessagesTotal messages: 21
FYI...as soon as this has been reviewed I'll start the effort to move this code into the Android repo.
Sign in to reply to this message.
I wonder if you can write a single "safe" function to encapsulate
1. the need to have called load_system_fonts
2. mutex safety
e.g.
SkTypeface* FindFromFontID(SkFontID fontID) {
SkMutexAcquire ...;
load_system_fonts();
return find_from_unique(fontID);
}
and then just call this where needed, so you don't have to explain why you're
doing that dance of grabbing the mutex for narrow scoped calls in several
places...
http://codereview.appspot.com/6197074/diff/1005/include/core/SkScalerContext.h
File include/core/SkScalerContext.h (right):
http://codereview.appspot.com/6197074/diff/1005/include/core/SkScalerContext....
include/core/SkScalerContext.h:312: // should not be called by other callers
"generate" is probably not the right prefix, since that (mostly) means "to be
implemented by subclass". How about "private" or "privateForAndroid" to really
drive home the message?
Hell, do we want to put the #ifdef ANDROID around this?
http://codereview.appspot.com/6197074/diff/1005/src/ports/SkFontHost_android.cpp
File src/ports/SkFontHost_android.cpp (right):
http://codereview.appspot.com/6197074/diff/1005/src/ports/SkFontHost_android....
src/ports/SkFontHost_android.cpp:1029: // we ref(), since the semantic is to
return a new instance
Do we need to perform the ref() inside the mutex?
Sign in to reply to this message.
I did the explicit locking as it will make integration with the android repo's fork of this file. However, I do plan to merge these two, but after the current release is done. http://codereview.appspot.com/6197074/diff/1005/include/core/SkScalerContext.h File include/core/SkScalerContext.h (right): http://codereview.appspot.com/6197074/diff/1005/include/core/SkScalerContext.... include/core/SkScalerContext.h:312: // should not be called by other callers Done. I've put an ifdef around it and modified the name. On 2012/05/11 16:01:27, reed1 wrote: > "generate" is probably not the right prefix, since that (mostly) means "to be > implemented by subclass". How about "private" or "privateForAndroid" to really > drive home the message? > > Hell, do we want to put the #ifdef ANDROID around this? http://codereview.appspot.com/6197074/diff/1005/src/ports/SkFontHost_android.cpp File src/ports/SkFontHost_android.cpp (right): http://codereview.appspot.com/6197074/diff/1005/src/ports/SkFontHost_android.... src/ports/SkFontHost_android.cpp:1029: // we ref(), since the semantic is to return a new instance Probably, in any case it isn't a bad idea. On 2012/05/11 16:01:27, reed1 wrote: > Do we need to perform the ref() inside the mutex?
Sign in to reply to this message.
Derek, thanks for doing this. This LGTM except some nits. http://codereview.appspot.com/6197074/diff/6001/src/ports/SkFontHost_android.cpp File src/ports/SkFontHost_android.cpp (right): http://codereview.appspot.com/6197074/diff/6001/src/ports/SkFontHost_android.... src/ports/SkFontHost_android.cpp:961: Nit: Empty line? http://codereview.appspot.com/6197074/diff/6001/src/ports/SkFontHost_android.... src/ports/SkFontHost_android.cpp:1048: return kFallbackScriptNumber; // Use SkTypeface_ValidScript as an invalid value. Nit: This looks like my mistake in the previous change. Should be 'Use kFallbackScriptNumber as an invalid value.'.
Sign in to reply to this message.
http://codereview.appspot.com/6197074/diff/6001/src/ports/SkFontHost_android.cpp File src/ports/SkFontHost_android.cpp (left): http://codereview.appspot.com/6197074/diff/6001/src/ports/SkFontHost_android.... src/ports/SkFontHost_android.cpp:622: } Can we still do initialization for each fallback font here, instead of calling initFBScriptInfo after init_system_fonts are done? http://codereview.appspot.com/6197074/diff/6001/src/ports/SkFontHost_android.cpp File src/ports/SkFontHost_android.cpp (right): http://codereview.appspot.com/6197074/diff/6001/src/ports/SkFontHost_android.... src/ports/SkFontHost_android.cpp:977: SkScalerContext* ctx = cache->getScalerContext(); I wonder if we can use local information to traverse this fallback list, which can save the change to other two public files? See above.
Sign in to reply to this message.
http://codereview.appspot.com/6197074/diff/6001/src/ports/SkFontHost_android.cpp File src/ports/SkFontHost_android.cpp (left): http://codereview.appspot.com/6197074/diff/6001/src/ports/SkFontHost_android.... src/ports/SkFontHost_android.cpp:622: } It would be more work to populate it here as each time you would have to traverse all the scripts that had an unpopulated FontID and look to see if this typeface has the representative character. Additionally, it shouldn't happen in this function as the fallback order could be changed (e.g. based on locale) and we wouldn't pick up the new ordering. On 2012/05/14 03:31:32, zhenghao1 wrote: > Can we still do initialization for each fallback font here, instead of calling > initFBScriptInfo after init_system_fonts are done? http://codereview.appspot.com/6197074/diff/6001/src/ports/SkFontHost_android.cpp File src/ports/SkFontHost_android.cpp (right): http://codereview.appspot.com/6197074/diff/6001/src/ports/SkFontHost_android.... src/ports/SkFontHost_android.cpp:961: On 2012/05/14 03:23:09, zhenghao1 wrote: > Nit: Empty line? Done. http://codereview.appspot.com/6197074/diff/6001/src/ports/SkFontHost_android.... src/ports/SkFontHost_android.cpp:977: SkScalerContext* ctx = cache->getScalerContext(); On 2012/05/14 03:31:32, zhenghao1 wrote: > I wonder if we can use local information to traverse this fallback list, which > can save the change to other two public files? See above. Without duplicating a lot of the code in those other files there is no way that I'm aware of to do this lookup efficiently here without the other classes. These lookups aren't expensive and we are only doing them 1x. This also benefits consumers of Skia who don't use these functions as they don't pay the cost of doing the lookups. http://codereview.appspot.com/6197074/diff/6001/src/ports/SkFontHost_android.... src/ports/SkFontHost_android.cpp:1048: return kFallbackScriptNumber; // Use SkTypeface_ValidScript as an invalid value. On 2012/05/14 03:23:09, zhenghao1 wrote: > Nit: This looks like my mistake in the previous change. Should be 'Use > kFallbackScriptNumber as an invalid value.'. Done.
Sign in to reply to this message.
http://codereview.appspot.com/6197074/diff/11001/include/ports/SkTypeface_and... File include/ports/SkTypeface_android.h (right): http://codereview.appspot.com/6197074/diff/11001/include/ports/SkTypeface_and... include/ports/SkTypeface_android.h:25: kTamilBold_FallbackScript, One more thing: if TamilBold is not present (ICS case), we should use TamilRegular instead.
Sign in to reply to this message.
http://codereview.appspot.com/6197074/diff/11001/include/ports/SkTypeface_and... File include/ports/SkTypeface_android.h (right): http://codereview.appspot.com/6197074/diff/11001/include/ports/SkTypeface_and... include/ports/SkTypeface_android.h:25: kTamilBold_FallbackScript, I think the caller should make that decision. If they ask for TamilBold and get a NULL response then they can ask for TamilRegular. I don't think that should be hardcoded into the implementation. On 2012/05/15 12:34:59, zhenghao1 wrote: > One more thing: if TamilBold is not present (ICS case), we should use > TamilRegular instead.
Sign in to reply to this message.
On 2012/05/15 13:13:33, djsollen wrote: > http://codereview.appspot.com/6197074/diff/11001/include/ports/SkTypeface_and... > File include/ports/SkTypeface_android.h (right): > > http://codereview.appspot.com/6197074/diff/11001/include/ports/SkTypeface_and... > include/ports/SkTypeface_android.h:25: kTamilBold_FallbackScript, > I think the caller should make that decision. If they ask for TamilBold and get > a NULL response then they can ask for TamilRegular. I don't think that should > be hardcoded into the implementation. > > On 2012/05/15 12:34:59, zhenghao1 wrote: > > One more thing: if TamilBold is not present (ICS case), we should use > > TamilRegular instead. OK, makes sense.
Sign in to reply to this message.
http://codereview.appspot.com/6197074/diff/11001/src/ports/SkFontHost_android... File src/ports/SkFontHost_android.cpp (right): http://codereview.appspot.com/6197074/diff/11001/src/ports/SkFontHost_android... src/ports/SkFontHost_android.cpp:992: gFamilyHeadAndNameListMutex.acquire(); There seems a deadlock here. When called into here from load_system_fonts(), gFamilyHeadAndNameListMutex is already acquired by the caller of load_system_fonts(), like CreateTypeface(). So reed's first comment is correct.
Sign in to reply to this message.
http://codereview.appspot.com/6197074/diff/11001/src/ports/SkFontHost_android... File src/ports/SkFontHost_android.cpp (right): http://codereview.appspot.com/6197074/diff/11001/src/ports/SkFontHost_android... src/ports/SkFontHost_android.cpp:962: gFamilyHeadAndNameListMutex.acquire(); Same. Deadlock here. http://codereview.appspot.com/6197074/diff/11001/src/ports/SkFontHost_android... src/ports/SkFontHost_android.cpp:973: SkAutoGlyphCache autoCache(paint, NULL); I tested the code in chromium for android. The code never returns from SkAutoGlyphCache constructor, after the deadlocks above and below are commented out. Looks like paint.detachCache -> descriptorProc never returns, however I don't look deeper. I think involving such complicated behavior in initialization is error prone. I still think as all fallback info can be obtained internally here, there is no need to involve SkScalarContext and SkGlyphCache, which is a circular dependency here. Generally, there are 2 methods of populating gFBScriptInfo: 1. when getting a fallback font, try loop through all representative chars, and find the correct FB script for it. This is what I was doing before by file name matching. I think this is also applicable to your approach. 2. when getting a fallback script, try loop through all fallback fonts, and find the correct font by one representative char of the script. This is what you are doing here. If you think this way is better, you can store all fallback fonts in a temp array when loading, and loop through it, instead of using external things like SkScalarContext.
Sign in to reply to this message.
sorry about that. I accidentally updated the CL with one of my experiments
that I shouldn't have. To fix this you can just move...
// ensure the system fonts are loaded
gFamilyHeadAndNameListMutex.acquire();
load_system_fonts();
gFamilyHeadAndNameListMutex.release();
from initFBScriptInfo into SkCreateTypefaceForScript. I was trying to
figure out how to only call the load_system_fonts when we were doing the
table population to save that extra mutex grab, but that isn't the way to
do it.
~ Derek
On Tue, May 15, 2012 at 10:02 AM, <zhenghao@google.com> wrote:
>
> http://codereview.appspot.com/**6197074/diff/11001/src/ports/**
>
SkFontHost_android.cpp<http://codereview.appspot.com/6197074/diff/11001/src/ports/SkFontHost_android.cpp>
> File src/ports/SkFontHost_android.**cpp (right):
>
> http://codereview.appspot.com/**6197074/diff/11001/src/ports/**
>
SkFontHost_android.cpp#**newcode962<http://codereview.appspot.com/6197074/diff/11001/src/ports/SkFontHost_android.cpp#newcode962>
> src/ports/SkFontHost_android.**cpp:962:
> gFamilyHeadAndNameListMutex.**acquire();
> Same. Deadlock here.
>
> http://codereview.appspot.com/**6197074/diff/11001/src/ports/**
>
SkFontHost_android.cpp#**newcode973<http://codereview.appspot.com/6197074/diff/11001/src/ports/SkFontHost_android.cpp#newcode973>
> src/ports/SkFontHost_android.**cpp:973: SkAutoGlyphCache autoCache(paint,
> NULL);
> I tested the code in chromium for android. The code never returns from
> SkAutoGlyphCache constructor, after the deadlocks above and below are
> commented out. Looks like paint.detachCache -> descriptorProc never
> returns, however I don't look deeper.
>
> I think involving such complicated behavior in initialization is error
> prone. I still think as all fallback info can be obtained internally
> here, there is no need to involve SkScalarContext and SkGlyphCache,
> which is a circular dependency here.
>
> Generally, there are 2 methods of populating gFBScriptInfo:
>
> 1. when getting a fallback font, try loop through all representative
> chars, and find the correct FB script for it. This is what I was doing
> before by file name matching. I think this is also applicable to your
> approach.
>
> 2. when getting a fallback script, try loop through all fallback fonts,
> and find the correct font by one representative char of the script. This
> is what you are doing here. If you think this way is better, you can
> store all fallback fonts in a temp array when loading, and loop through
> it, instead of using external things like SkScalarContext.
>
>
http://codereview.appspot.com/**6197074/<http://codereview.appspot.com/6197074/>
>
Sign in to reply to this message.
On 2012/05/15 14:25:17, djsollen wrote: > sorry about that. I accidentally updated the CL with one of my experiments > that I shouldn't have. To fix this you can just move... > > // ensure the system fonts are loaded > gFamilyHeadAndNameListMutex.acquire(); > load_system_fonts(); > gFamilyHeadAndNameListMutex.release(); > > from initFBScriptInfo into SkCreateTypefaceForScript. I was trying to > figure out how to only call the load_system_fonts when we were doing the > table population to save that extra mutex grab, but that isn't the way to > do it. > > ~ Derek > > On Tue, May 15, 2012 at 10:02 AM, <mailto:zhenghao@google.com> wrote: > > > > > http://codereview.appspot.com/**6197074/diff/11001/src/ports/** > > > SkFontHost_android.cpp<http://codereview.appspot.com/6197074/diff/11001/src/ports/SkFontHost_android.cpp> > > File src/ports/SkFontHost_android.**cpp (right): > > > > http://codereview.appspot.com/**6197074/diff/11001/src/ports/** > > > SkFontHost_android.cpp#**newcode962<http://codereview.appspot.com/6197074/diff/11001/src/ports/SkFontHost_android.cpp#newcode962> > > src/ports/SkFontHost_android.**cpp:962: > > gFamilyHeadAndNameListMutex.**acquire(); > > Same. Deadlock here. > > > > http://codereview.appspot.com/**6197074/diff/11001/src/ports/** > > > SkFontHost_android.cpp#**newcode973<http://codereview.appspot.com/6197074/diff/11001/src/ports/SkFontHost_android.cpp#newcode973> > > src/ports/SkFontHost_android.**cpp:973: SkAutoGlyphCache autoCache(paint, > > NULL); > > I tested the code in chromium for android. The code never returns from > > SkAutoGlyphCache constructor, after the deadlocks above and below are > > commented out. Looks like paint.detachCache -> descriptorProc never > > returns, however I don't look deeper. > > > > I think involving such complicated behavior in initialization is error > > prone. I still think as all fallback info can be obtained internally > > here, there is no need to involve SkScalarContext and SkGlyphCache, > > which is a circular dependency here. > > > > Generally, there are 2 methods of populating gFBScriptInfo: > > > > 1. when getting a fallback font, try loop through all representative > > chars, and find the correct FB script for it. This is what I was doing > > before by file name matching. I think this is also applicable to your > > approach. > > > > 2. when getting a fallback script, try loop through all fallback fonts, > > and find the correct font by one representative char of the script. This > > is what you are doing here. If you think this way is better, you can > > store all fallback fonts in a temp array when loading, and loop through > > it, instead of using external things like SkScalarContext. > > > > > http://codereview.appspot.com/**6197074/%3Chttp://codereview.appspot.com/6197...> > > I just realized that that doesn't solve the issue either. I'll think about this for a little and update the CL when I have a better solution.
Sign in to reply to this message.
ping?
Sign in to reply to this message.
The CL was updated with a fix, but I forgot to post a comment to notify the reviewers. I've already chatted with Hao and he is going to test out the change tomorrow. On Mon, May 21, 2012 at 11:25 PM, <zhenghao@google.com> wrote: > ping? > > http://codereview.appspot.com/**6197074/<http://codereview.appspot.com/6197074/> >
Sign in to reply to this message.
The deadlock seems gone in my test. Some additional comments. http://codereview.appspot.com/6197074/diff/2006/include/ports/SkTypeface_andr... File include/ports/SkTypeface_android.h (right): http://codereview.appspot.com/6197074/diff/2006/include/ports/SkTypeface_andr... include/ports/SkTypeface_android.h:25: kTamilBold_FallbackScript, Please add another 3 fonts recently added into JB. See https://android-git.corp.google.com/g/#/c/188359/ http://codereview.appspot.com/6197074/diff/2006/src/ports/SkFontHost_android.cpp File src/ports/SkFontHost_android.cpp (right): http://codereview.appspot.com/6197074/diff/2006/src/ports/SkFontHost_android.... src/ports/SkFontHost_android.cpp:684: resetFBScriptInfo(); It looks to me that there is no need to reload FB Script info after reloading fallback bonts, which only change the order of fallback fonts. As long as font id doesn't change, no need to change gFBScriptInfo[]. http://codereview.appspot.com/6197074/diff/2006/src/ports/SkFontHost_android.... src/ports/SkFontHost_android.cpp:963: static void resetFBScriptInfo() { Same. Code style. http://codereview.appspot.com/6197074/diff/2006/src/ports/SkFontHost_android.... src/ports/SkFontHost_android.cpp:992: static void initFBScriptInfo() { I'm not quite familiar with code style in Skia. But this seems not compatible with other static method, e.g. init_system_fonts(). http://codereview.appspot.com/6197074/diff/2006/src/ports/SkFontHost_android.... src/ports/SkFontHost_android.cpp:1007: SkDebugf("No fallback was found for script type %s", scriptInfo.fScriptID); What's the difference between SkDebugf() and SkDEBUGF(). Shall we be consistent on these debug outputs?
Sign in to reply to this message.
I've uploaded a new patch that includes the additional fonts http://codereview.appspot.com/6197074/diff/2006/include/ports/SkTypeface_andr... File include/ports/SkTypeface_android.h (right): http://codereview.appspot.com/6197074/diff/2006/include/ports/SkTypeface_andr... include/ports/SkTypeface_android.h:25: kTamilBold_FallbackScript, On 2012/05/30 12:07:25, Hao Zheng wrote: > Please add another 3 fonts recently added into JB. See > https://android-git.corp.google.com/g/#/c/188359/ Done. http://codereview.appspot.com/6197074/diff/2006/src/ports/SkFontHost_android.cpp File src/ports/SkFontHost_android.cpp (right): http://codereview.appspot.com/6197074/diff/2006/src/ports/SkFontHost_android.... src/ports/SkFontHost_android.cpp:684: resetFBScriptInfo(); The use case I imagined when I added this is suppose two fonts supported the same character codes. If the new fallback chain switches the order of those two fonts then we would want to update our script to grab the most appropriate font file for that fallback list. On 2012/05/30 12:07:25, Hao Zheng wrote: > It looks to me that there is no need to reload FB Script info after reloading > fallback bonts, which only change the order of fallback fonts. As long as font > id doesn't change, no need to change gFBScriptInfo[]. http://codereview.appspot.com/6197074/diff/2006/src/ports/SkFontHost_android.... src/ports/SkFontHost_android.cpp:992: static void initFBScriptInfo() { The underscore naming convention is actually not standard in Skia and will be updated in an upcoming CL when I merge in changes from the android framework. On 2012/05/30 12:07:25, Hao Zheng wrote: > I'm not quite familiar with code style in Skia. But this seems not compatible > with other static method, e.g. init_system_fonts(). http://codereview.appspot.com/6197074/diff/2006/src/ports/SkFontHost_android.... src/ports/SkFontHost_android.cpp:1007: SkDebugf("No fallback was found for script type %s", scriptInfo.fScriptID); The one in all caps will only print when built in debug mode, the lower case one will always print a debug message regardless of the build type. On 2012/05/30 12:07:25, Hao Zheng wrote: > What's the difference between SkDebugf() and SkDEBUGF(). Shall we be consistent > on these debug outputs?
Sign in to reply to this message.
http://codereview.appspot.com/6197074/diff/2006/include/ports/SkTypeface_andr... File include/ports/SkTypeface_android.h (right): http://codereview.appspot.com/6197074/diff/2006/include/ports/SkTypeface_andr... include/ports/SkTypeface_android.h:25: kTamilBold_FallbackScript, Thanks for adding these. New scripts render perfectly! On 2012/05/30 14:02:07, DerekS wrote: > On 2012/05/30 12:07:25, Hao Zheng wrote: > > Please add another 3 fonts recently added into JB. See > > https://android-git.corp.google.com/g/#/c/188359/ > > Done. http://codereview.appspot.com/6197074/diff/2006/src/ports/SkFontHost_android.cpp File src/ports/SkFontHost_android.cpp (right): http://codereview.appspot.com/6197074/diff/2006/src/ports/SkFontHost_android.... src/ports/SkFontHost_android.cpp:684: resetFBScriptInfo(); Although the use case itself seems reasonable, I don't think this is very useful in the near future. Now only locale ja needs to reload fallback fonts, which is really a very special case as Japanese and Chinese glyphs have some overlap. I don't expect there would be any other locale needs to do this. So I think we can remove this logic completely. On 2012/05/30 14:02:07, DerekS wrote: > The use case I imagined when I added this is suppose two fonts supported the > same character codes. If the new fallback chain switches the order of those two > fonts then we would want to update our script to grab the most appropriate font > file for that fallback list. > > On 2012/05/30 12:07:25, Hao Zheng wrote: > > It looks to me that there is no need to reload FB Script info after reloading > > fallback bonts, which only change the order of fallback fonts. As long as font > > id doesn't change, no need to change gFBScriptInfo[]. > http://codereview.appspot.com/6197074/diff/2006/src/ports/SkFontHost_android.... src/ports/SkFontHost_android.cpp:1007: SkDebugf("No fallback was found for script type %s", scriptInfo.fScriptID); On 2012/05/30 14:02:07, DerekS wrote: > The one in all caps will only print when built in debug mode, the lower case one > will always print a debug message regardless of the build type. > > On 2012/05/30 12:07:25, Hao Zheng wrote: > > What's the difference between SkDebugf() and SkDEBUGF(). Shall we be > consistent > > on these debug outputs? > Removing these lines looks ok. Could we rename the lower case one to sth like SkWARNINGF or SkINFOF in the future? It's really confusing. http://codereview.appspot.com/6197074/diff/25002/src/ports/SkFontHost_android... File src/ports/SkFontHost_android.cpp (right): http://codereview.appspot.com/6197074/diff/25002/src/ports/SkFontHost_android... src/ports/SkFontHost_android.cpp:1008: scriptInfo.fFontID = findFontIDForChar(scriptInfo.fChar, scriptInfo.fStyle); Please add a comment above like 'If bold version is missing, normal version of font will be used.'
Sign in to reply to this message.
I've updated the comments and removed the reset function. I'll hold off until tomorrow morning to submit this to give you a chance to try it out.
Sign in to reply to this message.
lgtm Thanks! I have verified it works for me. http://codereview.appspot.com/6197074/diff/23004/src/ports/SkFontHost_android... File src/ports/SkFontHost_android.cpp (right): http://codereview.appspot.com/6197074/diff/23004/src/ports/SkFontHost_android... src/ports/SkFontHost_android.cpp:984: Nit: empty line.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
