-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256 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. 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 [email protected] >>>>>> http://lists.freedesktop.org/mailman/listinfo/poppler >>>>> >>>>> _______________________________________________ poppler >>>>> mailing list [email protected] >>>>> http://lists.freedesktop.org/mailman/listinfo/poppler >>>> >>>> _______________________________________________ poppler >>>> mailing list [email protected] >>>> http://lists.freedesktop.org/mailman/listinfo/poppler >>> >>> _______________________________________________ poppler >>> mailing list [email protected] >>> http://lists.freedesktop.org/mailman/listinfo/poppler > > -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJUqwIuAAoJEPSSjE3STU34jEEH/3vsrWvHpVqwB1cEhke7QKw9 YqwarcjtfEdUW/wpn0kiOSKtIDBTx7nqcH+wwyZ1p3J64eWULP/IG59UmLiB3Im9 RoSBgK5QL03BbaD6flHcb99Q9oBvw1Mp8T6DFOO+joQsRbwFLcM8JY00LFaFitdP QH82Ed8n9NAZJJ6Tis1Q2ZWvyN9ceEZTCsohUKKcb4GyBkWlw/ddRIFITw9xtH0y ipMlZrhFXfz2jNN+pe51Zhn/yO/9cWfwf0daLPX5B/L1B4+MjbQMcymX9c7cCMb1 yuadAuEaMhe0kmPzOHjp6Zg3ZvatamN8WrkOW2PyBReg2vBdi6IMBHNwfR7s8R4= =4rQj -----END PGP SIGNATURE-----
>From 486d9290f57097f84acc7a1d34495fa3cbb3f888 Mon Sep 17 00:00:00 2001 From: Adam Reichold <[email protected]> Date: Mon, 5 Jan 2015 21:42:45 +0100 Subject: [PATCH 1/2] Inline 'doGet/LookChar' within 'JPXStream' Make 'doGetChar' and 'doLookChar' static inline freestanding functions instead of private methods to improve compiler optimization opportunities. --- poppler/JPEG2000Stream.cc | 44 ++++++++++++++++++++++---------------------- poppler/JPEG2000Stream.h | 4 ---- 2 files changed, 22 insertions(+), 26 deletions(-) diff --git a/poppler/JPEG2000Stream.cc b/poppler/JPEG2000Stream.cc index 2d48899..beeb9d2 100644 --- a/poppler/JPEG2000Stream.cc +++ b/poppler/JPEG2000Stream.cc @@ -49,6 +49,25 @@ struct JPXStreamPrivate { #endif }; +static inline int doLookChar(JPXStreamPrivate* priv) { + if (unlikely(priv->inited == gFalse)) + init(); + + if (unlikely(priv->counter >= priv->npixels)) + return EOF; + + return ((unsigned char *)priv->image->comps[priv->ccounter].data)[priv->counter]; +} + +static inline int doGetChar(JPXStreamPrivate* priv) { + int result = doLookChar(priv); + if (++priv->ccounter == priv->ncomps) { + priv->ccounter = 0; + ++priv->counter; + } + return result; +} + JPXStream::JPXStream(Stream *strA) : FilterStream(strA) { priv = new JPXStreamPrivate; priv->inited = gFalse; @@ -92,7 +111,7 @@ Goffset JPXStream::getPos() { int JPXStream::getChars(int nChars, Guchar *buffer) { for (int i = 0; i < nChars; ++i) { - const int c = doGetChar(); + const int c = doGetChar(priv); if (likely(c != EOF)) buffer[i] = c; else return i; } @@ -100,30 +119,11 @@ int JPXStream::getChars(int nChars, Guchar *buffer) { } int JPXStream::getChar() { - return doGetChar(); -} - -int JPXStream::doLookChar() { - if (unlikely(priv->inited == gFalse)) - init(); - - if (unlikely(priv->counter >= priv->npixels)) - return EOF; - - return ((unsigned char *)priv->image->comps[priv->ccounter].data)[priv->counter]; + return doGetChar(priv); } int JPXStream::lookChar() { - return doLookChar(); -} - -int JPXStream::doGetChar() { - int result = doLookChar(); - if (++priv->ccounter == priv->ncomps) { - priv->ccounter = 0; - ++priv->counter; - } - return result; + return doLookChar(priv); } GooString *JPXStream::getPSFilter(int psLevel, const char *indent) { diff --git a/poppler/JPEG2000Stream.h b/poppler/JPEG2000Stream.h index 50b7586..7223ce9 100644 --- a/poppler/JPEG2000Stream.h +++ b/poppler/JPEG2000Stream.h @@ -49,10 +49,6 @@ private: void init(); virtual GBool hasGetChars() { return true; } virtual int getChars(int nChars, Guchar *buffer); - - int doGetChar(); - - int doLookChar(); }; #endif -- 2.2.1 >From 85d2b4ecafe20c823b690b718b5cdd67a56d9ce3 Mon Sep 17 00:00:00 2001 From: Adam Reichold <[email protected]> Date: Mon, 5 Jan 2015 22:24:20 +0100 Subject: [PATCH 2/2] Move JPXStream lazy-initialization to entry points To fix the helper methods, we move the lazy initialization of JPXStream into the entry point methods which also has the benifit of doing the check only once for each call of 'getChars'. --- poppler/JPEG2000Stream.cc | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/poppler/JPEG2000Stream.cc b/poppler/JPEG2000Stream.cc index beeb9d2..378be00 100644 --- a/poppler/JPEG2000Stream.cc +++ b/poppler/JPEG2000Stream.cc @@ -50,9 +50,6 @@ struct JPXStreamPrivate { }; static inline int doLookChar(JPXStreamPrivate* priv) { - if (unlikely(priv->inited == gFalse)) - init(); - if (unlikely(priv->counter >= priv->npixels)) return EOF; @@ -60,7 +57,7 @@ static inline int doLookChar(JPXStreamPrivate* priv) { } static inline int doGetChar(JPXStreamPrivate* priv) { - int result = doLookChar(priv); + const int result = doLookChar(priv); if (++priv->ccounter == priv->ncomps) { priv->ccounter = 0; ++priv->counter; @@ -110,6 +107,8 @@ Goffset JPXStream::getPos() { } int JPXStream::getChars(int nChars, Guchar *buffer) { + if (unlikely(priv->inited == gFalse)) { init(); } + for (int i = 0; i < nChars; ++i) { const int c = doGetChar(priv); if (likely(c != EOF)) buffer[i] = c; @@ -119,10 +118,14 @@ int JPXStream::getChars(int nChars, Guchar *buffer) { } int JPXStream::getChar() { + if (unlikely(priv->inited == gFalse)) { init(); } + return doGetChar(priv); } int JPXStream::lookChar() { + if (unlikely(priv->inited == gFalse)) { init(); } + return doLookChar(priv); } @@ -135,8 +138,7 @@ GBool JPXStream::isBinary(GBool last) { } void JPXStream::getImageParams(int *bitsPerComponent, StreamColorSpaceMode *csMode) { - if (priv->inited == gFalse) - init(); + if (unlikely(priv->inited == gFalse)) { init(); } *bitsPerComponent = 8; if (priv->image && priv->image->numcomps == 3) -- 2.2.1
_______________________________________________ poppler mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/poppler
