[poppler] Openjpeg2 patches: Why de-inline doGetChar?

[Adam Reichold]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Hello,

Am 05.01.2015 um 23:02 schrieb Adam Reichold:
> Hello,
> 
> Am 05.01.2015 um 22:49 schrieb Albert Astals Cid:
>> El Dilluns, 5 de gener de 2015, a les 22:29:22, Adam Reichold va 
>> escriure:
>>> Hello again,
>>> 
>>> Am 05.01.2015 um 21:59 schrieb Albert Astals Cid:
>>>> El Dilluns, 5 de gener de 2015, a les 21:46:32, Adam
>>>> Reichold va
>>>> 
>>>> escriure:
>>>>> Hello again,
>>>>> 
>>>>> suggested changes attached as patch.
>>>> 
>>>> Doesn't compile, init() is not accessible from that static 
>>>> function.
>>> 
>>> Sorry about that, it seems CMake does not find OpenJPEG (1 or
>>> 2) at all on my system with the current master, the library is
>>> found alright but the includes are
>>> "LIBOPENJPEG_INCLUDE_DIR-NOTFOUND" so the file was not compiled
>>> at all. Autoconf on the other hand not problems finding at
>>> least the preferred OpenJPEG version 1.
> 
>> HAve you tried cleaning your cmake build dir? It works fine to
>> me.
> 
> It seems for some reason Arch Linux installs the OpenJPEG header
> only into "/usr/include/openjpeg-1.5/openjpeg.h" and "find_path"
> does not find that, symlinking "/usr/include/openjpeg.h" there
> works fine. (Possibly nobody noticed as the package is built using
> Autoconf?) (Also I have to correct myself, OpenJPEG version 2 is
> found correctly.)

It seems the difference is that "configure.ac" will try pkg-config for
OpenJPEG version 1 first and then fall back to heuristics whereas
"FindLIBOPENJPEG.cmake" does not try pkg-config at all?

Best regards, Adam.

> Best regards, Adam.
> 
>> Cheers, Albert
> 
>>> I also attached a fixed patch that moves the
>>> lazy-initialization into the entry points which also has the
>>> benefit of making the check only once for each call of
>>> 'getChars' but is admittedly uglier. (One solution would be to
>>> move the 'inited' flag into the class proper instead of the
>>> pimpl or even use "priv != NULL" as the initialization
>>> condition, but that felt unnecessarily intrusive.)
>>> 
>>> Best regards, Adam.
>>> 
>>>> Cheers, Albert
>>>> 
>>>>> Best regards, Adam.
>>>>> 
>>>>> Am 05.01.2015 um 21:24 schrieb Adam Reichold:
>>>>>> Hello,
>>>>>> 
>>>>>> Am 04.01.2015 um 23:44 schrieb Albert Astals Cid:
>>>>>>> El Dilluns, 5 de gener de 2015, a les 08:53:26, Adrian
>>>>>>>  Johnson
>>>>>>> 
>>>>>>> va escriure:
>>>>>>>> On 05/01/15 08:39, Albert Astals Cid wrote:
>>>>>>>>> Adrian, any reason you de-inlined doGetChar in the
>>>>>>>>>  patches for openjpeg2?
>>>>>>>>> 
>>>>>>>>> As far as i remember when i made this function
>>>>>>>>> inline it got us a not so bad free speed increase.
>>>>>>>> 
>>>>>>>> When I wrote the patch I moved all openjpeg specific 
>>>>>>>> types and code into the .cc file. I could not figure 
>>>>>>>> out how to make an inline function in the .h access 
>>>>>>>> the JPXStreamPrivate struct that is defined in the
>>>>>>>> .cc file.
>>>>>>> 
>>>>>>> I see :/
>>>>>>> 
>>>>>>> Cheers, Albert
>>>>>> 
>>>>>> Looking at the implemenation, both "doGetChar" and 
>>>>>> "doLookChar" will always be called inside 
>>>>>> "JPEG2000Stream.cc" via the virtual methods "getChar(s)" 
>>>>>> and "lookChar", so I'd say we could just mark the 
>>>>>> implemenations as "inline" as the compiler can still
>>>>>> inline within the compilation unit.
>>>>>> 
>>>>>> Actually, I think there is no compelling reason to have 
>>>>>> those present in the header at all, as they always seem
>>>>>> to access the data via the "priv" pointer anyway, so I'd
>>>>>> say the declarations like
>>>>>> 
>>>>>> static inline int doGetChar(JPXStreamPrivate* priv); 
>>>>>> static inline int doLookChar(JPXStreamPrivate* priv);
>>>>>> 
>>>>>> only within the source file could open even more 
>>>>>> possibilities for compiler optimizations. (You could 
>>>>>> probably also make them inline methods of 
>>>>>> "JPXStreamPrivate" as well, but then the compiler would 
>>>>>> have to infer the effective linkage. Maybe we could put
>>>>>> the whole definition of "JPXStreamPrivate" within the
>>>>>> anonymous namespace anyway?)
>>>>>> 
>>>>>> Best regards, Adam.
>>>>>> 
>>>>>>>>> Cheers,
>>>>>>>>> 
>>>>>>>>> Albert
>>>>>>>>> 
>>>>>>>>> _______________________________________________ 
>>>>>>>>> poppler mailing list poppler at lists.freedesktop.org
>>>>>>>>>  
>>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>
>>>>>>>>> 
_______________________________________________ poppler
>>>>>>>> mailing list poppler at lists.freedesktop.org 
>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>>>>>>
>>>>>>>
>>>>>>>> 
_______________________________________________ poppler
>>>>>>> mailing list poppler at lists.freedesktop.org 
>>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
>>>>>> 
>>>>>> _______________________________________________ poppler 
>>>>>> mailing list poppler at lists.freedesktop.org 
>>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler
> 
>> _______________________________________________ poppler mailing 
>> list poppler at lists.freedesktop.org 
>> http://lists.freedesktop.org/mailman/listinfo/poppler
> 
> _______________________________________________ poppler mailing
> list poppler at lists.freedesktop.org 
> http://lists.freedesktop.org/mailman/listinfo/poppler
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJUqxFcAAoJEPSSjE3STU34hSkIAKcmRY5HVChurpo1UyzCGNfw
nASEqsrwmD0gMnztLSeKPR3wcoAfGyv3i12EaHZxFc9NBoFsOTWgJFL0vVFQpPWJ
0BfE5cqCC63Tfj42Gx33AkMKljz4cRY6HdeZgurwXVIPN1Hvwg+u9n+61tRCUax3
+MRkGWHnvsUDHfNdNZMJ/KHLWXKVZAaJhnNSQ+yMSF8mkyNkW6YiA59rsHFkPl5L
8WFcUPC5ddqGpyvv22EinL/Nl+A9HLGktcEaQd0jmcjq774AxeyA1X1JQcqUrdbm
74mjE90hIOjpcSk3nNfHbzCEF3UqnONzVA5QofzAZ1FlS6gg5hSr96OOzakPtIQ=
=XxTV
-----END PGP SIGNATURE-----