Message ID | 1413599958-24524-1-git-send-email-maarten@treewalker.org |
---|---|
State | Rejected |
Headers | show |
Hi Maarten, all On Sat, Oct 18, 2014 at 4:39 AM, Maarten ter Huurne <maarten@treewalker.org> wrote: > If bzip2 is available, it might still be preferable to not include > support for bzip2 compressed fonts, for example to reduce the number > of libraries loaded when running a GUI application. > > Signed-off-by: Maarten ter Huurne <maarten@treewalker.org> > --- > package/freetype/Config.in | 9 +++++++++ > package/freetype/freetype.mk | 2 +- > 2 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/package/freetype/Config.in b/package/freetype/Config.in > index ded8738..f245b14 100644 > --- a/package/freetype/Config.in > +++ b/package/freetype/Config.in > @@ -4,3 +4,12 @@ config BR2_PACKAGE_FREETYPE > a free, high-quality and portable font engine. > > http://www.freetype.org/ > + > +if BR2_PACKAGE_FREETYPE > + > +config BR2_PACKAGE_FREETYPE_BZIP2 > + bool "support bzip2 compressed fonts" I would have had something like: bool "Enable compressed fonts" help Enable supports for bzip2 compressed fonts > + depends on BR2_PACKAGE_BZIP2 > + default y Why would we want this to 'default y' ? It kind of goes against what you state in the commit log, you don't want to have compressed fonts by default even if bzip2 is selected, right ? Also, if the users selects this options, it should select the package bzip2 > + > +endif > diff --git a/package/freetype/freetype.mk b/package/freetype/freetype.mk > index b85d0ec..b2d999f 100644 > --- a/package/freetype/freetype.mk > +++ b/package/freetype/freetype.mk > @@ -37,7 +37,7 @@ else > FREETYPE_CONF_OPTS += --without-zlib > endif > > -ifeq ($(BR2_PACKAGE_BZIP2),y) > +ifeq ($(BR2_PACKAGE_FREETYPE_BZIP2),y) > FREETYPE_DEPENDENCIES += bzip2 > FREETYPE_CONF_OPTS += --with-bzip2 > else > -- > 1.8.4.5 > > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
On 18/10/14 04:39, Maarten ter Huurne wrote: > If bzip2 is available, it might still be preferable to not include > support for bzip2 compressed fonts, for example to reduce the number > of libraries loaded when running a GUI application. I don't agree with this. The impact will be really small, both for memory size and for load time - especially because any application using freetype will most likely load much larger GUI libraries. If you're working at such details, you'll unavoidably have to do some manual hacks to get it even better. So I don't think it makes sense to add a config option for it - we try to keep the number of config options in check. So unless you can show me a use case where it makes a real impact, it's a NACK. > > Signed-off-by: Maarten ter Huurne <maarten@treewalker.org> > --- > package/freetype/Config.in | 9 +++++++++ > package/freetype/freetype.mk | 2 +- > 2 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/package/freetype/Config.in b/package/freetype/Config.in > index ded8738..f245b14 100644 > --- a/package/freetype/Config.in > +++ b/package/freetype/Config.in > @@ -4,3 +4,12 @@ config BR2_PACKAGE_FREETYPE > a free, high-quality and portable font engine. > > http://www.freetype.org/ > + > +if BR2_PACKAGE_FREETYPE > + > +config BR2_PACKAGE_FREETYPE_BZIP2 > + bool "support bzip2 compressed fonts" > + depends on BR2_PACKAGE_BZIP2 > + default y In reply to Maxime's comment: it should indeed default to yes, so the default keeps the behaviour we had up to now. Regards, Arnout > + > +endif > diff --git a/package/freetype/freetype.mk b/package/freetype/freetype.mk > index b85d0ec..b2d999f 100644 > --- a/package/freetype/freetype.mk > +++ b/package/freetype/freetype.mk > @@ -37,7 +37,7 @@ else > FREETYPE_CONF_OPTS += --without-zlib > endif > > -ifeq ($(BR2_PACKAGE_BZIP2),y) > +ifeq ($(BR2_PACKAGE_FREETYPE_BZIP2),y) > FREETYPE_DEPENDENCIES += bzip2 > FREETYPE_CONF_OPTS += --with-bzip2 > else >
On Saturday 18 October 2014 15:12:11 Arnout Vandecappelle wrote: > On 18/10/14 04:39, Maarten ter Huurne wrote: > > If bzip2 is available, it might still be preferable to not include > > support for bzip2 compressed fonts, for example to reduce the number > > of libraries loaded when running a GUI application. > > I don't agree with this. The impact will be really small, both for memory > size and for load time - especially because any application using > freetype will most likely load much larger GUI libraries. If you're > working at such details, you'll unavoidably have to do some manual hacks > to get it even better. So I don't think it makes sense to add a config > option for it - we try to keep the number of config options in check. > > So unless you can show me a use case where it makes a real impact, it's a > NACK. The menu we're using (GMenu2X) uses FreeType via SDL_ttf, not via a large GUI toolkit. Dropping the libbzip2 dependency reduces the number of libraries loaded from 12 to 11. I haven't benchmarked how many milliseconds of a difference that makes in startup time. Note that the only place where bzip2 compression is actually supported by FreeType is when opening PCF files. Disabling the feature has no effect on support for other font formats such as TTF. Bye, Maarten
On 19/10/14 02:49, Maarten ter Huurne wrote: > On Saturday 18 October 2014 15:12:11 Arnout Vandecappelle wrote: >> On 18/10/14 04:39, Maarten ter Huurne wrote: >>> If bzip2 is available, it might still be preferable to not include >>> support for bzip2 compressed fonts, for example to reduce the number >>> of libraries loaded when running a GUI application. >> >> I don't agree with this. The impact will be really small, both for memory >> size and for load time - especially because any application using >> freetype will most likely load much larger GUI libraries. If you're >> working at such details, you'll unavoidably have to do some manual hacks >> to get it even better. So I don't think it makes sense to add a config >> option for it - we try to keep the number of config options in check. >> >> So unless you can show me a use case where it makes a real impact, it's a >> NACK. > > The menu we're using (GMenu2X) uses FreeType via SDL_ttf, not via a large > GUI toolkit. Dropping the libbzip2 dependency reduces the number of > libraries loaded from 12 to 11. I haven't benchmarked how many milliseconds > of a difference that makes in startup time. > > Note that the only place where bzip2 compression is actually supported by > FreeType is when opening PCF files. Disabling the feature has no effect on > support for other font formats such as TTF. Okay, let's leave it up to the Benevolent Dictator to make the call. If it's a go, and the depends on is changed into a select like Maxime requested, it gets my Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> Regards, Arnout
>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes: Hi, >>> So unless you can show me a use case where it makes a real impact, it's a >>> NACK. >> >> The menu we're using (GMenu2X) uses FreeType via SDL_ttf, not via a large >> GUI toolkit. Dropping the libbzip2 dependency reduces the number of >> libraries loaded from 12 to 11. I haven't benchmarked how many milliseconds >> of a difference that makes in startup time. >> >> Note that the only place where bzip2 compression is actually supported by >> FreeType is when opening PCF files. Disabling the feature has no effect on >> support for other font formats such as TTF. > Okay, let's leave it up to the Benevolent Dictator to make the call. If it's a > go, and the depends on is changed into a select like Maxime requested, it gets > my Reviewed-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be> Sorry - We discussed it at the Buildroot developer days, and agreed that it it isn't worth the trouble to have an explicit option for this.
diff --git a/package/freetype/Config.in b/package/freetype/Config.in index ded8738..f245b14 100644 --- a/package/freetype/Config.in +++ b/package/freetype/Config.in @@ -4,3 +4,12 @@ config BR2_PACKAGE_FREETYPE a free, high-quality and portable font engine. http://www.freetype.org/ + +if BR2_PACKAGE_FREETYPE + +config BR2_PACKAGE_FREETYPE_BZIP2 + bool "support bzip2 compressed fonts" + depends on BR2_PACKAGE_BZIP2 + default y + +endif diff --git a/package/freetype/freetype.mk b/package/freetype/freetype.mk index b85d0ec..b2d999f 100644 --- a/package/freetype/freetype.mk +++ b/package/freetype/freetype.mk @@ -37,7 +37,7 @@ else FREETYPE_CONF_OPTS += --without-zlib endif -ifeq ($(BR2_PACKAGE_BZIP2),y) +ifeq ($(BR2_PACKAGE_FREETYPE_BZIP2),y) FREETYPE_DEPENDENCIES += bzip2 FREETYPE_CONF_OPTS += --with-bzip2 else
If bzip2 is available, it might still be preferable to not include support for bzip2 compressed fonts, for example to reduce the number of libraries loaded when running a GUI application. Signed-off-by: Maarten ter Huurne <maarten@treewalker.org> --- package/freetype/Config.in | 9 +++++++++ package/freetype/freetype.mk | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-)