Bug 80873

I thought I had a workaround for that issue when implementing the size specific design support. let me take a look.

Created attachment 102232 [details] [review]
proposed patch

the patch to just hide the range of the size to keep the same output.

I don't like NOT showing something that is set.  Why is it set anyway?

Given that a missing SIZE will match with score 0, and a SIZE of (0,inf) also always match with score 0, I really prefer that we DON'T set size if it's not available.

I did a couple commits to that effect.  Please see:

  https://github.com/behdad/fontconfig/commits/size-rework

Needs testing.  On top of this, I want to see us clean up some more:

  - Remove the bits about range's is_inclusive,

  - Remove special-cases of ranges with start==end,

  - Move the part that sets FC_SIZE for bitmap-only fonts close to where FC_PIXEL_SIZE is set, or remove it.  I don't think we had that part before and you introduced it during adding ranges.

Why did you decide to set FC_SIZE as a range all the time to begin with?  It's not required AFAIU.

Thanks!

(In reply to comment #3)
> I don't like NOT showing something that is set.  Why is it set anyway?
> 
> Given that a missing SIZE will match with score 0, and a SIZE of (0,inf)
> also always match with score 0, I really prefer that we DON'T set size if
> it's not available.
> 
> I did a couple commits to that effect.  Please see:
> 
>   https://github.com/behdad/fontconfig/commits/size-rework
> 
> Needs testing.  On top of this, I want to see us clean up some more:
> 
>   - Remove the bits about range's is_inclusive,
> 
>   - Remove special-cases of ranges with start==end,
> 
>   - Move the part that sets FC_SIZE for bitmap-only fonts close to where
> FC_PIXEL_SIZE is set, or remove it.  I don't think we had that part before
> and you introduced it during adding ranges.

One more:

  - Accept integer for size.  I think in some places we only accept range and double.

(In reply to comment #3)
> I don't like NOT showing something that is set.  Why is it set anyway?
> 
> Given that a missing SIZE will match with score 0,

This isn't correct. the default score is 1e99 and mismatch is 0. so if the certain value to evaluate is missing, that score is always 1e99 but the score for a font which has a value will be 0 on mismatch.
that is the reason why we have to have something for objects affecting scoring.

I don't have an env to have further comments ATM so may be back later but anyway.

(In reply to comment #5)
> (In reply to comment #3)
> > I don't like NOT showing something that is set.  Why is it set anyway?
> > 
> > Given that a missing SIZE will match with score 0,
> 
> This isn't correct. the default score is 1e99 and mismatch is 0. so if the
> certain value to evaluate is missing, that score is always 1e99 but the
> score for a font which has a value will be 0 on mismatch.
> that is the reason why we have to have something for objects affecting
> scoring.

Are you sure?!  My understanding is that if either the pattern or font doesn't an element, that counts as a perfect match, ie. score 0:

static FcBool
FcCompare (FcPattern    *pat,
           FcPattern    *fnt,
           double       *value,
           FcResult     *result)
{
    int             i, i1, i2;

    for (i = 0; i < PRI_END; i++)
        value[i] = 0.0;

(In reply to comment #6)
> Are you sure?!  My understanding is that if either the pattern or font
> doesn't an element, that counts as a perfect match, ie. score 0:

Yes, I am...:

static FcBool
FcCompareValueList (FcObject	     object,
		    const FcMatcher *match,
		    FcValueListPtr   v1orig,	/* pattern */
		    FcValueListPtr   v2orig,	/* target */
		    FcValue         *bestValue,
		    double          *value,
		    int             *n,
		    FcResult        *result)
{
...
    best = 1e99;
    bestStrong = 1e99;
    bestWeak = 1e99;
    j = 1;
    for (v1 = v1orig; v1; v1 = FcValueListNext(v1))
    {
	for (v2 = v2orig, k = 0; v2; v2 = FcValueListNext(v2), k++)
	{
	    v = (match->compare) (&v1->value, &v2->value);
	    if (v < 0)
	    {
		*result = FcResultTypeMismatch;
		return FcFalse;
	    }
	    v = v * 1000 + j;
	    if (v < best)
	    {
		if (bestValue)
		    *bestValue = FcValueCanonicalize(&v2->value);
		best = v;
		pos = k;
	    }
	    if (v1->binding == FcValueBindingStrong)
	    {
		if (v < bestStrong)
		    bestStrong = v;
	    }
	    else
	    {
		if (v < bestWeak)
		    bestWeak = v;
	    }
	}
	j++;
    }
...
}

The above code assumes the cache (at least and the pattern as optional) always contains a certain value for elements which has a matcher. i.e. FC_SIZE is expected to match on FcCompareSizeRange() if we don't set any value, the default becomes 1e99 then.
that was a pitfall for me when I broke the cache in development and saw weird behavior on match.

(In reply to comment #7)
> (In reply to comment #6)
> > Are you sure?!  My understanding is that if either the pattern or font
> > doesn't an element, that counts as a perfect match, ie. score 0:
> 
> Yes, I am...:
> 
> static FcBool
> FcCompareValueList (FcObject	     object,
> 		    const FcMatcher *match,
> 		    FcValueListPtr   v1orig,	/* pattern */
> 		    FcValueListPtr   v2orig,	/* target */
> 		    FcValue         *bestValue,
> 		    double          *value,
> 		    int             *n,
> 		    FcResult        *result)
> {
> ...
>     best = 1e99;
>     bestStrong = 1e99;
>     bestWeak = 1e99;
>     j = 1;
>     for (v1 = v1orig; v1; v1 = FcValueListNext(v1))
>     {
> 	for (v2 = v2orig, k = 0; v2; v2 = FcValueListNext(v2), k++)
> 	{
> 	    v = (match->compare) (&v1->value, &v2->value);
> 	    if (v < 0)
> 	    {
> 		*result = FcResultTypeMismatch;
> 		return FcFalse;
> 	    }
> 	    v = v * 1000 + j;
> 	    if (v < best)
> 	    {
> 		if (bestValue)
> 		    *bestValue = FcValueCanonicalize(&v2->value);
> 		best = v;
> 		pos = k;
> 	    }
> 	    if (v1->binding == FcValueBindingStrong)
> 	    {
> 		if (v < bestStrong)
> 		    bestStrong = v;
> 	    }
> 	    else
> 	    {
> 		if (v < bestWeak)
> 		    bestWeak = v;
> 	    }
> 	}
> 	j++;
>     }
> ...
> }
> 
> The above code assumes the cache (at least and the pattern as optional)
> always contains a certain value for elements which has a matcher. i.e.
> FC_SIZE is expected to match on FcCompareSizeRange() if we don't set any
> value, the default becomes 1e99 then.
> that was a pitfall for me when I broke the cache in development and saw
> weird behavior on match.

Really?  How about this: the FcCompareValueList is *only* called from FcCompare if both patterns have the element in question.  Please check.  I'm sure a missing item gets a score of zero and double-checked and triple-checked yesterday.

(In reply to comment #8)
> Really?  How about this: the FcCompareValueList is *only* called from
> FcCompare if both patterns have the element in question.  Please check.  I'm
> sure a missing item gets a score of zero and double-checked and
> triple-checked yesterday.

You're right. but:

(In reply to comment #6)
> Are you sure?!  My understanding is that if either the pattern or font
> doesn't an element, that counts as a perfect match, ie. score 0:

This is the problem. for example, one tries to find a font matching with :size=12 say, it won't matches to the 12-ish sized (bitmap) fonts, because the best matched score is always more than 1000. that's why we have certain values in the cache and the pattern which is being targeted by matchers. in that sense, I'm not sure if implementing Bug#80929 will gives us a corrrect result.

(In reply to comment #9)
> (In reply to comment #8)
> > Really?  How about this: the FcCompareValueList is *only* called from
> > FcCompare if both patterns have the element in question.  Please check.  I'm
> > sure a missing item gets a score of zero and double-checked and
> > triple-checked yesterday.
> 
> You're right. but:
> 
> (In reply to comment #6)
> > Are you sure?!  My understanding is that if either the pattern or font
> > doesn't an element, that counts as a perfect match, ie. score 0:
> 
> This is the problem. for example, one tries to find a font matching with
> :size=12 say, it won't matches to the 12-ish sized (bitmap) fonts, because
> the best matched score is always more than 1000. that's why we have certain
> values in the cache and the pattern which is being targeted by matchers. in
> that sense, I'm not sure if implementing Bug#80929 will gives us a corrrect
> result.

Ok, that's a fair and very good point, but that's independent of supporting ranges, right?  I wish you had brought it up with me before.

Looks like I broke that in 2008, and I can't justify why.  I think we should revert that change instead:

commit a5a384c5ffb479e095092c2aaedd406f8785280a
Author: Behdad Esfahbod <behdad@behdad.org>
Date:   Wed Dec 31 19:44:32 2008 -0500

    [fcmatch] When matching, reserve score 0 for when elements don't exist
    
    Previously an index j was added to element score to prefer matches earlier
    in the value list to the later ones.  This index started from 0, meaning
    that the score zero could be generated for the first element.  By starting
    j from one, scores for when the element exists in both pattern and font
    can never be zero.  The score zero is reserved for when the element is
    NOT available in both font and pattern.  We will use this property later.
    
    This shouldn't change matching much.  The only difference I can think of
    is that if a font family exists both as a bitmap font and a scalable
    version, and when requesting it at the size of the bitmap version,
    previously the font returned was nondeterministic.  Now the scalable
    version will always be preferred.

diff --git a/src/fcmatch.c b/src/fcmatch.c
index 4d20a61..49dd0dd 100644
--- a/src/fcmatch.c
+++ b/src/fcmatch.c
@@ -317,7 +317,7 @@ FcCompareValueList (FcObject         object,
     best = 1e99;
     bestStrong = 1e99;
     bestWeak = 1e99;
-    j = 0;
+    j = 1;
     for (v1 = v1orig; v1; v1 = FcValueListNext(v1))
     {
        for (v2 = v2orig; v2; v2 = FcValueListNext(v2))

I made some more progress.  I think I addressed issues listed in comment #3.  Will confirm, and check for comment #4 issues tomorrow.  All in the github branch.

I think this might be ready.  Akira, can you please review?

Thanks.

(In reply to comment #12)
> I think this might be ready.  Akira, can you please review?
> 
> Thanks.

Well, "it matches any value perfectly" approach ignores the score when it exactly matches the value perfectly, doesn't it?
j is increased per matchers. speaking of FC_SIZE, j will be 9 at this moment. given that one tries to query with :size=12 say, and a cache doesn't contain FC_SIZE, the score will be 0 right. but if it has, the score will be 9. in this case that works because an outlined font can takes care of it. how about the sort of flags? I tend to agree with not having the unnecessary values in the cache though, do we need to pay more attentions in FcDefaultSubstitute() to prevent such situation or improve the score estimation to make better even without it?

(In reply to Akira TAGOH from comment #13)
> (In reply to comment #12)
> > I think this might be ready.  Akira, can you please review?
> > 
> > Thanks.
> 
> Well, "it matches any value perfectly" approach ignores the score when it
> exactly matches the value perfectly, doesn't it?

The become the same, yes.

> j is increased per matchers. speaking of FC_SIZE, j will be 9 at this
> moment. given that one tries to query with :size=12 say, and a cache doesn't
> contain FC_SIZE, the score will be 0 right. but if it has, the score will be
> 9.

9 is really small.  See:

  v = v * 1000 + j;

so if there's any difference, that would be at least 1000.  I don't think that's a problem.


> in this case that works because an outlined font can takes care of it.
> how about the sort of flags? I tend to agree with not having the unnecessary
> values in the cache though, do we need to pay more attentions in
> FcDefaultSubstitute() to prevent such situation or improve the score
> estimation to make better even without it?

Do you have a clear example of that?

Lets try to land this soon.  Thanks.

(In reply to Behdad Esfahbod from comment #14)
> 9 is really small.  See:
> 
>   v = v * 1000 + j;
> 
> so if there's any difference, that would be at least 1000.  I don't think
> that's a problem.

Not exactly. when the value exactly matches to the pattern, v will be 0. so it won't be more than 1000. that is how fontconfig figures out the best font and to make difference with exact-match and fallback on scoring right.

> > in this case that works because an outlined font can takes care of it.
> > how about the sort of flags? I tend to agree with not having the unnecessary
> > values in the cache though, do we need to pay more attentions in
> > FcDefaultSubstitute() to prevent such situation or improve the score
> > estimation to make better even without it?
> 
> Do you have a clear example of that?

maybe not at this moment. assuming we are about to have some state in the cache that is keeping the available properties only there, just guessed it might be a problem then. it may depends on how the default behavior is. for example, that is "color". we always have that property in the cache at this moment though, if we add it only when color is true, querying a font with :color=true won't work as expected so that the score becomes higher when it doesn't have.

Another topic on your patch is, we may need to postpone to apply the change of FcRange structure. that requires bumping the cache version again so that it breaks the serialization of FcRange in the cache.

I rebased the branch on top of master and bumped cache version.  I don't think there's a problem with bumping it again.

Sure. well, I was planning to bump the cache version in next next stable release so just wonder if I should move up to merge the change into next release. that's it.

merged into master.

Thanks.