Refactor Dictionary writing code

Original code was written before we could shallow copy objects, so all the filtering was done by suppressing the output of certain keys and replacing them with other keys. Now we can simplify the code greatly by modifying shallow copies of dictionaries in place.

Refactor Dictionary writing code
Original code was written before we could shallow copy objects, so all the filtering was done by suppressing the output of certain keys and replacing them with other keys. Now we can simplify the code greatly by modifying shallow copies of dictionaries in place.
Jay Berkenbilt
1 parent e57c2581
Showing 8 changed files with 97 additions and 109 deletions
include/qpdf/QPDFWriter.hh
libqpdf/QPDFWriter.cc
qpdf/qpdf.testcov
qpdf/qtest/qpdf.test
qpdf/qtest/qpdf/extensions-adbe-force-1.8.5.qdf
qpdf/qtest/qpdf/extensions-adbe-other-force-1.8.5.qdf
qpdf/qtest/qpdf/extensions-none-force-1.8.5.qdf
qpdf/qtest/qpdf/extensions-other-force-1.8.5.qdf
@@ -271,7 +271,6 @@ class QPDFWriter
     static int const f_stream = 	1 << 0;
     static int const f_filtered =	1 << 1;
     static int const f_in_ostream =     1 << 2;
-    static int const f_in_extensions =  1 << 3;
     enum trailer_e { t_normal, t_lin_first, t_lin_second };
@@ -1135,7 +1135,8 @@ QPDFWriter::unparseObject(QPDFObjectHandle object, int level,
 			  unsigned int flags, size_t stream_length,
                           bool compress)
 {
-    unsigned int child_flags = flags & ~f_stream & ~f_in_extensions;
+    int old_id = object.getObjectID();
+    unsigned int child_flags = flags & ~f_stream;
     std::string indent;
     for (int i = 0; i < level; ++i)
@@ -1167,10 +1168,19 @@ QPDFWriter::unparseObject(QPDFObjectHandle object, int level,
     }
     else if (object.isDictionary())
     {
+        // Make a shallow copy of this object so we can modify it
+        // safely without affecting the original.  This code makes
+        // assumptions about things that are made true in
+        // prepareFileForWrite, such as that certain things are direct
+        // objects so that replacing them doesn't leave unreferenced
+        // objects in the output.
+        object = object.shallowCopy();
+
         // Handle special cases for specific dictionaries.
-        // Extensions dictionaries are complicated.  We have one of
-        // several cases:
+        // Extensions dictionaries.
+
+        // We have one of several cases:
         //
         // * We need ADBE
         //    - We already have Extensions
@@ -1183,16 +1193,16 @@ QPDFWriter::unparseObject(QPDFObjectHandle object, int level,
         //       - If it has other things, keep those and remove ADBE
         //    - We have no extensions: no action required
         //
-        // We may be in the root dictionary, or we may be inside the
-        // extensions dictionary itself.  The latter is determined by
-        // the presence of the f_in_extensions flag.
+        // Before writing, we guarantee that /Extensions, if present,
+        // is direct through the ADBE dictionary, so we can modify in
+        // place.
         bool is_root = false;
         bool have_extensions_other = false;
         bool have_extensions_adbe = false;
         QPDFObjectHandle extensions;
-        if (object.getObjectID() == pdf.getRoot().getObjectID())
+        if (old_id == pdf.getRoot().getObjectID())
         {
             is_root = true;
             if (object.hasKey("/Extensions") &&
@@ -1201,10 +1211,7 @@ QPDFWriter::unparseObject(QPDFObjectHandle object, int level,
                 extensions = object.getKey("/Extensions");
             }
         }
-        else if (flags & f_in_extensions)
-        {
-            extensions = object;
-        }
+
         if (extensions.isInitialized())
         {
             std::set<std::string> keys = extensions.getKeys();
@@ -1221,10 +1228,6 @@ QPDFWriter::unparseObject(QPDFObjectHandle object, int level,
         bool need_extensions_adbe = (this->final_extension_level > 0);
-        bool write_new_extensions = false;
-        bool write_new_adbe = false;
-        bool suppress_existing_extensions = false;
-        bool suppress_existing_adbe = false;
         if (is_root)
         {
             if (need_extensions_adbe)
@@ -1235,26 +1238,23 @@ QPDFWriter::unparseObject(QPDFObjectHandle object, int level,
                     // it here.
                     QTC::TC("qpdf", "QPDFWriter create Extensions",
                             this->qdf_mode ? 0 : 1);
-                    write_new_extensions = true;
-                    suppress_existing_extensions = true;
-                }
-                else
-                {
-                    // Preserve existing Extensions and do the work
-                    // in the extensions dictionary.
+                    extensions = QPDFObjectHandle::newDictionary();
+                    object.replaceKey("/Extensions", extensions);
                 }
             }
             else if (! have_extensions_other)
             {
                 // We have Extensions dictionary and don't want one.
-                suppress_existing_extensions = true;
                 if (have_extensions_adbe)
                 {
                     QTC::TC("qpdf", "QPDFWriter remove existing Extensions");
+                    object.removeKey("/Extensions");
+                    extensions = QPDFObjectHandle(); // uninitialized
                 }
             }
         }
-        else if (flags & f_in_extensions)
+
+        if (extensions.isInitialized())
         {
             QTC::TC("qpdf", "QPDFWriter preserve Extensions");
             QPDFObjectHandle adbe = extensions.getKey("/ADBE");
@@ -1272,77 +1272,54 @@ QPDFWriter::unparseObject(QPDFObjectHandle object, int level,
             }
             else
             {
-                suppress_existing_adbe = true;
                 if (need_extensions_adbe)
                 {
-                    write_new_adbe = true;
+                    extensions.replaceKey(
+                        "/ADBE",
+                        QPDFObjectHandle::parse(
+                            "<< /BaseVersion /" + this->final_pdf_version +
+                            " /ExtensionLevel " +
+                            QUtil::int_to_string(this->final_extension_level) +
+                            " >>"));
+                }
+                else
+                {
+                    QTC::TC("qpdf", "QPDFWriter remove ADBE");
+                    extensions.removeKey("/ADBE");
                 }
             }
         }
-	writeString("<<");
-	writeStringQDF("\n");
+        // Stream dictionaries.
-        if (write_new_extensions || write_new_adbe)
+        if (flags & f_stream)
         {
-            writeStringQDF(indent);
-            writeStringQDF("  ");
-            writeStringNoQDF(" ");
-            if (write_new_extensions)
-            {
-                writeString("/Extensions << ");
-            }
-            writeString("/ADBE << /BaseVersion /");
-            writeString(this->final_pdf_version);
-            writeString(" /ExtensionLevel ");
-            writeString(QUtil::int_to_string(this->final_extension_level));
-            writeString(" >>");
-            if (write_new_extensions)
+            // Suppress /Length since we will write it manually
+            object.removeKey("/Length");
+
+            // XXX BUG: /Crypt filters should always be removed.
+	    if (flags & f_filtered)
             {
-                writeString(" >>");
+                object.removeKey("/Filter");
+                object.removeKey("/DecodeParms");
             }
-            writeStringQDF("\n");
         }
+	writeString("<<");
+	writeStringQDF("\n");
+
 	std::set<std::string> keys = object.getKeys();
 	for (std::set<std::string>::iterator iter = keys.begin();
 	     iter != keys.end(); ++iter)
 	{
-	    // I'm not fully clear on /Crypt keys in /DecodeParms.  If
-	    // one is found, we refuse to filter, so we should be
-	    // safe.
 	    std::string const& key = *iter;
-	    if ((flags & f_filtered) &&
-		((key == "/Filter") ||
-		 (key == "/DecodeParms")))
-	    {
-		continue;
-	    }
-	    if ((flags & f_stream) && (key == "/Length"))
-	    {
-		continue;
-	    }
-
-            bool is_extensions = (is_root && (key == "/Extensions"));
-            if (suppress_existing_extensions && is_extensions)
-            {
-                QTC::TC("qpdf", "QPDFWriter skip Extensions");
-                continue;
-            }
-            if (suppress_existing_adbe && (key == "/ADBE"))
-            {
-                QTC::TC("qpdf", "QPDFWriter skip ADBE");
-                continue;
-            }
-
 	    writeStringQDF(indent);
 	    writeStringQDF("  ");
 	    writeStringNoQDF(" ");
 	    writeString(QPDF_Name::normalizeName(key));
 	    writeString(" ");
-            unparseChild(object.getKey(key), level + 1,
-                         child_flags | (is_extensions ? f_in_extensions : 0));
+            unparseChild(object.getKey(key), level + 1, child_flags);
 	    writeStringQDF("\n");
 	}
@@ -1379,7 +1356,6 @@ QPDFWriter::unparseObject(QPDFObjectHandle object, int level,
     else if (object.isStream())
     {
 	// Write stream data to a buffer.
-	int old_id = object.getObjectID();
 	int new_id = obj_renumber[old_id];
 	if (! this->direct_stream_lengths)
 	{
@@ -249,9 +249,8 @@ QPDFWriter make Extensions direct 0
 QPDFWriter make ADBE direct 1
 QPDFWriter preserve Extensions 0
 QPDFWriter create Extensions 1
-QPDFWriter skip ADBE 0
+QPDFWriter remove ADBE 0
 QPDFWriter remove existing Extensions 0
-QPDFWriter skip Extensions 0
 QPDFWriter preserve ADBE 0
 QPDF_encryption skip 0x28 0
 QPDF_encrypt crypt array 0
@@ -134,7 +134,7 @@ foreach my $input (@ext_inputs)
                               "qpdf --static-id" .
                               " --$op-version=$version $input a.pdf"},
                          {$td->STRING => "", $td->EXIT_STATUS => 0});
-            $td->runtest("check version information",
+            $td->runtest("check version information ($op $version)",
                          {$td->COMMAND => "test_driver 34 a.pdf"},
                          {$td->FILE => "$base-$op-$version.out",
                           $td->EXIT_STATUS => 0},
@@ -6,7 +6,10 @@
 1 0 obj
 <<
   /Extensions <<
-    /ADBE << /BaseVersion /1.8 /ExtensionLevel 5 >>
+    /ADBE <<
+      /BaseVersion /1.8
+      /ExtensionLevel 5
+    >>
   >>
   /Pages 2 0 R
   /Type /Catalog
@@ -88,17 +91,17 @@ xref
 0 8
 0000000000 65535 f 
 0000000052 00000 n 
-0000000207 00000 n 
-0000000316 00000 n 
-0000000558 00000 n 
-0000000657 00000 n 
-0000000703 00000 n 
-0000000848 00000 n 
+0000000223 00000 n 
+0000000332 00000 n 
+0000000574 00000 n 
+0000000673 00000 n 
+0000000719 00000 n 
+0000000864 00000 n 
 trailer <<
   /Root 1 0 R
   /Size 8
   /ID [<e42c124696c09bd2cacaf7196e9c88a0><31415926535897932384626433832795>]
 >>
 startxref
-883
+899
 %%EOF
@@ -6,7 +6,10 @@
 1 0 obj
 <<
   /Extensions <<
-    /ADBE << /BaseVersion /1.8 /ExtensionLevel 5 >>
+    /ADBE <<
+      /BaseVersion /1.8
+      /ExtensionLevel 5
+    >>
     /Potato <<
       /BaseVersion /3.14159
       /ExtensionLevel 16059
@@ -92,17 +95,17 @@ xref
 0 8
 0000000000 65535 f 
 0000000052 00000 n 
-0000000285 00000 n 
-0000000394 00000 n 
-0000000636 00000 n 
-0000000735 00000 n 
-0000000781 00000 n 
-0000000926 00000 n 
+0000000301 00000 n 
+0000000410 00000 n 
+0000000652 00000 n 
+0000000751 00000 n 
+0000000797 00000 n 
+0000000942 00000 n 
 trailer <<
   /Root 1 0 R
   /Size 8
   /ID [<484577389048fa45fc00a1f5b434efa5><31415926535897932384626433832795>]
 >>
 startxref
-961
+977
 %%EOF
@@ -5,7 +5,12 @@
 %% Original object ID: 1 0
 1 0 obj
 <<
-  /Extensions << /ADBE << /BaseVersion /1.8 /ExtensionLevel 5 >> >>
+  /Extensions <<
+    /ADBE <<
+      /BaseVersion /1.8
+      /ExtensionLevel 5
+    >>
+  >>
   /Pages 2 0 R
   /Type /Catalog
 >>
@@ -86,17 +91,17 @@ xref
 0 8
 0000000000 65535 f 
 0000000052 00000 n 
-0000000201 00000 n 
-0000000310 00000 n 
-0000000552 00000 n 
-0000000651 00000 n 
-0000000697 00000 n 
-0000000842 00000 n 
+0000000223 00000 n 
+0000000332 00000 n 
+0000000574 00000 n 
+0000000673 00000 n 
+0000000719 00000 n 
+0000000864 00000 n 
 trailer <<
   /Root 1 0 R
   /Size 8
   /ID [<31415926535897932384626433832795><31415926535897932384626433832795>]
 >>
 startxref
-877
+899
 %%EOF
@@ -6,7 +6,10 @@
 1 0 obj
 <<
   /Extensions <<
-    /ADBE << /BaseVersion /1.8 /ExtensionLevel 5 >>
+    /ADBE <<
+      /BaseVersion /1.8
+      /ExtensionLevel 5
+    >>
     /Potato <<
       /BaseVersion /3.14159
       /ExtensionLevel 16059
@@ -92,17 +95,17 @@ xref
 0 8
 0000000000 65535 f 
 0000000052 00000 n 
-0000000285 00000 n 
-0000000394 00000 n 
-0000000636 00000 n 
-0000000735 00000 n 
-0000000781 00000 n 
-0000000926 00000 n 
+0000000301 00000 n 
+0000000410 00000 n 
+0000000652 00000 n 
+0000000751 00000 n 
+0000000797 00000 n 
+0000000942 00000 n 
 trailer <<
   /Root 1 0 R
   /Size 8
   /ID [<369e89600ee1a6c4c7e73533610180c2><31415926535897932384626433832795>]
 >>
 startxref
-961
+977
 %%EOF