Dear Adrian, OK - considering that there is a lag to the day that Linux distribution ship the poppler stable branch including CairoOutputDev with JBIG2 support even if I support some snapshot of Cairo in current development branch of poppler (and there would be no need to support Cairo snapshot in the day), I decided to follow original version checking.
Also I removed the default value (NULL) for Ref object to JBIG2Stream constructor and removed NULL pointer checking in JBIG2Stream constructor (as same as the argument to pass Globals object). I replaced "st" in CairoOutputDev, by "status". Here I attached revised patch - oh, I'm quite sorry, my previous posts were misunderstanding this year as 2014. Regards, mpsuzuki Adrian Johnson wrote:
On 08/01/15 13:35, suzuki toshiya wrote:Dear Adrian, Great Thank you for prompt review! I attached the revised patch, but please let me discuss a few points. Adrian Johnson wrote:On 07/01/15 02:58, suzuki toshiya wrote:Hi all, Here I submit a preliminary patch for git head, which enables JBIG2 embedding to PDF surface via Cairo.+static cairo_status_t setJBIG2GlobalsIdByRefNums(int refNum, int refGen, + cairo_surface_t *image) +{ + GooString *globalsStrId; + char *idBuffer; + cairo_status_t st; + + + globalsStrId = new GooString; + globalsStrId->appendf("{0:d}-{1:d}", refNum, refGen); + idBuffer = copyString(globalsStrId->getCString()); + st = cairo_surface_set_mime_data (image, CAIRO_MIME_TYPE_JBIG2_GLOBAL_ID, The cairo JBIG2 mime types are new to 1.14.0 so will need to be inside a #if CAIRO_VERSION >= CAIRO_VERSION_ENCODE(1, 14, 0).If it's acceptable, I want to use the conditional like #ifdef CAIRO_MIME_TYPE_JBIG2_GLOBAL_ID The official release of cairo including JBIG2 support is 1.14.0, however, some distributions use a snapshot before 1.14 which supports JBIG2. For example, Ubuntu 14.04 (long time support) ships with 1.13.0~20140204. http://packages.ubuntu.com/search?suite=trusty§ion=all&arch=any&keywords=libcairo2&searchon=names It includes JBIG2 support. Thus I prefer checking by the JBIG2 macros, instead of the version checking.I'm not keen on making an exception for this case. The target audience for this is very small. It would only help users that compile a new version of poppler on an old distribution without also recompiling cairo. JBIG2 support for pdftocairo is a very minor feature that not many people would be using (so far no one else has requested this feature). If you really want to support distributions using a cairo 1.13 snapshot I suggest adding a comment before the first occurrence of #ifdef CAIRO_MIME_TYPE_JBIG2_GLOBAL_ID to explain why the version macro is not used. Also include "#if CAIRO_VERSION >= CAIRO_VERSION_ENCODE(1, 14, 0)" in the comment so that: a) searching for CAIRO_VERSION will find it, and b) to document that JBIG2 support is in 1.14 or later.BTW, when the string for UNIQUE_ID is constructed, its syntax is RefGen-RefNum. In PDF data, RefNum then RefGen is popular syntax. Is there any reason why the order is reverted?No reason. Feel free to change it to num-gen order.+GBool CairoOutputDev::setMimeDataForJBIG2Globals(Stream *str, + cairo_surface_t *image) +{ + JBIG2Stream *jbig2Str = static_cast<JBIG2Stream *>(str); + Object* globalsStr = jbig2Str->getGlobalsStream(); + char *globalsBuffer; + int globalsLength; + + // According to PDF spec, Globals should be stream (or nothing) + + if (globalsStr->isNull()) + return gTrue; + + if (!globalsStr->isStream()) + return gFalse; + + // See JBIG2Stream constructor for valid reference number + if (!jbig2Str->getGlobalsStreamRefNum()) + return gFalse; The globalsStr->isStream() check should be sufficient. The reference number should always be valid of the global stream is valid.Umm. Requesting "both of JBIG2Stream::globalsStream and Ref to it must be always valid as far as GlobalsStream is used" is intuitive solution, but public JBIG2Stream constructor has not requested the Ref to GlobalsStream before in earlier version. If some developers use JBIG2Stream constructor with valid GlobalsStream but without Ref to it (as the syntax before my patch), there is the situation that JBIG2Stream::globalsStream is valid but Ref to it is invalid (if I supply the default value for the argument to pass Ref). How do we detect or prevent such case? A) Supplying the default value (NULL pointer) in the argument to pass the Ref, and JBIG2Stream returns an error when globalsStream is valid stream but its Ref is not passed - and prevent the case that globalsStream is valid but its Ref is invalid. B) Not supplying the default value, and prevent the construction without the Ref to the globalsStream. C) Add new boolean value in JBIG2Stream class, to indicate if Ref to globalsStream is explicitly set. D) Initialize Ref with irregular value (e.g. 0,0 or 0,65535) internally when Ref is not passed, and deal the case as Ref to the globalsStream is not initialized (my last patch design) Which is the best? Or, no need to case such case?JBIG2Stream is not part of the public API so there is no need to preserve compatibility. Something else I just noticed - could you rename "st" to "status". CairoOutputdev.cc always uses the name "status" for cairo status so we should be consistent with this convention.
poppler-cairo-jbig2_20150111a.diff.xz
Description: application/xz
_______________________________________________ poppler mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/poppler
