Bug 93808

Created attachment 121178 [details] [review]
[PATCH 1/3] ply-label: Don't crash if label plugin fails

Created attachment 121179 [details] [review]
[PATCH 2/3] Add label-ft plugin

Created attachment 121180 [details] [review]
[PATCH 3/3] Install label-ft plugin into initrd, if available

Created attachment 121254 [details] [review]
[PATCH 2/3 v2] Add label-ft plugin

A small bug was found and fixed now in Patch 2/3.
With specific glyphs the end of a line was cut short by a few pixels.

Created attachment 121596 [details] [review]
[PATCH 1/3 v2] ply-label: Don't crash if label plugin fails

Small bug in patch 1 this time. A different code path leading to a similiar nullptr SEGV.

Ping.

Ping #2.

sorry for being slow to review this.  So have you been using the patchset in opensuse the last year ?

Comment on attachment 121596 [details] [review]
[PATCH 1/3 v2] ply-label: Don't crash if label plugin fails

Review of attachment 121596 [details] [review]:
-----------------------------------------------------------------

well plymouth makes no attempt to handle alloc failures, but this patch seems okay to me regardless.

(In reply to Ray Strode [halfline] from comment #8)
> sorry for being slow to review this.  So have you been using the patchset in
> opensuse the last year ?

Yes, it's used by default instead of the full label plugin. So far I haven't seen any issues or even bug reports about it.

openQA considers it fine as well: https://openqa.opensuse.org/tests/337059#step/boot_encrypt/1

(In reply to Ray Strode [halfline] from comment #9)
> Comment on attachment 121596 [details] [review] [review]
> [PATCH 1/3 v2] ply-label: Don't crash if label plugin fails
> 
> Review of attachment 121596 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> well plymouth makes no attempt to handle alloc failures, but this patch
> seems okay to me regardless.

Creation of label-ft also fails if no font file could be loaded.

Comment on attachment 121254 [details] [review]
[PATCH 2/3 v2] Add label-ft plugin

Review of attachment 121254 [details] [review]:
-----------------------------------------------------------------

thanks for working on this. one thing is, the source file probably needs a sed 's/\([^ ]\)(/\1 (/g'.  braces also need to be moved up to the same line.

have you tried this on a hidpi display?

::: src/plugins/controls/Makefile.am
@@ +2,5 @@
>  if ENABLE_PANGO
> +SUBDIRS += label
> +endif
> +if ENABLE_FREETYPE
> +SUBDIRS += label-ft

maybe call it label-minimal instead of label-ft

::: src/plugins/controls/label-ft/plugin.c
@@ +37,5 @@
> +
> +#include "ply-label-plugin.h"
> +
> +/* This is used if fontconfig (fc-match) is not available, like in the initrd. */
> +#define FONT_FALLBACK "/usr/share/fonts/Plymouth.ttf"

i'd put this in /usr/share/plymouth or whatever. If ever ended up in /usr/share/fonts on the rootfs, it would just be a duplicate font, so doesn't make sense in /usr/share/fonts

@@ +68,5 @@
> +{
> +        FILE *fp;
> +        static char fc_match_out[PATH_MAX];
> +
> +        fp = popen("/usr/bin/fc-match -f %{file}", "r");

i wouldn't bother with this.  Let's keep the plugin minimal.  The other one does fontconfig etc, just always use the hard coded font for this plugin.

@@ +226,5 @@
> +static FT_Int
> +min (FT_Int a,
> +     FT_Int b)
> +{
> +    return a < b ? a : b;

we already have this in ply-utils.h as a macro

@@ +308,5 @@
> +        if(target_size.height == 0)
> +                return; /* This happens sometimes. */
> +
> +        /* 64ths of a pixel */
> +        pen.y = label->area.y << 6;

I guess this shift stuff is about freetype using a fixed point representation.  does it have accessor macros you could use?

@@ +320,5 @@
> +                pen.x = label->area.x << 6;
> +
> +                /* Start at start position (alignment) */
> +                if(label->alignment == PLY_LABEL_ALIGN_CENTER)
> +                    pen.x += (label->width - width_of_line(label, cur_c)) << 5;

this is too subtle.  instead put label->width - width_of_line(label, cur_c) in a temporary, empty_space.  Then do

pen.x += empty_space / 2;

@@ +326,5 @@
> +                    pen.x += (label->width - width_of_line(label, cur_c)) << 6;
> +
> +                while(*cur_c && *cur_c != '\n')
> +                {
> +                    /* TODO: Unicode support. */

so of course there will be limits on what kind of support we can do here.  to get full support would need a large collection of fonts and pango etc.
But, we can at least cover some of the european languages relatively painlessly. mbrtowc() and ply_utf8_character_get_size should be useful.

(In reply to Ray Strode [halfline] from comment #11)
> Comment on attachment 121254 [details] [review] [review]
> [PATCH 2/3 v2] Add label-ft plugin
> 
> Review of attachment 121254 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> thanks for working on this. one thing is, the source file probably needs a
> sed 's/\([^ ]\)(/\1 (/g'.  braces also need to be moved up to the same line.

Will do.

> have you tried this on a hidpi display?

No, but I will do so later. I don't know what needs to be done to support HiDPI in plymouth.

> ::: src/plugins/controls/Makefile.am
> @@ +2,5 @@
> >  if ENABLE_PANGO
> > +SUBDIRS += label
> > +endif
> > +if ENABLE_FREETYPE
> > +SUBDIRS += label-ft
> 
> maybe call it label-minimal instead of label-ft

IMO minimal is the wrong adjective, what about "basic"?

> ::: src/plugins/controls/label-ft/plugin.c
> @@ +37,5 @@
> > +
> > +#include "ply-label-plugin.h"
> > +
> > +/* This is used if fontconfig (fc-match) is not available, like in the initrd. */
> > +#define FONT_FALLBACK "/usr/share/fonts/Plymouth.ttf"
> 
> i'd put this in /usr/share/plymouth or whatever. If ever ended up in
> /usr/share/fonts on the rootfs, it would just be a duplicate font, so
> doesn't make sense in /usr/share/fonts

Ok. This is so far only used inside the initrd (patch 3).

> @@ +68,5 @@
> > +{
> > +        FILE *fp;
> > +        static char fc_match_out[PATH_MAX];
> > +
> > +        fp = popen("/usr/bin/fc-match -f %{file}", "r");
> 
> i wouldn't bother with this.  Let's keep the plugin minimal.  The other one
> does fontconfig etc, just always use the hard coded font for this plugin.

Ok.

> @@ +226,5 @@
> > +static FT_Int
> > +min (FT_Int a,
> > +     FT_Int b)
> > +{
> > +    return a < b ? a : b;
> 
> we already have this in ply-utils.h as a macro

Ok.

> @@ +308,5 @@
> > +        if(target_size.height == 0)
> > +                return; /* This happens sometimes. */
> > +
> > +        /* 64ths of a pixel */
> > +        pen.y = label->area.y << 6;
> 
> I guess this shift stuff is about freetype using a fixed point
> representation.  does it have accessor macros you could use?

I'm not aware of any. Should I add one to label-ft.c?

> @@ +320,5 @@
> > +                pen.x = label->area.x << 6;
> > +
> > +                /* Start at start position (alignment) */
> > +                if(label->alignment == PLY_LABEL_ALIGN_CENTER)
> > +                    pen.x += (label->width - width_of_line(label, cur_c)) << 5;
> 
> this is too subtle.  instead put label->width - width_of_line(label, cur_c)
> in a temporary, empty_space.  Then do
> 
> pen.x += empty_space / 2;

I could use the macro above here as well.

> 
> @@ +326,5 @@
> > +                    pen.x += (label->width - width_of_line(label, cur_c)) << 6;
> > +
> > +                while(*cur_c && *cur_c != '\n')
> > +                {
> > +                    /* TODO: Unicode support. */
> 
> so of course there will be limits on what kind of support we can do here. 
> to get full support would need a large collection of fonts and pango etc.
> But, we can at least cover some of the european languages relatively
> painlessly. mbrtowc() and ply_utf8_character_get_size should be useful.

I'll need to read some more FreeType documentation for that first.

Created attachment 129067 [details] [review]
[PATCH 2/3 v2] Add label-ft plugin

Created attachment 129068 [details] [review]
[PATCH v3 1/4] ply-label: Don't crash if label plugin fails

Created attachment 129069 [details] [review]
[PATCH v3 2/4] Add label-ft plugin

Created attachment 129070 [details] [review]
Install label-ft plugin into initrd, if available

Created attachment 129071 [details] [review]
[PATCH v3 4/4] Add HiDPI support to label-ft plugin

v3 of the series fixes formatting and style, as well as macro usage (MIN instead of min, FT_GRID_TO_PX etc. instead of shifting).

I kept the use of fc-match as it would be less useful outside of the initrd otherwise.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/plymouth/plymouth/issues/45.