Re: [PATCH RESEND v3 1/2] font: Add font scaling feature to grub_font_draw_glyph()

[Daniel Kiper]

On Mon, Jun 27, 2022 at 05:35:24PM +0800, Zhang Boyang wrote:
> This patch adds an argument 'scale' to grub_font_draw_glyph(). If
> scale > 1, then the function will create a new scaled bitmap of the
> drawing glyph, and draws the scaled glyph. The scaled bitmap is cached
> in the glyph itself, so it can be reused if same glyph is used many
> times.
>
> To avoid interger overflow during scaling, this patch intruduces
> dimension limits to font files, described by GRUB_FONT_MAX_DIMENSION.
> The scaled glyph should also obey this limit. In addition, scale factor
> is also limited by GRUB_FONT_MAX_SCALE.

If you use overflow aware math operators defined as macros, e.g.
grub_mul(), defined in include/grub/safemath.h probably you
can drop these artificial limits.

> Signed-off-by: Zhang Boyang <zhangboyang.id@gmail.com>
> ---
>  grub-core/commands/videotest.c |   4 +-
>  grub-core/font/font.c          | 120 ++++++++++++++++++++++++++++++---
>  grub-core/gfxmenu/font.c       |   2 +-
>  grub-core/term/gfxterm.c       |   2 +-
>  include/grub/font.h            |  16 ++++-
>  5 files changed, 131 insertions(+), 13 deletions(-)
>
> diff --git a/grub-core/commands/videotest.c b/grub-core/commands/videotest.c
> index ac145afc2..d95ee411d 100644
> --- a/grub-core/commands/videotest.c
> +++ b/grub-core/commands/videotest.c
> @@ -87,7 +87,7 @@ grub_cmd_videotest (grub_command_t cmd __attribute__ 
> ((unused)),
>        return grub_error (GRUB_ERR_BAD_FONT, "no font loaded");
>
>      glyph = grub_font_get_glyph (fixed, '*');
> -    grub_font_draw_glyph (glyph, color, 200 ,0);
> +    grub_font_draw_glyph (glyph, color, 200, 0, 1);
>
>      color = grub_video_map_rgb (255, 255, 255);
>
> @@ -148,7 +148,7 @@ grub_cmd_videotest (grub_command_t cmd __attribute__ 
> ((unused)),
>        {
>       color = grub_video_map_color (i);
>       palette[i] = color;
> -     grub_font_draw_glyph (glyph, color, 16 + i * 16, 220);
> +     grub_font_draw_glyph (glyph, color, 16 + i * 16, 220, 1);
>        }
>    }
>
> diff --git a/grub-core/font/font.c b/grub-core/font/font.c
> index 42189c325..46ffec84c 100644
> --- a/grub-core/font/font.c
> +++ b/grub-core/font/font.c
> @@ -137,6 +137,8 @@ ascii_glyph_lookup (grub_uint32_t code)
>         ascii_font_glyph[current]->offset_x = 0;
>         ascii_font_glyph[current]->offset_y = -2;
>         ascii_font_glyph[current]->device_width = 8;
> +       ascii_font_glyph[current]->scaled_bitmap = NULL;
> +       ascii_font_glyph[current]->scale = 0;
>         ascii_font_glyph[current]->font = NULL;
>
>         grub_memcpy (ascii_font_glyph[current]->bitmap,
> @@ -172,6 +174,8 @@ grub_font_loader_init (void)
>    unknown_glyph->offset_x = 0;
>    unknown_glyph->offset_y = -3;
>    unknown_glyph->device_width = 8;
> +  unknown_glyph->scaled_bitmap = NULL;
> +  unknown_glyph->scale = 0;
>    grub_memcpy (unknown_glyph->bitmap,
>              unknown_glyph_bitmap, sizeof (unknown_glyph_bitmap));
>
> @@ -646,6 +650,20 @@ grub_font_load (const char *filename)
>        goto fail;
>      }
>
> +  if (font->max_char_width <= 0
> +      || font->max_char_width > GRUB_FONT_MAX_DIMENSION
> +      || font->max_char_height <= 0
> +      || font->max_char_height > GRUB_FONT_MAX_DIMENSION
> +      || font->ascent <= 0
> +      || font->ascent > GRUB_FONT_MAX_DIMENSION
> +      || font->descent <= 0
> +      || font->descent > GRUB_FONT_MAX_DIMENSION)
> +    {
> +      grub_error (GRUB_ERR_BAD_FONT,
> +               "invalid font file: bad font metrics");

grub_error (GRUB_ERR_BAD_FONT, N_("invalid font file: bad font metrics"));

Every grub_error() message has to be processed by N_() macro. And yes,
there is no space after N_ and before "(".

Additionally, I am OK with lines a bit longer than 80 chars. So, you can
put grub_error() call in one line here and below.

> +      goto fail;
> +    }
> +
>    /* Add the font to the global font registry.  */
>    if (register_font (font) != 0)
>      goto fail;
> @@ -738,7 +756,7 @@ grub_font_get_glyph_internal (grub_font_t font, 
> grub_uint32_t code)
>        grub_uint16_t height;
>        grub_int16_t xoff;
>        grub_int16_t yoff;
> -      grub_int16_t dwidth;
> +      grub_uint16_t dwidth;
>        int len;
>
>        if (index_entry->glyph)
> @@ -760,7 +778,18 @@ grub_font_get_glyph_internal (grub_font_t font, 
> grub_uint32_t code)
>         || read_be_uint16 (font->file, &height) != 0
>         || read_be_int16 (font->file, &xoff) != 0
>         || read_be_int16 (font->file, &yoff) != 0
> -       || read_be_int16 (font->file, &dwidth) != 0)
> +       || read_be_uint16 (font->file, &dwidth) != 0)
> +     {
> +       remove_font (font);
> +       return 0;
> +     }
> +
> +      /* Reject illegal glyphs.  */
> +      if (width > font->max_char_width
> +       || height > font->max_char_height
> +       || xoff < -GRUB_FONT_MAX_DIMENSION || xoff > GRUB_FONT_MAX_DIMENSION
> +       || yoff < -GRUB_FONT_MAX_DIMENSION || yoff > GRUB_FONT_MAX_DIMENSION
> +       || dwidth > GRUB_FONT_MAX_DIMENSION)
>       {
>         remove_font (font);
>         return 0;
> @@ -780,6 +809,8 @@ grub_font_get_glyph_internal (grub_font_t font, 
> grub_uint32_t code)
>        glyph->offset_x = xoff;
>        glyph->offset_y = yoff;
>        glyph->device_width = dwidth;
> +      glyph->scaled_bitmap = NULL;
> +      glyph->scale = 0;
>
>        /* Don't try to read empty bitmaps (e.g., space characters).  */
>        if (len != 0)
> @@ -787,6 +818,7 @@ grub_font_get_glyph_internal (grub_font_t font, 
> grub_uint32_t code)
>         if (grub_file_read (font->file, glyph->bitmap, len) != len)
>           {
>             remove_font (font);
> +           grub_free (glyph->scaled_bitmap);
>             grub_free (glyph);
>             return 0;
>           }
> @@ -1054,6 +1086,8 @@ grub_font_dup_glyph (struct grub_font_glyph *glyph)
>      return NULL;
>    grub_memcpy (ret, glyph, sizeof (*ret)
>              + (glyph->width * glyph->height + 7) / 8);
> +  ret->scaled_bitmap = NULL;
> +  ret->scale = 0;
>    return ret;
>  }
>  #endif
> @@ -1524,11 +1558,22 @@ grub_font_construct_glyph (grub_font_t hinted_font,
>
>    if (max_glyph_size < sizeof (*glyph) + (bounds.width * bounds.height + 
> GRUB_CHAR_BIT - 1) / GRUB_CHAR_BIT)
>      {
> -      grub_free (glyph);
> +      if (glyph)
> +     {
> +       grub_free (glyph->scaled_bitmap);
> +       grub_free (glyph);
> +     }
>        max_glyph_size = (sizeof (*glyph) + (bounds.width * bounds.height + 
> GRUB_CHAR_BIT - 1) / GRUB_CHAR_BIT) * 2;
>        if (max_glyph_size < 8)
>       max_glyph_size = 8;
>        glyph = grub_malloc (max_glyph_size);
> +      if (glyph)
> +     {
> +       glyph->scaled_bitmap = NULL;
> +       glyph->scale = 0;
> +     }
> +      else
> +     max_glyph_size = 0;
>      }
>    if (!glyph)
>      {
> @@ -1536,6 +1581,7 @@ grub_font_construct_glyph (grub_font_t hinted_font,
>        return main_glyph;
>      }
>
> +  grub_free (glyph->scaled_bitmap);
>    grub_memset (glyph, 0, sizeof (*glyph)
>              + (bounds.width * bounds.height
>                 + GRUB_CHAR_BIT - 1) / GRUB_CHAR_BIT);
> @@ -1545,6 +1591,8 @@ grub_font_construct_glyph (grub_font_t hinted_font,
>    glyph->height = bounds.height;
>    glyph->offset_x = bounds.x;
>    glyph->offset_y = bounds.y;
> +  glyph->scaled_bitmap = NULL;
> +  glyph->scale = 0;
>
>    if (glyph_id->attributes & GRUB_UNICODE_GLYPH_ATTRIBUTE_MIRROR)
>      grub_font_blit_glyph_mirror (glyph, main_glyph,
> @@ -1563,12 +1611,50 @@ grub_font_construct_glyph (grub_font_t hinted_font,
>    return glyph;
>  }
>
> +/* Try to scale the glyph bitmap.  If failed, glyph will not be touched.
> +   If succeeded, scaled bitmap will be stored at glyph->scaled_bitmap , and
> +   the effective scale factor will be stored at glyph->scale .  */

Incorrectly formatted comment. Please fix this. Here you can find more
details about comments formatting [1].

> +static void
> +try_scale_glyph (struct grub_font_glyph * glyph, int scale)
> +{
> +  int i, j;
> +  grub_uint8_t *bitmap;
> +
> +  if (glyph->scale == scale)
> +    return;
> +
> +  if (scale > GRUB_FONT_MAX_SCALE
> +      || glyph->height * scale > GRUB_FONT_MAX_DIMENSION
> +      || glyph->width * scale > GRUB_FONT_MAX_DIMENSION)
> +    return;
> +
> +  bitmap = grub_zalloc (((glyph->width * scale) *
> +                      (glyph->height * scale) + 7) / 8);

grub_calloc()? And I think you should use grub_mul() and grub_add()
macros here and below...

> +  if (!bitmap)

if (bitmap = NULL)

Please use NULL explicitly in such comparisons.

> +    return;
> +
> +  /* FIXME: suboptimal.  */
> +  for (i = 0; i < glyph->height * scale; i++)
> +    for (j = 0; j < glyph->width * scale; j++)
> +      {
> +     int n = (i / scale) * glyph->width + (j / scale);
> +     int m = i * (glyph->width * scale) + j;
> +     int pixel = (glyph->bitmap[n / 8] >> (7 - n % 8)) & 1;
> +     bitmap[m / 8] |= pixel << (7 - m % 8);
> +      }
> +
> +  grub_free (glyph->scaled_bitmap);
> +  glyph->scaled_bitmap = bitmap;
> +  glyph->scale = scale;
> +}
> +
>  /* Draw the specified glyph at (x, y).  The y coordinate designates the
>     baseline of the character, while the x coordinate designates the left
>     side location of the character.  */
>  grub_err_t
>  grub_font_draw_glyph (struct grub_font_glyph * glyph,
> -                   grub_video_color_t color, int left_x, int baseline_y)
> +                   grub_video_color_t color, int left_x, int baseline_y,
> +                   int scale)
>  {
>    struct grub_video_bitmap glyph_bitmap;
>
> @@ -1601,11 +1687,29 @@ grub_font_draw_glyph (struct grub_font_glyph * glyph,
>                         &glyph_bitmap.mode_info.fg_alpha);
>    glyph_bitmap.data = glyph->bitmap;
>
> -  int bitmap_left = left_x + glyph->offset_x;
> -  int bitmap_bottom = baseline_y - glyph->offset_y;
> -  int bitmap_top = bitmap_bottom - glyph->height;
> +  if (scale > 1)
> +    {
> +      try_scale_glyph (glyph, scale);
> +      if (glyph->scale == scale)
> +     {
> +       glyph_bitmap.mode_info.width = glyph->width * scale;
> +       glyph_bitmap.mode_info.height = glyph->height * scale;
> +       glyph_bitmap.mode_info.pitch = glyph->width * scale;
> +       glyph_bitmap.data = glyph->scaled_bitmap;
> +     }
> +      else
> +     {
> +       /* Scaled bitmap not suitable, fallback to no-scale.  */
> +       scale = 1;
> +     }
> +    }
> +
> +  int bitmap_left = left_x + glyph->offset_x * scale;
> +  int bitmap_bottom = baseline_y - glyph->offset_y * scale;
> +  int bitmap_top = bitmap_bottom - glyph->height * scale;

You do a lot of scaling math here and there. It would be probably better
if you use safe math macros wrapped in some scaling functions/macros.

>    return grub_video_blit_bitmap (&glyph_bitmap, GRUB_VIDEO_BLIT_BLEND,
>                                bitmap_left, bitmap_top,
> -                              0, 0, glyph->width, glyph->height);
> +                              0, 0,
> +                              glyph->width * scale, glyph->height * scale);
>  }
> diff --git a/grub-core/gfxmenu/font.c b/grub-core/gfxmenu/font.c
> index 756c24f20..ed59ca954 100644
> --- a/grub-core/gfxmenu/font.c
> +++ b/grub-core/gfxmenu/font.c
> @@ -67,7 +67,7 @@ grub_font_draw_string (const char *str, grub_font_t font,
>         err = grub_errno;
>         goto out;
>       }
> -      err = grub_font_draw_glyph (glyph, color, x, baseline_y);
> +      err = grub_font_draw_glyph (glyph, color, x, baseline_y, 1);
>        if (err)
>       goto out;
>        x += glyph->device_width;
> diff --git a/grub-core/term/gfxterm.c b/grub-core/term/gfxterm.c
> index 3c468f459..4512dee6f 100644
> --- a/grub-core/term/gfxterm.c
> +++ b/grub-core/term/gfxterm.c
> @@ -656,7 +656,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);
> +  grub_font_draw_glyph (glyph, color, x, y + ascent, 1);
>    grub_video_set_active_render_target (render_target);
>
>    /* Mark character to be drawn.  */
> diff --git a/include/grub/font.h b/include/grub/font.h
> index 708fa42ac..8fc646713 100644
> --- a/include/grub/font.h
> +++ b/include/grub/font.h
> @@ -24,6 +24,13 @@
>  #include <grub/file.h>
>  #include <grub/unicode.h>
>
> +/* Limit font dimensions to avoid interger overflow. Because some font 
> metrics
> +   may be derived from this limit, we must reserve some space for 
> arithmetics,
> +   so this value can't be too high.  When scaling fonts, this limit should 
> also
> +   applies to the font after scaling.  */

Please fix this comment formatting.

> +#define GRUB_FONT_MAX_DIMENSION 500
> +#define GRUB_FONT_MAX_SCALE 10
> +
>  /* Forward declaration of opaque structure grub_font.
>     Users only pass struct grub_font pointers to the font module functions,
>     and do not have knowledge of the structure contents.  */

Ditto and below please...

> @@ -79,6 +86,12 @@ struct grub_font_glyph
>    /* Number of pixels to advance to start the next character.  */
>    grub_uint16_t device_width;
>
> +  /* Pointer to cached scaled bitmap.  Allocated during a scaling drawing.  
> */
> +  grub_uint8_t *scaled_bitmap;
> +
> +  /* The scale factor for cached scaled bitmap.  */
> +  grub_uint16_t scale;
> +
>    /* Row-major order, packed bits (no padding; rows can break within a byte).
>       The length of the array is (width * height + 7) / 8.  Within a
>       byte, the most significant bit is the first (leftmost/uppermost) pixel.
> @@ -141,7 +154,8 @@ struct grub_font_glyph *EXPORT_FUNC 
> (grub_font_get_glyph_with_fallback) (grub_fo
>
>  grub_err_t EXPORT_FUNC (grub_font_draw_glyph) (struct grub_font_glyph *glyph,
>                                              grub_video_color_t color,
> -                                            int left_x, int baseline_y);
> +                                            int left_x, int baseline_y,
> +                                            int scale);
>
>  int
>  EXPORT_FUNC (grub_font_get_constructed_device_width) (grub_font_t 
> hinted_font,

Daniel

[1] https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Comments