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

[Adrian Johnson]

On 06/01/15 08:32, Adam Reichold wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
> 
> 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.)

I don't know much about cmake but looking in cmake/modules it appears
that FindLIBOPENJPEG2.cmake uses pkgconfig but FindLIBOPENJPEG.cmake
does not. That would explain why cmake is unable to find openjpeg1 in a
non standard location. The autoconf build always pkgconfig, with the
openjpeg1 check providing a fallback to a library/header check due to
versions prior to 1.4 not supporting pkgconfig.

> 
> 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
>>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2
> 
> iQEcBAEBCAAGBQJUqwoGAAoJEPSSjE3STU340SgIAMBUIC/PpPpND/m5MQlVhKac
> PVZqwtkXWcKF2xEzwl3bv+xzKQbHYIkuCkUDjvrIzyHCVQjoIzoFDiHyB6PaHLyc
> Yy9WNCwx62LreZDWOiwBkEs2YQsjcSPjMVOrB5uDco4D2Evls+RXyoUOKgwXIwLD
> lvKnzC2O/WO3N1k0IjJUKzAW5bzSEjUaEzO7C8kJOvSnum8exDTdYvQKmG8RTM69
> +qFJwY1T+qqlox53rPa2dmy8utLmeUjaLWt4aGVxjrjDyfsUKjkDgIDl2AKS2pjH
> H/lcsMb/QuhHy4eViJBAB79AF+fVjiYIJkPes5hqRwXG3fC0AGQAJiSbqlO9+NE=
> =/HOF
> -----END PGP SIGNATURE-----
> _______________________________________________
> poppler mailing list
> poppler at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/poppler
>