[poppler] Toward to JBIG2 support in CairoOutputDev

[suzuki toshiya]

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.

> You could also factor out the CAIRO_MIME_TYPE_UNIQUE_ID code into this
> function to avoid the duplication of code. eg name it
>   setMimeIdFromRef(cairo_surface_t *surface,
>                    const char *mime_type,
>                    Ref ref);
>
> and use it for setting UNIQUE_ID and JBIG2_GLOBAL_ID mime data.

Done.
I replaced JBIG2Stream::globalsStreamRefNum and
JBIG2Stream::globalsStreamRefGen (in my last patch) by
JBIG2Stream::globalsStreamRef.

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?

> +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?

>
> +#define getMimeTypeFromStreamKind( k ) \
> +        ( k == strDCT ? CAIRO_MIME_TYPE_JPEG : \
> +          ( k == strJPX ? CAIRO_MIME_TYPE_JP2 : \
> +            ( k == strJBIG2 ? CAIRO_MIME_TYPE_JBIG2 : \
> +              "" \
> +            ) \
> +          ) \
> +        )
> +
>       st = cairo_surface_set_mime_data (image,
> -				      str->getKind() == strDCT ?
> -				      CAIRO_MIME_TYPE_JPEG : CAIRO_MIME_TYPE_JP2,
> +				      getMimeTypeFromStreamKind(strKind),
>   				      (const unsigned char *)strBuffer,
>
> I don't like the macro for converting stream type to mime type. I
> suggest using a switch statement or lookup table instead.

Done. I introduced a pointer (mime_type) to string constant, and
initialize it when str->getKind() was checked.

>
> -JBIG2Stream::JBIG2Stream(Stream *strA, Object *globalsStreamA):
> +JBIG2Stream::JBIG2Stream(Stream *strA, Object *globalsStreamA, Object
> *globalsStreamRefA):
>     FilterStream(strA)
>   {
>     pageBitmap = NULL;
> @@ -1194,6 +1194,18 @@ JBIG2Stream::JBIG2Stream(Stream *strA, Object
> *globalsStreamA):
>     mmrDecoder = new JBIG2MMRDecoder();
>
>     globalsStreamA->copy(&globalsStream);
> +  if (globalsStreamRefA) {
> +    globalsStreamRefNum = globalsStreamRefA->getRefNum();
> +    globalsStreamRefGen = globalsStreamRefA->getRefGen();
> +  } else {
> +    // According to PDF spec, object reference number 0 is reserved
> +    // to indicate the head & tail of xref, it should not be used
> +    // for the valid reference. We use it to indicate the case that
> +    // JBIG2Stream should be decoded without Globals stream.
> +    globalsStreamRefNum = 0;
> +    globalsStreamRefGen = 0;
> +  }
> +
>
> globalStreamA is never null. I suggest storing the ref in a variable of
> type Ref. Then the code can be:
>
> if (globalsStreamA->isRef())
>    globalsStreamRef = globalsStreamA->getRef();
>
> Instead of using num,gen = 0 to indicate no global, I suggest checking
> getGlobalsStream()->isStream().

Almost done, please see discussion about the initialization
of Ref in above.

>
> +  virtual int getGlobalsStreamRefNum() { return globalsStreamRefNum; }
> +  virtual int getGlobalsStreamRefGen() { return globalsStreamRefGen; }
>
> You can use the Ref type to avoid having two functions to get the ref.
> eg   virtual Ref getGlobalsStreamRef() { return globalsStreamRef; }

Done.

>     Object globalsStream;
> +  int globalsStreamRefNum;	// to make unique ID for JBIG2 Globals in
> CairoOutputDev
> +  int globalsStreamRefGen;	// to make unique ID for JBIG2 Globals in
> CairoOutputDev
>
> No need to mention what the variables are used for outside of this class.

Done.

>
> @@ -336,11 +337,23 @@ Stream *Stream::makeFilter(char *name, Stream
> *str, Object *params, int recursio
>       }
>       str = new FlateStream(str, pred, columns, colors, bits);
>     } else if (!strcmp(name, "JBIG2Decode")) {
> +    obj.free();
>
> If obj is not null at this point it is a bug and the free belongs
> somewhere else. If you just want to ensure obj is null, use obj.initNull().	

Done.

>       if (params->isDict()) {
>         params->dictLookup("JBIG2Globals", &globals, recursion);
> +      if (globals.isStream()) {
> +        params->dictLookupNF("JBIG2Globals", &obj);
> +        XRef *xref = params->getDict()->getXRef();
> +        Object rObj;
> +        while (xref->fetch(obj.getRefNum(), obj.getRefGen(),
> &rObj)->isRef()) {
> +          obj.free();
> +          rObj.copy(&obj);
> +        }
> +        rObj.free();
> +      }
>       }
>
> This would be simpler and easier to understand when written as:
>
> if (params->isDict()) {
>    params->dictLookup("JBIG2Globals", &globals, recursion);
>    while (globals.isRef()) {
>      obj.free();
>      globals.copy(&obj);
>      globals.free();
>      obj.fetch(xref, &globals);
>    }
> }
> str = new JBIG2Stream(str, &globals, &refObj);

Done.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: poppler-cairo-jbig2_20140108a.diff.xz
Type: application/x-xz
Size: 2584 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/poppler/attachments/20150108/8b0f2351/attachment.bin>