Bug 55494

I can confirm that this is necessary for Windows XP.

Can either of you provide me with some #ifdef trick to detect when those are NOT available?  I can then try to make it compile on older systems too.

After some search, it looks like there's no simple way to detect which Windows is used. Plus, the dll will only works for a specific Windows.

I think the most simple way is to use the "getversion" function from Windows and to combine it with a dynamic load library (with LoadLibrary and GetProcAddress).

I don't have time today but I think I will be able to post a patch tomorrow to use these functions.

(In reply to comment #3)
> After some search, it looks like there's no simple way to detect which
> Windows is used. Plus, the dll will only works for a specific Windows.
> 
> I think the most simple way is to use the "getversion" function from Windows
> and to combine it with a dynamic load library (with LoadLibrary and
> GetProcAddress).
> 
> I don't have time today but I think I will be able to post a patch tomorrow
> to use these functions.

Yeah, from what I've seen, GetProcAddress seems like the correct approach.  Though we also use a bunch of structs new in Vista.

Alternatively, maybe I should simply add a configure.ac check to look for the specific OpenType function we use before enabling the Uniscribe backend.

Note that the Uniscribe backend is mostly used for testing, and is NOT supposed to be used in production libraries.  That it is enabled by default right now is just my fault.  Lemme do a simple fix for now.

(In reply to comment #4)
> Yeah, from what I've seen, GetProcAddress seems like the correct approach. 
> Though we also use a bunch of structs new in Vista.
> 
> Alternatively, maybe I should simply add a configure.ac check to look for
> the specific OpenType function we use before enabling the Uniscribe backend.
> 
> Note that the Uniscribe backend is mostly used for testing, and is NOT
> supposed to be used in production libraries.  That it is enabled by default
> right now is just my fault.  Lemme do a simple fix for now.

I'm sure you know harzbuff so much better than me. If you have a simple fix in mind, I think I'll let you do it.

Ok, I pushed out a fix.  Please check.

The fix looks good but the problem is in the usp10.h that mingw have. When you include usp10.h, it also add :

HRESULT WINAPI ScriptShapeOpenType(HDC,SCRIPT_CACHE*,SCRIPT_ANALYSIS*,OPENTYPE_TAG,OPENTYPE_TAG,int*,TEXTRANGE_PROPERTIES**,int,const WCHAR*,int,int,WORD*,SCRIPT_CHARPROP*,WORD*,SCRIPT_GLYPHPROP*,int*);

without something that : #if (_WIN32_WINNT >= 0x0600)
So with an #include <usp10.h> you also include ScriptShapeOpenType despite the version of Windows you have.

I see three solutions for this problem
1) use GetProcAddress
2) always disable uniscribe for Windows
3) add a --without-uniscribe in the configure.ac

I don't know anything about harfbuzz so I don't know what do uniscribe or if it's important. I let you choose the solution you prefer or maybe you have a better one. I can give you a fix for all the three solutions.

Ah, I see.  I thought you are actually building on WinXP.  Ok, so next step is to GetProcAddress it now that we know we are *compiling* on a recent enough SDK at least.  I can look into it in a few days, but if you can get something running to show me how GetProcAddress works, I can finish the patch.

Created attachment 71360 [details]
Call Script*OpenType function with GetProcAddress

Could you check that patch ?
Please, consider I never used GetProcAddress before. Test with care the _hb_uniscribe_shape function still working well under Windows Vista.

Thanks.  That gives me enough to move forward at least.

When building harfbuzz 0.9.11 with the patch in comment 9 i get this build breakage in the tests:

https://gist.github.com/16a31deef71d46068315


Here's the build output copied locally in case that gist link goes down:
[  174s] libtool: link: Could not determine the host path corresponding to
[  174s] libtool: link:   `/home/abuild/rpmbuild/BUILD/harfbuzz-0.9.11/src/.libs:/usr/lib/gcc/i686-w64-mingw32/4.7.2'
[  174s] libtool: link: Continuing, but uninstalled executables may not work.
[  174s] libtool: link: Could not determine the host path corresponding to
[  174s] libtool: link:   `/usr/i686-w64-mingw32/sys-root/mingw/lib:/usr/i686-w64-mingw32/sys-root/mingw/bin:/home/abuild/rpmbuild/BUILD/harfbuzz-0.9.11/src/.libs:/usr/lib/gcc/i686-w64-mingw32/4.7.2'
[  174s] libtool: link: Continuing, but uninstalled executables may not work.
[  175s]   CC     test_c-test-c.o
[  176s] In file included from test-c.c:51:0:
[  176s] ../../src/hb-uniscribe.h:39:98: error: unknown type name 'TEXTRANGE_PROPERTIES'
[  176s] ../../src/hb-uniscribe.h:39:152: error: unknown type name 'SCRIPT_CHARPROP'
[  176s] ../../src/hb-uniscribe.h:39:175: error: unknown type name 'SCRIPT_GLYPHPROP'
[  176s] ../../src/hb-uniscribe.h:40:98: error: unknown type name 'TEXTRANGE_PROPERTIES'
[  176s] ../../src/hb-uniscribe.h:40:98: error: unknown type name 'SCRIPT_CHARPROP'
[  176s] ../../src/hb-uniscribe.h:40:98: error: unknown type name 'SCRIPT_GLYPHPROP'
[  176s] make[3]: *** [test_c-test-c.o] Error 1
[  176s] make[3]: Leaving directory `/home/abuild/rpmbuild/BUILD/harfbuzz-0.9.11/test/api'
[  176s] make[2]: *** [all-recursive] Error 1
[  176s] make[2]: Leaving directory `/home/abuild/rpmbuild/BUILD/harfbuzz-0.9.11/test'
[  176s] make[1]: *** [all-recursive] Error 1
[  176s] make[1]: Leaving directory `/home/abuild/rpmbuild/BUILD/harfbuzz-0.9.11'
[  176s] make: *** [all] Error 2
[  176s] error: Bad exit status from /var/tmp/rpm-tmp.cGb41h (%build)
[  176s] 
[  176s] 
[  176s] RPM build errors:
[  176s]     Bad exit status from /var/tmp/rpm-tmp.cGb41h (%build)
[  177s] /.build/build: line 317:   208 Killed                  background_monitor_process
[  179s] [  152.825799] SysRq : Power Off
[  180s] [  153.055074] Power down.

Do you have the same problem without the patch ?
Because, I don't understand how my patch could give you that kind of error.

(In reply to comment #12)
> Do you have the same problem without the patch ?
> Because, I don't understand how my patch could give you that kind of error.

I believe some of those types are new in Win7+, where ScriptItemizeOpenType was introduced.

As much as I like to fix this bug, it's rather low priority for me right now.  If someone else wants to pick it up, I'd be happy to commit the resulting patchset.  Otherwise, I'm leaving open to get back to it eventually.

(In reply to comment #9)
> Created attachment 71360 [details]
> Call Script*OpenType function with GetProcAddress
> 
> Could you check that patch ?
> Please, consider I never used GetProcAddress before. Test with care the
> _hb_uniscribe_shape function still working well under Windows Vista.

My main comment on the patch is that if the "OpenType" version of the functions are not available, we should fall back to the WinXP version of them (without "OpenType").  If you can cook something like that, it would help me integrating it.

Created attachment 79026 [details] [review]
Trying to use the uniscribe opentype over function pointers and falling back to the non-opentype functions if failed

I just added a patch that prototypes what could be done as a fallback. Please check, cause I don't know how to test whether it does the right thing.

Created attachment 79064 [details] [review]
A better patch that takes address of already loaded module instead of increasing its refcount

We link the usp10.dll, so when the GetProcAddress will be called, the module will be already loaded. And since loading libraries by their name is a security hazard, just use the GetModuleHandle on a module that will already be in memory.

(In reply to comment #14)
> My main comment on the patch is that if the "OpenType" version of the
> functions are not available, we should fall back to the WinXP version of
> them (without "OpenType").  If you can cook something like that, it would
> help me integrating it.

Beghdad, please can you check whether the latest patch cannot be integrated as it corresponds to what you were asking for?

Created attachment 79067 [details] [review]
The above patch, but as git format-patch against harfbuzz master

I'm disabling the Uniscribe backend.  That should "fix" this bug also.

Uniscribe backend is disabled now.  So I think we can close this.

Checking again, I like the patch.  I'll clean up and commit when I have access to my windows machine.

Pushed out a much cleaned up version.