RenderTextHarfBuzz: Set font render parameters in font data functions

ui/gfx/render_text_harfbuzz.cc

 // Copyright 2014 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/render_text_harfbuzz.h"
 
 #include <map>
 
 #include "base/i18n/bidi_line_iterator.h"
 #include "base/i18n/break_iterator.h"
@@ skipping 250 matching lines @@
 
   hb_face_t* get() {
     return face_;
   }
 
  private:
   hb_face_t* face_;
 };
 
 // Creates a HarfBuzz font from the given Skia face and text size.
-hb_font_t* CreateHarfBuzzFont(SkTypeface* skia_face, int text_size) {
+hb_font_t* CreateHarfBuzzFont(SkTypeface* skia_face,
+                              int text_size,
+                              const FontRenderParams& params) {
   typedef std::pair<HarfBuzzFace, GlyphCache> FaceCache;
 
   // TODO(ckocagil): This shouldn't grow indefinitely. Maybe use base::MRUCache?
   static std::map<SkFontID, FaceCache> face_caches;
 
   FaceCache* face_cache = &face_caches[skia_face->uniqueID()];
   if (face_cache->first.get() == NULL)
     face_cache->first.Init(skia_face);
 
   hb_font_t* harfbuzz_font = hb_font_create(face_cache->first.get());
   const int scale = SkScalarToFixed(text_size);
   hb_font_set_scale(harfbuzz_font, scale, scale);
   FontData* hb_font_data = new FontData(&face_cache->second);
   hb_font_data->paint_.setTypeface(skia_face);
   hb_font_data->paint_.setTextSize(text_size);
+  // TODO(ckocagil): Do we need to update these params later?
+  internal::ApplyRenderParams(params, &hb_font_data->paint_);
   hb_font_set_funcs(harfbuzz_font, g_font_funcs.Get().get(), hb_font_data,
                     DeleteByType<FontData>);
   hb_font_make_immutable(harfbuzz_font);
   return harfbuzz_font;
 }
 
 // Returns true if characters of |block_code| may trigger font fallback.
 bool IsUnusualBlockCode(UBlockCode block_code) {
   return block_code == UBLOCK_GEOMETRIC_SHAPES ||
          block_code == UBLOCK_MISCELLANEOUS_SYMBOLS;
@@ skipping 568 matching lines @@
   }
 }
 
 void RenderTextHarfBuzz::DrawVisualText(Canvas* canvas) {
   DCHECK(!needs_layout_);
   internal::SkiaTextRenderer renderer(canvas);
   ApplyFadeEffects(&renderer);
   ApplyTextShadows(&renderer);
 
 #if defined(OS_WIN) || defined(OS_LINUX)
   renderer.SetFontRenderParams(

[msw, 2014/08/15 19:21:17]

Can we somehow use the run-specific |hb_font_data->paint_| setup in
CreateHarfBuzzFont, rather than setting a single FontRenderParams here based on
the PrimaryFont? (On Linux, the FontRenderParams can be font-specific).

[ckocagil, 2014/08/15 22:28:01]

We're now calculating FontRenderParams once while shaping and store it in the
run, then use it when we need to draw.

[msw, 2014/08/16 00:30:11]

Nice.

       font_list().GetPrimaryFont().GetFontRenderParams(),
       background_is_transparent());
 #endif
 
   ApplyCompositionAndSelectionStyles();
 
   int current_x = 0;
   const Vector2d line_offset = GetLineOffset(0);
   for (size_t i = 0; i < runs_.size(); ++i) {
     const internal::TextRunHarfBuzz& run = *runs_[visual_to_logical_[i]];
@@ skipping 219 matching lines @@
 }
 
 bool RenderTextHarfBuzz::ShapeRunWithFont(internal::TextRunHarfBuzz* run,
                                           const std::string& font_family) {
   const base::string16& text = GetLayoutText();
   skia::RefPtr<SkTypeface> skia_face =
       internal::CreateSkiaTypeface(font_family, run->font_style);
   if (skia_face == NULL)
     return false;
   run->skia_face = skia_face;
+  FontRenderParamsQuery query(false);
+  query.families.push_back(font_family);
+  query.pixel_size = run->font_size;
+  query.style = run->font_style;
+  FontRenderParams params = GetFontRenderParams(query, NULL);
+  if (background_is_transparent())
+    params.subpixel_rendering = FontRenderParams::SUBPIXEL_RENDERING_NONE;

[Daniel Erat, 2014/08/15 18:34:25]

can you make ApplyRenderParams() check whether the background is transparent and
only do this there?

[ckocagil, 2014/08/15 19:05:57]

ApplyRenderParams doesn't know whether the background is transparent because it
isn't a RenderText member method. To make that work we'd have to move it into
RenderText and make it public so that it can be called from
<anon>::CreateHarfBuzzFont.

Another option is to add a "void
RenderText::AdjustFontRenderParams(FontRenderParams*)" that checks transparency
and sets the subpixel setting. We'd then call that whenever we get a
FontRenderParams.

[msw, 2014/08/15 19:21:17]

I wonder if the pattern here is correct, given how different it is from the
other impls. Should we be using gfx::Font for things like GetFontRenderParams?

[ckocagil, 2014/08/15 22:28:01]

gfx::Font (as it currently is) is kinda heavyweight and other than a few things
like GetFontRenderParams, it is completely unnecessary to have in RTHB.

Ideally we'd have something like PlatformFontSkia. Then we'd be able to use
gfx::Font like other impls. Perhaps a TODO will suffice for now?

[msw, 2014/08/16 00:30:11]

Acknowledged; if you see a specific reason to add a TODO, go for it.

   hb_font_t* harfbuzz_font = CreateHarfBuzzFont(run->skia_face.get(),
-                                                run->font_size);
+                                                run->font_size, params);
 
   // Create a HarfBuzz buffer and add the string to be shaped. The HarfBuzz
   // buffer holds our text, run information to be used by the shaping engine,
   // and the resulting glyph data.
   hb_buffer_t* buffer = hb_buffer_create();
   hb_buffer_add_utf16(buffer, reinterpret_cast<const uint16*>(text.c_str()),
                       text.length(), run->range.start(), run->range.length());
   hb_buffer_set_script(buffer, ICUScriptToHBScript(run->script));
   hb_buffer_set_direction(buffer,
       run->is_rtl ? HB_DIRECTION_RTL : HB_DIRECTION_LTR);
@@ skipping 13 matching lines @@
   run->glyph_to_char.resize(run->glyph_count);
   run->positions.reset(new SkPoint[run->glyph_count]);
   run->width = 0.0f;
   for (size_t i = 0; i < run->glyph_count; ++i) {
     run->glyphs[i] = infos[i].codepoint;
     run->glyph_to_char[i] = infos[i].cluster;
     const int x_offset = SkFixedToScalar(hb_positions[i].x_offset);
     const int y_offset = SkFixedToScalar(hb_positions[i].y_offset);
     run->positions[i].set(run->width + x_offset, -y_offset);
     run->width += SkFixedToScalar(hb_positions[i].x_advance);
+    run->width = std::floor(run->width + 0.5f);

[msw, 2014/08/15 19:21:17]

Whoa, this seems really bad... This rounds the run width each time a glyph's
width is added. Can we avoid this rounding altogether? If not, can we simply
round the overall run width after this glyph loop? Please explain why any
rounding is needed.

[ckocagil, 2014/08/15 22:28:01]

This rounding is needed to place the glyphs at integer values, which is what
Pango does. And it can't be just floor or ceil, it has to be rounded to the
nearest integer (I tried).

[msw, 2014/08/16 00:30:11]

Yuck, please add a comment then. Would we get the same results disabling
subpixel positioning instead? That might be a better [temporary] solution.

While Pango's glyph positioning does seem to look better in the Linux Desktop
screenshots I posted < http://crbug.com/402715 >, I think this will break
CanvasTestMac.*, and I wonder how it affects the look on Mac, Windows, and
Chrome OS (especially with high-dpi). Can you post native/HarfBuzz screenshots
on any of those OSes, similar to the ones I posted? I guess we really want to
achieve parity first and follow up with potential improvements.

[ckocagil, 2014/08/16 14:01:14]

Subpixel positioning seems to be always disabled on Linux. The problem here is,
Skia's rounding logic doesn't match Pango's, so we have to roll our own.

I fixed this to only do the rounding if subpixel positioning is off. I'll add
the screenshots of different rounding methods to http://crbug.com/402715.

   }
 
   hb_buffer_destroy(buffer);
   hb_font_destroy(harfbuzz_font);
   return true;
 }
 
 }  // namespace gfx