Re: Bug 687560 "Invalid PDF if /BP pdfmarkswithnon-unique /_objdef"

Dear Igor,

I already did a few changes to the patch, but before posting a new version
we need to agree on some of the points. So, here are some explanations and
questions.

----- Original Message -----
From: "Igor Melichev" <[email protected]>
To: "SaGS" <[email protected]>; <[email protected]>
Sent: Thursday, 4 November 2004 20:01
Subject: Re: [gs-code-review] Fix: Bug 687560 "Invalid PDF if /BP
pdfmarkswithnon-unique /_objdef"


> ...
> First of all, please follow C-style.htm about the code formatting.
> ...

OK. Also got "indent", a 98% friend.


> ...
> 1. The function cos_dict_rename looks some unsmooth
> and it's name isn't enough leading. Could you please
> replace its usage with 2 "traditional" operations :
> remove_entry(old_name),  add_entry(new_name) ?
> (Yes, "rename" saves a few CPU time with
> memory allocations, but it doesn't looks so important optimization).
> Or at least cos_dict_rename to be renamed with
> cos_dict_rename_entry.

(1.A) The name "cos_dict_rename_entry" is fine with me. I considered
cos_dict_rename[_entry]() can be reused when (if ever) other pdfmarks will
be fixed to deal with duplicate distill-time names. (Haven't looked closely,
but it looks this is somehow random at present: /ANN raises an error, /OBJ
implementation is entirely "original" and /NI creates an invalid PDF the
same way as /BP and /PS do at present.)

(1.B) There are already plenty of "put()". If you think that a "remove()"
will be much more useful, then that's fine with me too and I'll change the
code. I suggest the name "cos_dict_unadd()", as its array counterpart. The
function will remove an entry from the dictionary, free the key if
necessary, and return the value (the value will NOT be freed).

Please decide between (1.A) and (1.B).


> 2. I'm unclear with the implementation of the branch
> PDF_NAMECOLLISION_REBUILD.
> Do you really need the "surgery" ?
> I would recommend to do it as this :
> 2.1. Create a new object with pdf_enter_substream;
> 2.2. Copy all useful keys from the old cos dict to the new one;
> 2.3. Remove the old object from the named object list,
> and add the new one;
> 2.4. Cancel and free the old object.
> Note that cos_write_stream_redir, cos_stream_move_all become unnecessary.
> (Well, my suggestion won't work if there are references to
> the old object from others. But in this case your code
> doesn't work also).

Let not say "reference", I consider this word too imprecise. It can mean:
(a) A C pointer. I'll name these "C-ptrs";
(b) References based exclusively on object numbers, those "n 0 R" or "/Rn".
I'll name these "n 0 Refs".
(c) The construct "{OBJNAME}" when it appears in the value part of a
key-value pair and the key is anything except /_objdef. I'll name these
"{OBJRefs}". (These appear only in the source PostScript code, and are
converted internally to "n 0 Refs", but that's not an important fact here.)

With the latest version of the patch, "C-ptrs" remain valid because neither
the old cos_object_t nor the old pdf_resource_t are freed or reallocated,
only their contents may change. Also, the object number and name remain, so
"n 0 Refs" and "{OBJRefs}" are OK too. What exactly makes you believe "in
this case [the] code doesn't work also"?
Notes:
- With the preceding version of the patch, "C-ptrs" didn't survive a
PDF_NAMECOLLISION_REBUILD;
- While I didn't find a "C-ptr" to an XObject, I'm not confident that such
pointers don't exist, nor that they won't be used in the future; for this
reason, I prefer not to change the patch to conform to 2.1 ... 2.4 above.

Regarding the "surgery", I don't like the idea either, because it's a source
of errors. In fact, in the meantime I discovered that, even if the patient
did not die, the surgery was incomplete,
pdev->accumulating_substream_resource needing to be patched too.

Given this situation, I think the options are:
(2.A) Change pdf_alloc_aside() as it was in the preceding version of the
patch. It will continue to output an "aside", but the added "pcofwd"
parameter allows the caller, by setting this to !=NULL, to feed-in whatever
pieces of the future "aside" are already available. This way, the right
object is used in the first place, all those intricate connections will be
made to the right object, and "surgery" will be unnecessary.
(pdf_enter_substream() and pdf_open-aside() will also need this extra
parameter, but their logic doesn't really change, they only pass this
parameter to the next function.) A VARIATION of this: don't add an extra
parameter, but make the existing "ppres" to be an in/out parameter rather
than an out parameter. (In this 2nd case, I identified a few places in 4
files - including gdevpdfm.c - where *ppres needs to be initialized to 0.)

(2.B) Let the "surgery" as-is (with the rectification about
pdev->accumulating_substream_resource I mentioned).

Please take a decision on this.


>
> 3. Please note that recently I defined pdf_forget_resource (see
> http://www.ghostscript.com/pipermail/gs-cvs/2004-November/004992.html),
> which may be useful for your code.
> ...

OK, it seems I can simply replace pdf_cancel_resource() with this one
(except it returns void, not a "code").


> 4. It would be useful to group (factor out)
> the call to pdf_enter_substream
> and pdev->substream_Resources = cos_dict_alloc(...)
> into a special local function pdf_enter_BP
> (Well, I'm not in favor with the name pdf_enter_BP,
> fell free to improve it). The purposes are :
> 4.1. do not distribute redundant specifics of BP through GS code;
> 4.2. Simplify the view of the big operator "switch" you introduced;

OK. For the name, I'll choose "pdf_enter_XObject" (being used not only for
/BP, but for /PS too), unless you have another preference.

> 4.3. Move /* Have no better switch*/ to a single place.

While writing the new pdf_enter_XObject(), I noticed an unused parameter of
start_XObject(). It looks we do have a better choice: the "compress" option
passed from the caller. I'll use this, unless you find a reason not to. It
is set by the caller to pdev->CompressPages, not pdev->CompressFonts, which
looks more appropriate; and anyway it's a good idea to let the caller decide
on compression. If this change is not OK with you, it's the moment to say
so.


> 5. You coded :
>
> +           if (!(oper &PDF_NAMECOLLISION_NONAME))
> +               code = cos_dict_put(pdev->local_named_objects,
> objname->data,
> +                                   objname->size,
cos_object_value(&value,
> pres->object));
> +           else
> +               DO_NOTHING;
> +           if (code < 0)
> +               return code;
> ...

Stupid mistake, sorry.


Sincerely yours,
SaGS