[poppler] Toward to JBIG2 support in CairoOutputDev

[Adrian Johnson]

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.

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