PlatformFontWin::DeriveFontWithHeight insensitive to base font's height

ui/gfx/platform_font_win.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/gfx/platform_font_win.h"
 
 #include <windows.h>
 #include <math.h>
 
 #include <algorithm>
@@ skipping 94 matching lines @@
       if (font_height == font.GetHeight() && font_size == font.GetFontSize())
         break;
       font_height = font.GetHeight();
       font_size = font.GetFontSize();
     }
     return font;
   }
 
   LOGFONT font_info;
   GetObject(GetNativeFont(), sizeof(LOGFONT), &font_info);
-  font_info.lfHeight = height;
   SetLogFontStyle(style, &font_info);
 
   HFONT hfont = CreateFontIndirect(&font_info);
-  return Font(new PlatformFontWin(CreateHFontRef(hfont)));
+  Font best_font(new PlatformFontWin(CreateHFontRef(hfont)));
+  int best_font_height = best_font.GetHeight();
+
+  while (best_font_height <= height) {
+    Font font = best_font.Derive(1, style);
+    int font_height = font.GetHeight();
+    if (font_height > height) {

[Alexei Svitkine (slow), 2014/04/25 15:56:14]

I'm not sure this if is needed, since the loop should naturally end due to
best_font_height <= height on next iteration.

So your current logic can just be simplified to:

Font best_font(new PlatformFontWin(CreateHFontRef(hfont)));
while (best_font.GetHeight() <= height) {
  best_font = best_font.Derive(1, style);
}

return best_font;

However, I need a bit more convincing that this is really what we want to do.
Ultimately, anything that hits this codepath (just like the loop above) will be
slow, since it will need to do multiple Derive() operations which are somewhat
expensive.

And it seems your change will force this codepath always since it actually seems
to be starting with the base font, rather than trying the CreateFontIndirect()
with the specified height for lfHeight.

(In fact, given that you're starting with the base font, your code can be
further simplified to not do the
GetObject()/SetLogFontStyle()/CreateFontIndirect()/CreateHFontRef()/etc calls
above and just init best_font with *this. However, as I say above, I'm not sure
it's the right solution.)

[Tomasz Moniuszko, 2014/05/08 14:08:17]

The reason for this fix is that GetTextMetrics returns the same font height
(calculated in PlatformFontWin::CreateHFontRef) for fonts of different sizes.
For instance:

Arial:
font size=12 height=15
font size=13 height=16
font size=14 height=16
font size=15 height=17

Segoe UI:
font size=17 height=23
font size=18 height=25
font size=19 height=25
font size=20 height=28

The result of PlatformFontWin::DeriveFontWithHeight call is dependent on base
font's height. For instance:

gfx::Font arial_12("Arial", 12);
gfx::Font arial_13 =
static_cast<PlatformFontWin*>(arial_12.platform_font())->DeriveFontWithHeight(16,
0);
gfx::Font arial_15("Arial", 15);
gfx::Font arial_14 =
static_cast<PlatformFontWin*>(arial_15.platform_font())->DeriveFontWithHeight(16,
0);

Both arial_13 and arial_14 fonts are the same height but they are different in
size so EXPECT_EQ(arial_13.GetHeight(), arial_14.GetHeight()); will pass but
EXPECT_EQ(arial_13.GetFontSize(), arial_14.GetFontSize()); will fail. This is
the reason why RenderTextTest.ElidedText failed in my other review:
https://codereview.chromium.org/230793003/

After applying this fix we will get Arial 14 font in both cases (if font is
derived from Arial 12 or Arial 15).

On 2014/04/25 15:56:14, Alexei Svitkine wrote:
With this logic returned font would be too big because it's changed inside the
loop. We need to return the biggest font which height isn't greater than
requested height.
Maybe I could create font using CreateFontIndirect (with lfHeight) first and
then iterate in loop to check if bigger font also fits requested height. And
return the biggest font finally. What do you think?

[Alexei Svitkine (slow), 2014/05/09 21:17:50]

I see, that makes sense since we iterate down in one case, but don't in the
other case.

Can you codify the above into a new unit test?
That sounds better than the current logic here. Could you also measure the
performance impact of the new code vs. the old one?

To get a good measurement, do it e.g. 100k times in a loop for both cases and
post the results. Thanks!

[Tomasz Moniuszko, 2014/05/29 10:46:40]

Done.

Test results for 100k repeats with different font heights (times in ms):
height=8 old_time=1155 new_time=3432
height=9 old_time=1154 new_time=3432
height=10 old_time=1155 new_time=3416
height=11 old_time=1139 new_time=3432
height=12 old_time=1185 new_time=3432
height=13 old_time=1139 new_time=3432
height=14 old_time=1139 new_time=3432
height=15 old_time=1155 new_time=3432
height=16 old_time=1170 new_time=3447
height=17 old_time=1155 new_time=3479
height=18 old_time=1154 new_time=3463
height=19 old_time=1155 new_time=3463
height=20 old_time=1170 new_time=3463
height=21 old_time=1155 new_time=3463
height=22 old_time=1154 new_time=3463
height=23 old_time=1155 new_time=3463

Average time is about 3 times greater. Some of this time is consumed to get
screen DC and create HFontRef in loop. This can be partially avoided by getting
TEXTMETRIC directly in DeriveFontWithHeight method and then creating HFontRef
only for the best font. The code is something like:

  LOGFONT font_info;
  GetObject(GetNativeFont(), sizeof(LOGFONT), &font_info);
  font_info.lfHeight = height;
  SetLogFontStyle(style, &font_info);

  base::win::ScopedGetDC screen_dc(NULL);
  gfx::ScopedSetMapMode mode(screen_dc, MM_TEXT);

  base::win::ScopedGDIObject<HFONT> best_font(CreateFontIndirect(&font_info));
  TEXTMETRIC best_font_metrics;
  {
    base::win::ScopedSelectObject scoped_font(screen_dc, best_font);
    GetTextMetrics(screen_dc, &best_font_metrics);
  }

  while (best_font_metrics.tmHeight <= height) {
    font_info.lfHeight =
        -(best_font_metrics.tmHeight - best_font_metrics.tmInternalLeading + 1);
    base::win::ScopedGDIObject<HFONT> font(CreateFontIndirect(&font_info));
    TEXTMETRIC font_metrics;
    {
      base::win::ScopedSelectObject scoped_font(screen_dc, font);
      GetTextMetrics(screen_dc, &font_metrics);
    }
    if (font_metrics.tmHeight > height) {
      break;
    }
    best_font.Set(font.release());
    best_font_metrics = font_metrics;
  }
 
  return Font(new PlatformFontWin(
      CreateHFontRef(best_font.release(), best_font_metrics)));

With these additional changes I got test times about 2570 ms on my computer. I'm
not sure if it's worth to change this logic though because DeriveFontWithHeight
method is currently used only from DeriveFontIfNecessary function and only on
Windows XP. Also CreateHFontRef has to be slightly modified. What do you think?

[Alexei Svitkine (slow), 2014/05/29 21:13:34]

Seems like a sizeable-enough win to do the optimized path (especially since XP
machines are usually slower so it's a big difference). However, instead of
blowing up the code like above, could we make an internal version of Derive()
that simply takes more parameters? (e.g. taking the screen_dc and whatever other
things we need to cache between runs).

+      break;
+    }
+    best_font = font;
+    best_font_height = font_height;
+  }
+
+  return best_font;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // PlatformFontWin, PlatformFont implementation:
 
 Font PlatformFontWin::DeriveFont(int size_delta, int style) const {
   LOGFONT font_info;
   GetObject(GetNativeFont(), sizeof(LOGFONT), &font_info);
   const int requested_font_size = font_ref_->requested_font_size();
   font_info.lfHeight = AdjustFontSize(-requested_font_size, size_delta);
@@ skipping 192 matching lines @@
   return new PlatformFontWin(native_font);
 }
 
 // static
 PlatformFont* PlatformFont::CreateFromNameAndSize(const std::string& font_name,
                                                   int font_size) {
   return new PlatformFontWin(font_name, font_size);
 }
 
 }  // namespace gfx