[poppler] Toward to JBIG2 support in CairoOutputDev
suzuki toshiya
mpsuzuki at hiroshima-u.ac.jp
Mon Jan 19 07:49:36 PST 2015
Dear Carlos,
Thank you for finding out the points! I attached revised patch.
The difference from my previous revision is following.
Regards,
mpsuzuki
diff --git a/poppler/CairoOutputDev.cc b/poppler/CairoOutputDev.cc
index a428d2c..17b6b3b 100644
--- a/poppler/CairoOutputDev.cc
+++ b/poppler/CairoOutputDev.cc
@@ -2833,13 +2833,12 @@ void CairoOutputDev::setMimeData(GfxState *state, Stream *str, Object *ref,
#endif
if (getStreamData (str->getNextStream(), &strBuffer, &len)) {
- cairo_status_t status;
+ cairo_status_t status = CAIRO_STATUS_SUCCESS;
#if CAIRO_VERSION >= CAIRO_VERSION_ENCODE(1, 11, 2)
if (ref && ref->isRef()) {
- Ref imgRef = ref->getRef();
status = setMimeIdFromRef(image, CAIRO_MIME_TYPE_UNIQUE_ID,
- "poppler-surface-", imgRef);
+ "poppler-surface-", ref->isRef());
}
#endif
if (!status) {
Carlos Garcia Campos wrote:
> Adrian Johnson <ajohnson at redneon.com> writes:
>
>> The latest patch looks good. I'll wait a week before committing to see
>> if anyone else has any comments. This will give me time to do some
>> testing of the patch.
>
> Looks good to me as well in general. There's only one thing I've
> noticed.
>
>> @@ -2746,31 +2827,28 @@ void CairoOutputDev::setMimeData(GfxState *state, Stream *str, Object *ref,
>> if (!colorMapHasIdentityDecodeMap(colorMap))
>> return;
>>
>> +#if CAIRO_VERSION >= CAIRO_VERSION_ENCODE(1, 14, 0)
>> + if (strKind == strJBIG2 && !setMimeDataForJBIG2Globals(str, image))
>> + return;
>> +#endif
>> +
>> if (getStreamData (str->getNextStream(), &strBuffer, &len)) {
>> - cairo_status_t st;
>> + cairo_status_t status;
>>
>> #if CAIRO_VERSION >= CAIRO_VERSION_ENCODE(1, 11, 2)
>> if (ref && ref->isRef()) {
>> Ref imgRef = ref->getRef();
>> - GooString *surfaceId = new GooString("poppler-surface-");
>> - surfaceId->appendf("{0:d}-{1:d}", imgRef.gen, imgRef.num);
>> - char *idBuffer = copyString(surfaceId->getCString());
>> - st = cairo_surface_set_mime_data (image, CAIRO_MIME_TYPE_UNIQUE_ID,
>> - (const unsigned char *)idBuffer,
>> - surfaceId->getLength(),
>> - gfree, idBuffer);
>> - if (st)
>> - gfree(idBuffer);
>> - delete surfaceId;
>> + status = setMimeIdFromRef(image, CAIRO_MIME_TYPE_UNIQUE_ID,
>> + "poppler-surface-", imgRef);
>
> We could pass ref->getRef() directly here.
>
>> }
>> #endif
>> + if (!status) {
>
> status might be uninitialized at this point if cairo version is not
> recent enough or the ref && ref->isRef() condition is False.
>
>> + status = cairo_surface_set_mime_data (image, mime_type,
>> + (const unsigned char *)strBuffer, len,
>> + gfree, strBuffer);
>> + }
>
>> - st = cairo_surface_set_mime_data (image,
>> - str->getKind() == strDCT ?
>> - CAIRO_MIME_TYPE_JPEG : CAIRO_MIME_TYPE_JP2,
>> - (const unsigned char *)strBuffer, len,
>> - gfree, strBuffer);
>> - if (st)
>> + if (status)
>> gfree (strBuffer);
>> }
>> }
>
>
>> On 11/01/15 23:05, suzuki toshiya wrote:
>>> 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 mailing list
>> poppler at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/poppler
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: poppler-cairo-jbig2_20150119.diff.xz
Type: application/octet-stream
Size: 2608 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/poppler/attachments/20150120/9e87ecd2/attachment.obj>
More information about the poppler
mailing list