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.
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.
poppler-cairo-jbig2_20140108a.diff.xz
Description: application/xz
_______________________________________________ poppler mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/poppler
