DBus font installs [incomplete]
Deepak Bhole
dbhole at redhat.com
Thu Apr 14 12:32:55 PDT 2011
Please re-post this patch with proper docs for all of the functions and
comments on what the non-obvious lines of code are doing, especially
when they call external API.
Comments on generic issues below:
* Andrew Su <asu at redhat.com> [2011-03-02 13:35]:
> Hello,
>
> This patch is an attempt at requesting users to install a font that will allow them to display the text being rendered if the glyph is missing. It is still a work in progress.
>
> Known issues:
> - Segmentation fault when rechecking for newly installed fonts to load.
This is a showstopper. Nothing we add should make the JVM crash under
any circumstances.
> - If characters from different languages are being rendered one at a time it would constantly request user to install the languages not found.
Can't we maintain a list and not ask back if the user said no once?
> - May ask to install a non-standard language (unrecognised by packagekit).
>
What will packagekit try to install if it is non-standard?
> Attached:
> - addFontInstall.patch: This will add the patch and modify the Makefile accordingly.
> - GlyphChecker.c: Used to create libglyphchecker.so (which should be placed in the openjdk's lib directory.
> - GlyphChecker.h: Header for using jni.
>
> I used [on a x86_64 machine]
>
> gcc -I/usr/lib/jvm/java-1.6.0-openjdk-1.6.0.0.x86_64/include/ -I/usr/lib/jvm/java-1.6.0-openjdk-1.6.0.0.x86_64/include/linux/ -I/usr/include/dbus-1.0/ -I/usr/include/glib-2.0/ -I/usr/lib64/glib-2.0/include -shared -fPIC -g -O0 -Wall -lfontconfig -ldbus-glib-1 -o libglyphchecker.so GlyphChecker.c
>
> to build the library file. (Replace with where your libs are..) once compiled, place the .so into your jre's lib dir.
> ex: ${BUILD_DIR}/openjdk.build/j2sdk-image/jre/lib/amd64/
>
> To test that it searches and installs fonts..
> You can run it as follows:
> GlyphChecker.addMissing((char) 0x710);
> GlyphChecker.doAll(); // This should return true if you accepted to install
>
> Please let me know if you see any issues with it.
>
...
> #include <fontconfig/fontconfig.h>
> #include <stdlib.h>
> #include <dlfcn.h>
>
> FcChar8* getSuperSet(FcLangSet* fls) {
> // printf("Getting superset.\n");
>
Above should be removed. As should all other uncommented printfs
> FcChar8* s = NULL;
> FcStrSet* ss = FcLangSetGetLangs(fls);
> FcStrList* sl = FcStrListCreate(ss);
> FcChar8* name = FcStrListNext(sl);
> const FcCharSet* finalSet = NULL;
> if (name != NULL)
> finalSet = FcLangGetCharSet(name);
>
> while ((s = FcStrListNext(sl))) {
> const FcCharSet* fs = FcLangGetCharSet(s);
> if (FcCharSetIsSubset(finalSet, fs)) {
> FcCharSetDestroy((FcCharSet*) finalSet);
> finalSet = fs;
> FcStrFree(name);
> name = FcStrCopy(s);
> } else {
> FcCharSetDestroy((FcCharSet*) fs);
> }
> FcStrFree(s);
Should you be freeing the string? Man page for FcStrListNext does not
talk about caller having to free the return reference.
...
...
> FcChar32* dst = (FcChar32*) malloc(sizeof(FcChar32) * 4); // This should be enough as UTF-32 uses 4 bytes to encode each character.
> FcLangSet* completeSet = FcLangSetCreate();
>
> // printf("Character is %s\n", characters);
> // printf("Length: %d\n", FcUtf8ToUcs4((FcChar8*) characters, dst, len));
> FcUtf8ToUcs4((FcChar8*) characters, dst, len);
> while ((s = FcStrListNext(sl))) {
> const FcCharSet* fs = FcLangGetCharSet(s);
> if (FcCharSetHasChar(fs, *dst)) {
> // printf("Found in language %s\n", (char*) s);
> FcLangSetAdd(completeSet, s); // Create our pool of languages that use this.
> found = 1;
> }
> FcCharSetDestroy((FcCharSet*) fs);
> }
>
dst is leaking memory as it is never freed.
> FcStrListDone(sl);
> FcStrSetDestroy(ss);
> if (!found) {
> // fprintf(stderr, "Font did not match any language's character set.\n");
> return NULL;
> }
>
> return (char*) FcStrPlus((FcChar8*) ":lang=", getSuperSet(completeSet));
getSuperSet could return NULL (is this the SEGV you mentioned above?).
> }
>
> JNIEXPORT jobjectArray JNICALL
> Java_org_classpath_icedtea_font_GlyphChecker_getLangs(JNIEnv *env,
> jobject self, jobjectArray sArr, jintArray iArr) {
>
> int i;
> int len = (*env)->GetArrayLength(env, sArr);
> jobjectArray ret = (*env)->NewObjectArray(env, len, (*env)->FindClass(env,
> "java/lang/String"), NULL);
>
> jint *lens = (*env)->GetIntArrayElements(env, iArr, 0);
> for (i = 0; i < len; i++) {
> jstring s = (jstring)(*env)->GetObjectArrayElement(env, sArr, i);
>
> char* lang = checkChar(env, s, lens[i]);
checkChar is allocating the memory for lang. lang is never freed though.
...
...
>
> connection = dbus_g_bus_get(DBUS_BUS_SESSION, NULL);
Second param to dbus_g_bus_get is an error store. What if an error
happens above? DBus may not be running in the bg. In fact if it isn't,
we should return gracefully from here and just let the VM pick the font
to draw.
> proxy = dbus_g_proxy_new_for_name(connection, "org.freedesktop.PackageKit",
> "/org/freedesktop/PackageKit", "org.freedesktop.PackageKit.Modify");
>
> ret = dbus_g_proxy_call_with_timeout(proxy, "InstallFontconfigResources",
> INT_MAX, &error, G_TYPE_UINT, 0, G_TYPE_STRV, asFont,
> G_TYPE_STRING, "", G_TYPE_INVALID, G_TYPE_INVALID);
>
I think we should have a timeout rather than an infinite wait on user
input.
> free(asFont);
>
It is good practice to set a var (asFont above) to NULL after freeing.
> if (!ret) {
> g_warning("failed: %s", error->message);
> g_error_free(error);
> return 1;
> }
> return 0;
> }
>
> JNIEXPORT jint JNICALL
> Java_org_classpath_icedtea_font_GlyphChecker_libExist(JNIEnv *env, jobject self) {
Perhaps we can extend this method to also check if dbus is running and
if we can connect to it? That way we don't have to go through the pain
of searching for a font unnecessarily.
> diff -r 29c90e090d7b Makefile.am
> --- a/Makefile.am Wed Mar 02 18:41:37 2011 +0100
> +++ b/Makefile.am Wed Mar 02 13:01:52 2011 -0500
> @@ -327,7 +327,8 @@
> patches/openjdk/6766342-AA-simple-shape-performance.patch \
> patches/openjdk/7016856-pisces-performance.patch \
> patches/openjdk/6934977-MappedByteBuffer.load.patch \
> - patches/jaxp-serial-version-uid.patch
> + patches/jaxp-serial-version-uid.patch \
> + patches/fontinstall.patch
>
> if !WITH_ALT_HSBUILD
> ICEDTEA_PATCHES += \
That patch should probably be renamed to something more descriptive.
> ++ *
> ++ * Do those will try it on
> ++ * yum install wqy-unibit-fonts gnu-free-sans-fonts gnu-free-fonts-common -y
> ++ * yum remove wqy-unibit-fonts gnu-free-sans-fonts gnu-free-fonts-common -y
> ++ *
Not sure what the above comment means :)
> ++ *
> ++ */
> ++public class GlyphChecker {
> ++ private static final HashSet<String> asked = new HashSet<String>();
> ++ private static final HashSet<String> pending = new HashSet<String>();
> ++ private static final HashSet<String> missingChars = new HashSet<String>(); // Each entry should only be 1 character.
Above could be modified from different threads can't they? Is so,
consider using ConcurreptMap/ConcurrentHashMap or use
Collections.synchronizedSet.
> ++ private static SunGraphicsEnvironment sgEnv = null;
> ++ private static HashSet<String> temp = null;
> ++
> ++
> ++ static {
> ++
> ++ System.loadLibrary("glyphchecker");
> ++
> ++
> ++
> ++ }
> ++
Why all the extra spaces?
> ++ public static boolean doAll() {
> ++
Do what all? Also, this is a publicly exposed method. Does it need to
be?
> ++ if (libExist() != 0) return false;
> ++ getSunGraphicsEnvironment();
> ++
> ++ // We can also completely skip this, and not reask if they choose not to accept.
> ++ switch (ret) {
> ++ case 0:
> ++ System.out.println("it Worked!"); // TODO: Remove before commit.
Should be removed!
> ++ private static void cleanPending() {
> ++ if (temp == null) temp = new HashSet<String>();
> ++ temp.clear();
> ++ for (String s : pending) {
> ++ if (s != null) {
> ++ String tmp = s.split("-", 2)[0].toLowerCase();
> ++ if (!asked.contains(s)) temp.add(tmp);
> ++ }
> ++ }
> ++ pending.clear();
> ++ pending.addAll(temp);
> ++ }
> ++
temp is only used in this function. It should be in local scope only.
> ++ private static boolean haveMissing() {
> ++ switch (missingChars.size()) {
> ++ case 0:
> ++ return false;
> ++ }
> ++ return true;
> ++ }
> ++
> ++ private static boolean havePending() {
> ++ synchronized (asked) {
> ++ pending.removeAll(asked);
> ++ }
You are locking on the instance that will be read (asked), but not on
the instance what will be written to...
> +
> ++ // Must always be set back to false after changing.
> ++ public static boolean recheckFont = false;
> ++
I can't see anything using recheckFont
...
> +@@ -83,6 +102,7 @@
> + char data[], int offset, int length,
> + int ix, int iy)
> + {
> ++ Font f = GlyphChecker.dupeFont(sg2d.getFont());
> + FontInfo info = sg2d.getFontInfo();
> + float x, y;
> + if (info.pixelHeight > OutlineTextRenderer.THRESHHOLD) {
> +@@ -101,6 +121,20 @@
> + }
> + GlyphList gl = GlyphList.getInstance();
> + if (gl.setFromChars(info, data, offset, length, x, y)) {
> ++ if (GlyphChecker.doAll()) {
> ++ gl.dispose();
> ++ GlyphChecker.getSunGraphicsEnvironment().recheckForNewFonts();
> ++ try {
> ++ sg2d.setFont(f); // need to reset font and search since it will change to default.
> ++ Font2DHandle h = FontManager.findFont2D(f.getName(), f.getStyle(), FontManager.LOGICAL_FALLBACK).handle;
> ++ FontManager.setFont2D(f, h);
> ++ } catch (NullPointerException npe) {
> ++ // Do nothing proceed like nothing happened.
> ++ }
> ++ info = sg2d.getFontInfo();
> ++ gl = GlyphList.getInstance();
> ++ gl.setFromChars(info, data, offset, length, x, y);
> ++ }
> + drawGlyphList(sg2d, gl);
> + gl.dispose();
This block looks very similar to the one above. Are the identical? If
so, they should be in a function.
> + } else {
> +diff -r 9d90acd9f99f src/solaris/native/sun/awt/fontpath.c
> +--- openjdk/jdk/src/solaris/native/sun/awt/fontpath.c Wed Feb 16 09:39:35 2011 -0500
> ++++ openjdk/jdk/src/solaris/native/sun/awt/fontpath.c Wed Mar 02 09:24:59 2011 -0500
> +@@ -517,11 +517,11 @@
> + }
> +
> + JNIEXPORT jstring JNICALL Java_sun_font_FontManager_getFontPath
> +-(JNIEnv *env, jclass obj, jboolean noType1) {
> ++(JNIEnv *env, jclass obj, jboolean noType1, jboolean recheck) {
We should find some other way to recheck (whatever it is doing).
Changing a public api, even for a sun.* class is a no-no.
Deepak
More information about the distro-pkg-dev
mailing list