New issue
Advanced search Search tips

Issue 845468 link

Starred by 3 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 3
Type: Bug

Blocking:
issue 849877



Sign in to add a comment

Font.h device_scale_factor arguments not useful?

Project Member Reported by drott@chromium.org, May 22 2018

Issue description

Font::drawText and other methods have a device_scale_factor argument, which is sent from GraphicsContext for example. GraphicsContext's device_scale_factor is set for example from:
frame_painter.cc's:

  float device_scale_factor = blink::DeviceScaleFactorDeprecated(
      root_layer->GetLayoutObject().GetFrame());
  context.SetDeviceScaleFactor(device_scale_factor);

But even when we run the scalefactor200 virtual test suite, this device_scale_factor is 1 and not used anymore, probably due to using zoom for hidpi. 

Can we remove these arguments, or replace them with the physical DSF of the the display again? 

Background: I am trying to retrieve font rasterization settings from a new utility process that does not know about any browser command line arguments or more context and I need to pass a useful value here, since we decide whether we want subpixel positioning and depending on that, disable hinting, based on the DSF.

Assigning to fmalita@ for comment, thanks.
 

Comment 1 by drott@chromium.org, May 22 2018

Cc: -kojii@chromium.org chrishtr@chromium.org derat@chromium.org wangxianzhu@chromium.org behdad@chromium.org fmalita@chromium.org
Owner: ----
Status: Available (was: Assigned)
Adding chrishtr@ and wangxianzhu@ - could you explain the comment in: 
https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/page/page_widget_delegate.cc?type=cs&g=0&l=81

Specifically, (intuitively, I would agree, but) why is the device_scale_factor a layering violation in Blink?

More background:
We have code in Blink querying the browser for FontRenderParams and depending on what device scale factor is set for font rendering the browser, we enable subpixel positioning & disable hinting for matching system fonts. 

It's weird that the value can change from underneath the renderer due to changes in screen configuration. 

Instead of relying on what global value is set in the browser, I would need to pass a device scale factor to the QueryRenderParams function. What would make sense to pass here?

More widely asking:
I don't fully understand why we arrived at these settings for deciding on subpixel positioning for system fonts, and why we make a difference here for Chrome OS: ( https://cs.chromium.org/chromium/src/ui/gfx/font_render_params_linux.cc?q=font_render_params_linux&sq=package:chromium&dr&l=283 )

#if !defined(OS_CHROMEOS)
    params.subpixel_positioning = actual_query.device_scale_factor > 1.0f;
#else
    // We want to enable subpixel positioning for fractional dsf.
    params.subpixel_positioning =
        std::abs(std::round(actual_query.device_scale_factor) -
                 actual_query.device_scale_factor) >
        std::numeric_limits<float>::epsilon();
#endif  // !defined(OS_CHROMEOS)

    // // To enable subpixel positioning, we need to disable hinting.
    if (params.subpixel_positioning)
    params.hinting = FontRenderParams::HINTING_NONE;





Alternatively, we need to make the decision on whether we want to enable subpixel_positioning and hinting independent of the device scale factor in our font code.





Comment 2 by derat@chromium.org, May 22 2018

Cc: malaykeshav@chromium.org osh...@chromium.org
Adding people who know more about why it's done this way.
> Adding chrishtr@ and wangxianzhu@ - could you explain the comment in: 
> https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/page/page_widget_delegate.cc?type=cs&g=0&l=81
> Specifically, (intuitively, I would agree, but) why is the device_scale_factor a layering violation in Blink?

I think the comment applies to the old device_scale_factor mechanism in which most of the blink code shouldn't know device_scale_factor and the factor is applied only when we convert blink space (in px) into device space. Some platforms (e.g. android) are using the old mechanism. It doesn't apply to use-zoom-for-dsf in which page.DeviceScaleFactorDeprecated() is always 1.

That's my understanding also:

- previously, final_scale = DSF * zoom

- now (use-zoom-for-dsf enabled), final_scale = 1 * zoom

DSF plumbing is historical (and confusing), and will hopefully go away when all platforms enable use-zoom-for-dsf (issue 485650).


@drott, off-hand, do you care about anything other than the final/total scale?  If not, it shouldn't matter how it gets derived, right?  Some of the existing heuristics seem odd if they only consider DSF and not the final scale.

I would totally vote to remove DSF, but we can prolly only do that after use-zoom-for-dsf ships everywhere.

Comment 5 by osh...@chromium.org, May 22 2018

While I didn't work on blink/webkit side font handling, this may still needed for mac (and possibly Android, if it hasn't switched to use-zoom-for-dsf).

The reason why we "had to" use subpixel positioning was that impl side painting
layout contents at 1x (thus uses hinting for 1x), then draw at 2x on 2x devices, and that was causing incorrect font rendering.

In theory, we shouldn't need it any more (with use-zoom-for-dsf, and pixel-canvas painting) but 
it's blocked by a bug in gfx::RenderText.

Comment 6 by drott@chromium.org, May 24 2018

At least for one thing, I noticed that the DSF known to Blink, for example during certain HiDPI/Scalefactor tests and the one in the browser process are out of sync. We should pass it explicitly to gfx::GetFontRenderParams. I'll upload a patch.

Comment 7 by osh...@chromium.org, May 24 2018

In use-zoom-for-dsf mode, it works as if the 200% zoom is applied when dsf == 2.
In that case dsf that blink see is 1 while zoom factor is 200, and that's expected.

Comment 8 by drott@chromium.org, Jun 5 2018

Blocking: 849877
Project Member

Comment 9 by bugdroid1@chromium.org, Jun 6 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/13d087e7a5c30dfd10831dcb7cd5247c0b2bd623

commit 13d087e7a5c30dfd10831dcb7cd5247c0b2bd623
Author: Dominik Röttsches <drott@chromium.org>
Date: Wed Jun 06 18:39:52 2018

Pass DSF to FontCache before layout

Preparation for fixing the DSF that is used for querying render style
for strike, see  crbug.com/849877 .

If one renderer maintains multiple views, we need to inform the font
cache about this view's DSF before performing layout. This is because
FontPlatformData on Linux / CrOS needs the DSF for querying the
RenderStyle for fonts in this view correctly from out of process.

Prepare the FontCache for including the DSF in GetFontplatformData
queries.

Bug: 845468
Change-Id: Ib3de63dcdb18261502837905a04e563a30bcb42e
Reviewed-on: https://chromium-review.googlesource.com/1087908
Reviewed-by: Becca Hughes <beccahughes@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Commit-Queue: Dominik Röttsches <drott@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564978}
[modify] https://crrev.com/13d087e7a5c30dfd10831dcb7cd5247c0b2bd623/third_party/blink/renderer/core/css/css_font_face_source_test.cc
[modify] https://crrev.com/13d087e7a5c30dfd10831dcb7cd5247c0b2bd623/third_party/blink/renderer/core/layout/layout_view.cc
[modify] https://crrev.com/13d087e7a5c30dfd10831dcb7cd5247c0b2bd623/third_party/blink/renderer/modules/media_controls/media_controls_rotate_to_fullscreen_delegate_test.cc
[modify] https://crrev.com/13d087e7a5c30dfd10831dcb7cd5247c0b2bd623/third_party/blink/renderer/platform/fonts/font_cache.cc
[modify] https://crrev.com/13d087e7a5c30dfd10831dcb7cd5247c0b2bd623/third_party/blink/renderer/platform/fonts/font_cache.h
[modify] https://crrev.com/13d087e7a5c30dfd10831dcb7cd5247c0b2bd623/third_party/blink/renderer/platform/fonts/font_cache_key.h
[modify] https://crrev.com/13d087e7a5c30dfd10831dcb7cd5247c0b2bd623/third_party/blink/renderer/platform/fonts/font_description.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Jun 6 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c160acab0021bb90f4ae1ebb696f2364ed0879b2

commit c160acab0021bb90f4ae1ebb696f2364ed0879b2
Author: Dominik Röttsches <drott@chromium.org>
Date: Wed Jun 06 22:40:32 2018

Pass device scale factor explicitly to gfx::FontRenderParams

The device scale factor that is configured in the browser process, for example
through Gtk environment settings, or through the command line flag
--force-scale-factor is not always identical with the one in the renderer
process. The renderer process DSF can be configures als through TestingInternals
and other configuration mechanisms. So these two can go out of sync and do not
have the same semantics. As a solution, pass the DSF from Blink via IPC to
identify the intended rasterization settings for a system font.

Rebaselines are needed for those tests where the DSF settings in browser and
renderer went out of sync.

Overall, this CL is preparation for moving the Sandbox IPC functions to Mojo
instead of the current filedescriptor communication approach. Also, addressing
an older TODO about not passing size and is_bold, is_italic in one variable.

Bug: 845468
Change-Id: I6fbe5c906f31f2e8872296404ffa269fbeb652f0
Reviewed-on: https://chromium-review.googlesource.com/1071512
Commit-Queue: Dominik Röttsches <drott@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565068}
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/content/browser/sandbox_ipc_linux.cc
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/content/child/child_process_sandbox_support_impl_linux.cc
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/content/child/child_process_sandbox_support_impl_linux.h
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/content/common/common_sandbox_support_linux.cc
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/content/ppapi_plugin/ppapi_blink_platform_impl.cc
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/content/renderer/renderer_blink_platform_impl.cc
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/content/utility/utility_blink_platform_with_sandbox_support_impl.cc
[add] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/third_party/WebKit/LayoutTests/platform/linux/fast/canvas/canvas-hidpi-blurry-expected.png
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/third_party/WebKit/LayoutTests/platform/linux/fast/hidpi/broken-image-icon-hidpi-expected.png
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/third_party/WebKit/LayoutTests/platform/linux/fast/hidpi/broken-image-icon-hidpi-expected.txt
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/third_party/WebKit/LayoutTests/platform/linux/fast/hidpi/broken-image-with-size-hidpi-expected.png
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/third_party/WebKit/LayoutTests/platform/linux/fast/hidpi/broken-image-with-size-hidpi-expected.txt
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/third_party/WebKit/LayoutTests/platform/linux/fast/hidpi/clip-text-in-hidpi-expected.png
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/third_party/WebKit/LayoutTests/platform/linux/fast/hidpi/clip-text-in-hidpi-expected.txt
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/third_party/WebKit/LayoutTests/platform/linux/fast/hidpi/resize-corner-hidpi-expected.png
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/third_party/WebKit/LayoutTests/platform/linux/fast/hidpi/resize-corner-hidpi-expected.txt
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/third_party/WebKit/LayoutTests/platform/linux/fast/hidpi/scrollbar-appearance-increase-device-scale-factor-expected.png
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/third_party/WebKit/LayoutTests/platform/linux/fast/hidpi/scrollbar-appearance-increase-device-scale-factor-expected.txt
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/third_party/WebKit/LayoutTests/platform/linux/paint/markers/grammar-markers-hidpi-expected.png
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/third_party/WebKit/LayoutTests/platform/linux/paint/markers/inline-spelling-markers-hidpi-composited-expected.png
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/third_party/WebKit/LayoutTests/platform/linux/paint/markers/inline-spelling-markers-hidpi-expected.png
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/third_party/WebKit/LayoutTests/platform/linux/scrollbars/resize-scales-with-dpi-150-expected.png
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/third_party/WebKit/LayoutTests/platform/linux/svg/as-image/image-respects-deviceScaleFactor-expected.png
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/third_party/WebKit/LayoutTests/platform/linux/svg/as-image/image-respects-deviceScaleFactor-expected.txt
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.png
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/third_party/WebKit/LayoutTests/platform/linux/svg/custom/masking-clipping-hidpi-expected.txt
[add] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/third_party/WebKit/LayoutTests/platform/linux/virtual/gpu/fast/canvas/canvas-hidpi-blurry-expected.png
[delete] https://crrev.com/d6a779bddb099e1d2ae21572a6ca557adc964c36/third_party/WebKit/LayoutTests/platform/linux/virtual/prefer_compositing_to_lcd_text/scrollbars/resize-scales-with-dpi-150-expected.png
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/third_party/WebKit/LayoutTests/platform/linux/virtual/prefer_compositing_to_lcd_text/scrollbars/resize-scales-with-dpi-150-expected.txt
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/third_party/WebKit/LayoutTests/platform/linux/virtual/scalefactor150/fast/hidpi/static/validation-bubble-appearance-hidpi-expected.png
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/third_party/WebKit/LayoutTests/platform/linux/virtual/scalefactor200/fast/hidpi/static/validation-bubble-appearance-hidpi-expected.png
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/third_party/WebKit/LayoutTests/platform/linux/virtual/scalefactor200withzoom/fast/hidpi/static/validation-bubble-appearance-hidpi-expected.png
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/third_party/blink/public/platform/linux/web_sandbox_support.h
[modify] https://crrev.com/c160acab0021bb90f4ae1ebb696f2364ed0879b2/third_party/blink/renderer/platform/fonts/font_platform_data.cc

Sign in to add a comment