Re: [PATCH RESEND v3 2/2] term/gfxterm: Preliminary HiDPI support

[Daniel Kiper]

On Mon, Jun 27, 2022 at 05:35:25PM +0800, Zhang Boyang wrote:
> Currently GRUB's default font is too small to see on a HiDPI monitor.
> This patch adds preliminary HiDPI support to gfxterm. It introduces a
> new environment variable 'gfxterm_scale'. If set to 0, and a high
> resolution monitor is detected, it will scale the font size
> automatically. If set to other number, that number will be the scale
> factor, overriding automatic scale factor calculation.
>
> Signed-off-by: Zhang Boyang <zhangboyang.id@gmail.com>
> ---
>  docs/grub.texi           | 11 ++++++
>  grub-core/gfxmenu/view.c |  1 +
>  grub-core/term/gfxterm.c | 72 ++++++++++++++++++++++++++++------------
>  include/grub/gfxterm.h   |  3 +-
>  4 files changed, 64 insertions(+), 23 deletions(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 9b902273c..8f82061f6 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -3274,6 +3274,7 @@ These variables have special meaning to GRUB.
>  * gfxmode::
>  * gfxpayload::
>  * gfxterm_font::
> +* gfxterm_scale::
>  * grub_cpu::
>  * grub_platform::
>  * icondir::
> @@ -3548,6 +3549,16 @@ If this variable is set, it names a font to use for 
> text on the
>  available font.
>
>
> +@node gfxterm_scale
> +@subsection gfxterm_scale
> +
> +If this variable is not set, or set to @samp{0}, the @samp{gfxterm}
> +graphical terminal will scale the font automatically when a high resolution
> +monitor is detected.  If set to other number, the font scale factor will be
> +forced to that number.  Set this to @samp{1} if user don't want
> +@samp{gfxterm} to scale the font on screen.
> +
> +
>  @node grub_cpu
>  @subsection grub_cpu
>
> diff --git a/grub-core/gfxmenu/view.c b/grub-core/gfxmenu/view.c
> index 6358004b2..94b9ef4db 100644
> --- a/grub-core/gfxmenu/view.c
> +++ b/grub-core/gfxmenu/view.c
> @@ -546,6 +546,7 @@ init_terminal (grub_gfxmenu_view_t view)
>                             view->terminal_rect.height,
>                             view->double_repaint,
>                             terminal_font,
> +                           1,
>                             view->terminal_border);
>    grub_gfxterm_decorator_hook = grub_gfxmenu_draw_terminal_box;
>  }
> diff --git a/grub-core/term/gfxterm.c b/grub-core/term/gfxterm.c
> index 4512dee6f..df2d3f86b 100644
> --- a/grub-core/term/gfxterm.c
> +++ b/grub-core/term/gfxterm.c
> @@ -82,6 +82,7 @@ struct grub_virtual_screen
>
>    /* Font settings. */
>    grub_font_t font;
> +  unsigned int scale;
>
>    /* Terminal color settings.  */
>    grub_uint8_t standard_color_setting;
> @@ -204,7 +205,7 @@ grub_virtual_screen_free (void)
>  static grub_err_t
>  grub_virtual_screen_setup (unsigned int x, unsigned int y,
>                             unsigned int width, unsigned int height,
> -                        grub_font_t font)
> +                        grub_font_t font, unsigned int scale)
>  {
>    unsigned int i;
>
> @@ -213,6 +214,7 @@ grub_virtual_screen_setup (unsigned int x, unsigned int y,
>
>    /* Initialize with default data.  */
>    virtual_screen.font = font;
> +  virtual_screen.scale = scale;
>    virtual_screen.width = width;
>    virtual_screen.height = height;
>    virtual_screen.offset_x = x;
> @@ -220,9 +222,9 @@ grub_virtual_screen_setup (unsigned int x, unsigned int y,
>    virtual_screen.normal_char_width =
>      calculate_normal_character_width (virtual_screen.font);
>    virtual_screen.normal_char_height =
> -    grub_font_get_max_char_height (virtual_screen.font);
> +    grub_font_get_max_char_height (virtual_screen.font) * 
> virtual_screen.scale;

Another good place for safe math macro...

>    if (virtual_screen.normal_char_height == 0)
> -    virtual_screen.normal_char_height = 16;
> +    virtual_screen.normal_char_height = 16 * virtual_screen.scale;

... and here and below...

>    virtual_screen.cursor_x = 0;
>    virtual_screen.cursor_y = 0;
>    virtual_screen.cursor_state = 1;
> @@ -297,7 +299,8 @@ grub_err_t
>  grub_gfxterm_set_window (struct grub_video_render_target *target,
>                        int x, int y, int width, int height,
>                        int double_repaint,
> -                      grub_font_t font, int border_width)
> +                      grub_font_t font, int scale,
> +                      int border_width)
>  {
>    /* Clean up any prior instance.  */
>    destroy_window ();
> @@ -306,10 +309,10 @@ grub_gfxterm_set_window (struct 
> grub_video_render_target *target,
>    render_target = target;
>
>    /* Create virtual screen.  */
> -  if (grub_virtual_screen_setup (border_width, border_width,
> -                                 width - 2 * border_width,
> -                                 height - 2 * border_width,
> -                                 font)
> +  if (grub_virtual_screen_setup (border_width * scale, border_width * scale,
> +                                 width - 2 * border_width * scale,
> +                                 height - 2 * border_width * scale,
> +                                 font, scale)
>        != GRUB_ERR_NONE)
>      {
>        return grub_errno;
> @@ -332,11 +335,13 @@ static grub_err_t
>  grub_gfxterm_fullscreen (void)
>  {
>    const char *font_name;
> +  const char *scale_conf;
>    struct grub_video_mode_info mode_info;
>    grub_video_color_t color;
>    grub_err_t err;
>    int double_redraw;
>    grub_font_t font;
> +  int scale;
>
>    err = grub_video_get_info (&mode_info);
>    /* Figure out what mode we ended up.  */
> @@ -366,12 +371,34 @@ grub_gfxterm_fullscreen (void)
>    if (!font)
>      return grub_error (GRUB_ERR_BAD_FONT, "no font loaded");
>
> +  /* Decide font scale factor. */
> +  scale_conf = grub_env_get ("gfxterm_scale");
> +  scale = 0;
> +  if (scale_conf)

if (scale_conf != NULL)

> +    scale = (int) grub_strtoul (scale_conf, 0, 0);

You completely ignore grub_strtoul() errors. Please check commit
ac8a37dda (net/http: Allow use of non-standard TCP/IP ports) how it
should be done.

> +  if (scale < 0 || scale > GRUB_FONT_MAX_SCALE)
> +    scale = 0;
> +  if (scale == 0)
> +    {
> +      if (mode_info.width > 7680 && mode_info.height > 4320)
> +     scale = 8;
> +      else if (mode_info.width > 3840 && mode_info.height > 2160)
> +     scale = 4;
> +      else if (mode_info.width > 1920 && mode_info.height > 1080)

I would prefer if we come up with some proper math for this instead of
hard coding it like above.

> +     scale = 2;
> +      else
> +     scale = 1;
> +    }
> +  if (grub_font_get_max_char_width(font) * scale > GRUB_FONT_MAX_DIMENSION
> +      || grub_font_get_max_char_height(font) * scale > 
> GRUB_FONT_MAX_DIMENSION)
> +    scale = 1;
> +
>    grub_gfxterm_decorator_hook = NULL;
>
>    return grub_gfxterm_set_window (GRUB_VIDEO_RENDER_TARGET_DISPLAY,
>                                 0, 0, mode_info.width, mode_info.height,
>                                 double_redraw,
> -                               font, DEFAULT_BORDER_WIDTH);
> +                               font, scale, DEFAULT_BORDER_WIDTH);
>  }
>
>  static grub_err_t
> @@ -642,7 +669,7 @@ paint_char (unsigned cx, unsigned cy)
>        grub_errno = GRUB_ERR_NONE;
>        return;
>      }
> -  ascent = grub_font_get_ascent (virtual_screen.font);
> +  ascent = grub_font_get_ascent (virtual_screen.font) * virtual_screen.scale;
>
>    width = virtual_screen.normal_char_width * 
> calculate_character_width(glyph);
>    height = virtual_screen.normal_char_height;
> @@ -656,7 +683,7 @@ paint_char (unsigned cx, unsigned cy)
>    /* Render glyph to text layer.  */
>    grub_video_set_active_render_target (text_layer);
>    grub_video_fill_rect (bgcolor, x, y, width, height);
> -  grub_font_draw_glyph (glyph, color, x, y + ascent, 1);
> +  grub_font_draw_glyph (glyph, color, x, y + ascent, virtual_screen.scale);
>    grub_video_set_active_render_target (render_target);
>
>    /* Mark character to be drawn.  */
> @@ -690,9 +717,9 @@ draw_cursor (int show)
>      return;
>
>    /* Ensure that cursor doesn't go outside of character box.  */
> -  ascent = grub_font_get_ascent(virtual_screen.font);
> -  if (ascent > virtual_screen.normal_char_height - 2)
> -    ascent = virtual_screen.normal_char_height - 2;
> +  ascent = grub_font_get_ascent(virtual_screen.font) * virtual_screen.scale;
> +  if (ascent > virtual_screen.normal_char_height - 2 * virtual_screen.scale)
> +    ascent = virtual_screen.normal_char_height - 2 * virtual_screen.scale;
>
>    /* Determine cursor properties and position on text layer. */
>    x = virtual_screen.cursor_x * virtual_screen.normal_char_width;
> @@ -701,7 +728,7 @@ draw_cursor (int show)
>    y = ((virtual_screen.cursor_y + virtual_screen.total_scroll)
>         * virtual_screen.normal_char_height
>         + ascent);
> -  height = 2;
> +  height = 2 * virtual_screen.scale;
>
>    /* Render cursor to text layer.  */
>    grub_video_set_active_render_target (text_layer);
> @@ -957,18 +984,18 @@ calculate_normal_character_width (grub_font_t font)
>       width = glyph->device_width;
>      }
>    if (!width)
> -    return 8;
> +    return 8 * virtual_screen.scale;
>
> -  return width;
> +  return width * virtual_screen.scale;
>  }
>
>  static unsigned char
>  calculate_character_width (struct grub_font_glyph *glyph)
>  {
>    if (! glyph || glyph->device_width == 0)
> -    return 1;
> +    return 1 * virtual_screen.scale;

? I would just do "return virtual_screen.scale"...

Daniel