[poppler] Toward to JBIG2 support in CairoOutputDev

[suzuki toshiya]

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&section=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.
>


-------------- next part --------------
A non-text attachment was scrubbed...
Name: poppler-cairo-jbig2_20150111a.diff.xz
Type: application/x-xz
Size: 2596 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/poppler/attachments/20150111/71adc56e/attachment-0001.bin>