[poppler] Toward to JBIG2 support in CairoOutputDev

[Adrian Johnson]

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.