fix: address reviewer feedback regarding [GS operator related]

Cargo.lock

@@ -4027,6 +4027,7 @@ dependencies = [
  "pdfcraft-cos",
  "pdfcraft-fonts",
  "pdfcraft-model",
+ "pdfcraft-render",
  "thiserror 2.0.21",
  "unicode-bidi",
 ]

crates/edit/Cargo.toml

@@ -22,3 +22,4 @@ unicode-bidi = "0.3"
 
 [dev-dependencies]
 hayro-syntax = { workspace = true }
+pdfcraft-render = { workspace = true }

crates/edit/src/images.rs

@@ -10,6 +10,27 @@ use std::sync::Arc;
 
 use crate::EditError;
 
+/// The identity of a soft mask (`/SMask`) in the PDF graphics state.
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub enum MaskIdentity {
+    /// Indirect reference to a mask dictionary or stream.
+    Ref(u32, u16),
+    /// Indirect reference to the transparency group (`/G`) in a mask dictionary.
+    Group(u32, u16),
+    /// Explicit named mask or resource name.
+    Named(Vec<u8>),
+}
+
+impl std::fmt::Display for MaskIdentity {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            MaskIdentity::Ref(num, generation) => write!(f, "{num} {generation} R"),
+            MaskIdentity::Group(num, generation) => write!(f, "group {num} {generation} R"),
+            MaskIdentity::Named(name) => write!(f, "{}", String::from_utf8_lossy(name)),
+        }
+    }
+}
+
 /// An image drawn on a page.
 #[derive(Clone, Debug, PartialEq)]
 pub struct PageImage {
@@ -30,6 +51,8 @@ pub struct PageImage {
     /// Pixel size; zero for vector/grouped Form artwork.
     pub width: u32,
     pub height: u32,
+    /// Active soft mask identity at the image placement, if any.
+    pub smask: Option<MaskIdentity>,
     stream: usize,
     op: usize,
     /// CTM before `Do`, excluding a Form's own Matrix.
@@ -213,12 +236,16 @@ impl ImageState {
 pub fn page_images(doc: &Document, page: usize) -> Result<Vec<PageImage>, EditError> {
     let p = page_of(doc, page)?;
     let xo = xobjects(doc, &p.dict);
+    let res = p.dict.get(b"Resources").map(|r| doc.resolve(r)).and_then(|r| r.as_dict().cloned()).unwrap_or_default();
+    let ext_gstates = res.get(b"ExtGState").map(|g| doc.resolve(g)).and_then(|g| g.as_dict().cloned()).unwrap_or_default();
     let mut out = Vec::new();
     // The graphics state carries over from one stream to the next.
     let mut state = ImageState::new(Matrix::IDENTITY, p.crop(doc), Vec::new());
+    let mut tracker = ExtendedGStateTracker::default();
     for (si, (_, data)) in streams(doc, &p.dict).into_iter().enumerate() {
         for (i, op) in parse(&data).ops.iter().enumerate() {
             state.apply(op)?;
+            tracker.step(op, &ext_gstates, doc)?;
             let ctm = state.placement.matrix;
             if op.is("Do") {
                 let Some(name) = op.name(0) else { continue };
@@ -274,6 +301,7 @@ pub fn page_images(doc: &Document, page: usize) -> Result<Vec<PageImage>, EditEr
                     is_form,
                     width: if is_form { 0 } else { s.dict.int(b"Width").unwrap_or(0).clamp(0, u32::MAX as i64) as u32 },
                     height: if is_form { 0 } else { s.dict.int(b"Height").unwrap_or(0).clamp(0, u32::MAX as i64) as u32 },
+                    smask: tracker.current_state.smask.clone(),
                     stream: si,
                     op: i,
                     draw_matrix: ctm.0,
@@ -347,6 +375,7 @@ pub fn reading_images(doc: &Document, page: usize) -> Result<Vec<PageImage>, Edi
                     is_form: false,
                     width: s.dict.int(b"Width").unwrap_or(0).max(0) as u32,
                     height: s.dict.int(b"Height").unwrap_or(0).max(0) as u32,
+                    smask: None,
                     stream,
                     op: i,
                     draw_matrix: ctm.0,
@@ -548,7 +577,7 @@ struct EffectiveGState {
     ca: Option<f64>,
     ca_stroke: Option<f64>,
     bm: Option<Vec<u8>>,
-    smask: Option<Vec<u8>>,
+    smask: Option<MaskIdentity>,
 }
 
 impl EffectiveGState {
@@ -564,11 +593,8 @@ impl EffectiveGState {
         self.bm.as_deref().unwrap_or(b"Normal")
     }
 
-    fn effective_smask(&self) -> Option<&[u8]> {
-        match self.smask.as_deref() {
-            Some(b"None") | None => None,
-            Some(other) => Some(other),
-        }
+    fn effective_smask(&self) -> Option<&MaskIdentity> {
+        self.smask.as_ref()
     }
 
     fn visually_differs(&self, other: &EffectiveGState) -> bool {
@@ -578,7 +604,7 @@ impl EffectiveGState {
             || self.effective_smask() != other.effective_smask()
     }
 
-    fn merge_dict(&mut self, dict: &Dict, doc: &Document) {
+    fn merge_dict(&mut self, dict: &Dict, doc: &Document, gs_name: Option<&[u8]>) {
         if let Some(val) = dict.get(b"ca").map(|v| doc.resolve(v)).and_then(|v| v.as_f64()) {
             self.ca = Some(val);
         }
@@ -594,11 +620,28 @@ impl EffectiveGState {
                 self.bm = Some(first_name);
             }
         }
-        if let Some(smask_obj) = dict.get(b"SMask").map(|v| doc.resolve(v)) {
-            if let Some(name) = smask_obj.as_name() {
-                self.smask = Some(name.to_vec());
-            } else if smask_obj.as_dict().is_some() {
-                self.smask = Some(b"CustomSMask".to_vec());
+        if let Some(raw_smask) = dict.get(b"SMask") {
+            let resolved = doc.resolve(raw_smask);
+            if let Some(name) = resolved.as_name() {
+                if name == b"None" {
+                    self.smask = None;
+                } else {
+                    self.smask = Some(MaskIdentity::Named(name.to_vec()));
+                }
+            } else if let Some(mask_dict) = resolved.as_dict() {
+                if let Some(gr) = mask_dict.get(b"G").and_then(|g| g.as_ref()) {
+                    self.smask = Some(MaskIdentity::Group(gr.num, gr.generation));
+                } else if let Some(r) = raw_smask.as_ref() {
+                    self.smask = Some(MaskIdentity::Ref(r.num, r.generation));
+                } else if let Some(name) = mask_dict.get(b"Name").map(|n| doc.resolve(n)).and_then(|n| n.as_name().map(|s| s.to_vec())) {
+                    self.smask = Some(MaskIdentity::Named(name));
+                } else if let Some(name) = gs_name {
+                    self.smask = Some(MaskIdentity::Named(name.to_vec()));
+                } else {
+                    self.smask = Some(MaskIdentity::Named(b"CustomMask".to_vec()));
+                }
+            } else if let Some(r) = raw_smask.as_ref() {
+                self.smask = Some(MaskIdentity::Ref(r.num, r.generation));
             }
         }
     }
@@ -615,7 +658,7 @@ impl ExtendedGStateTracker {
     fn step(&mut self, op: &Op, ext_gstates: &Dict, doc: &Document) -> Result<(), EditError> {
         match op.op.as_slice() {
             b"q" => {
-                if self.stack.len() >= 256 {
+                if self.stack.len() >= 4096 {
                     return Err(EditError::Invalid("the image graphics state is too deeply nested".into()));
                 }
                 self.stack.push((self.current_state.clone(), self.active_gs_ops.clone()));
@@ -629,7 +672,7 @@ impl ExtendedGStateTracker {
             b"gs" => {
                 if let Some(name) = op.name(0) {
                     if let Some(dict) = ext_gstates.get(name).map(|v| doc.resolve(v)).and_then(|v| v.as_dict().cloned()) {
-                        self.current_state.merge_dict(&dict, doc);
+                        self.current_state.merge_dict(&dict, doc, Some(name));
                     } else if self.current_state.ca.is_none() {
                         self.current_state.ca = Some(0.5);
                     }
@@ -672,7 +715,7 @@ fn extract_scope_state(ops: &[Op], start: usize, target: usize) -> Result<Option
         let Some(op) = ops.get(i) else { return Ok(None); };
         match op.op.as_slice() {
             b"q" => {
-                if depth >= 256 {
+                if depth >= 4096 {
                     return Err(EditError::Invalid("the image graphics state is too deeply nested".into()));
                 }
                 depth = depth.saturating_add(1);
@@ -889,13 +932,17 @@ pub fn arrange_image(doc: &mut Document, page: usize, index: usize, order: Arran
         return Err(EditError::Invalid("unsupported graphics state: inherited clipping path cannot be cleanly preserved".into()));
     }
 
+    let mut outer_gs_ops = Vec::new();
     for src_gs in &source_gs_ops {
         let name = src_gs.name(0);
         let already_present = extracted.state_ops.iter().any(|o| o.name(0) == name);
         if !already_present {
-            extracted.state_ops.push(src_gs.clone());
+            outer_gs_ops.push(src_gs.clone());
         }
     }
+    let mut combined_state_ops = outer_gs_ops;
+    combined_state_ops.extend(extracted.state_ops);
+    extracted.state_ops = combined_state_ops;
 
     let mut reconstructed_state = dest_effective_state.clone();
     for op in &extracted.state_ops {
@@ -903,7 +950,7 @@ pub fn arrange_image(doc: &mut Document, page: usize, index: usize, order: Arran
             && let Some(name) = op.name(0)
         {
             if let Some(dict) = ext_gstates.get(name).map(|v| doc.resolve(v)).and_then(|v| v.as_dict().cloned()) {
-                reconstructed_state.merge_dict(&dict, doc);
+                reconstructed_state.merge_dict(&dict, doc, Some(name));
             } else if reconstructed_state.ca.is_none() {
                 reconstructed_state.ca = Some(0.5);
             }
@@ -918,6 +965,7 @@ pub fn arrange_image(doc: &mut Document, page: usize, index: usize, order: Arran
 
     let mut unit_ops = Vec::new();
     unit_ops.push(Op::new("q", vec![]));
+    unit_ops.extend(extracted.state_ops);
     if !extracted.clip_ops.is_empty() {
         if dest_ctm != Matrix::IDENTITY {
             unit_ops.push(Op::new("cm", dest_inv.0.map(pdfcraft_content::num).to_vec()));
@@ -932,7 +980,6 @@ pub fn arrange_image(doc: &mut Document, page: usize, index: usize, order: Arran
             unit_ops.push(Op::new("cm", local.0.map(pdfcraft_content::num).to_vec()));
         }
     }
-    unit_ops.extend(extracted.state_ops);
     unit_ops.push(ops.get(target).cloned().unwrap_or_else(|| Op::new("Do", vec![Object::name(&img.name)])));
     unit_ops.push(Op::new("Q", vec![]));
 

crates/edit/src/lib.rs

@@ -623,7 +623,7 @@ pub use objects::{EditableObject, MAX_MOVE_OBJECTS, ObjectKind, ObjectTarget, ed
 mod flatten;
 pub use flatten::{flatten, flatten_fill_sign};
 pub mod images;
-pub use images::{ArrangeOrder, ImageChange, PageImage, arrange_image, change_image, crop_image, page_images, reading_images, rect_to_rect, remove_image_crop, turn_about_centre};
+pub use images::{ArrangeOrder, ImageChange, MaskIdentity, PageImage, arrange_image, change_image, crop_image, page_images, reading_images, rect_to_rect, remove_image_crop, turn_about_centre};
 pub mod bidi;
 pub mod indic;
 pub mod text;

crates/edit/src/tests.rs

@@ -2074,6 +2074,101 @@ fn destination_differing_blend_mode_pollution_is_prevented() {
     assert_eq!(initial_bytes, after_bytes, "Preservation: content stream remained completely unmodified");
 }
 
+fn nested_soft_mask_page(content: &str) -> Document {
+    let objs: Vec<String> = vec![
+        "<< /Type /Catalog /Pages 2 0 R >>".into(),
+        "<< /Type /Pages /Kids [3 0 R] /Count 1 >>".into(),
+        "<< /Type /Page /Parent 2 0 R /MediaBox [0 0 100 100] /Contents 4 0 R /Resources << /Font << /F1 5 0 R >> /XObject << /Im0 6 0 R /GroupA 9 0 R /GroupB 10 0 R >> /ExtGState << /GSA 7 0 R /GSB 8 0 R >> >> >>".into(),
+        format!("<< /Length {} >>\nstream\n{content}\nendstream", content.len()),
+        "<< /Type /Font /Subtype /Type1 /BaseFont /Helvetica >>".into(),
+        "<< /Type /XObject /Subtype /Image /Width 2 /Height 2 /ColorSpace /DeviceGray /BitsPerComponent 8 /Length 4 >>\nstream\n\u{0}\u{0}\u{0}\u{0}\nendstream".into(),
+        "<< /Type /ExtGState /SMask << /Type /Mask /S /Alpha /G 9 0 R >> >>".into(),
+        "<< /Type /ExtGState /SMask << /Type /Mask /S /Alpha /G 10 0 R >> >>".into(),
+        "<< /Type /XObject /Subtype /Form /BBox [0 0 100 100] /Group << /S /Transparency /I true >> /Length 20 >>\nstream\n0 g 0 0 100 50 re f\nendstream".into(),
+        "<< /Type /XObject /Subtype /Form /BBox [0 0 100 100] /Group << /S /Transparency /I true >> /Length 20 >>\nstream\n0 g 0 0 50 100 re f\nendstream".into(),
+    ];
+    let mut out = b"%PDF-1.7\n".to_vec();
+    let mut offs = Vec::new();
+    for (i, o) in objs.iter().enumerate() {
+        offs.push(out.len());
+        out.extend_from_slice(format!("{} 0 obj\n{o}\nendobj\n", i + 1).as_bytes());
+    }
+    let x = out.len();
+    out.extend_from_slice(format!("xref\n0 {}\n0000000000 65535 f \n", objs.len() + 1).as_bytes());
+    for o in offs {
+        out.extend_from_slice(format!("{o:010} 00000 n \n").as_bytes());
+    }
+    out.extend_from_slice(format!("trailer\n<< /Size {} /Root 1 0 R >>\nstartxref\n{x}\n%%EOF\n", objs.len() + 1).as_bytes());
+    Document::open(Arc::new(out)).unwrap()
+}
+
+#[test]
+fn nested_soft_masks_replay_in_correct_order_and_effective_mask_is_rendered() {
+    let content = "q /GSA gs q /GSB gs 100 0 0 100 0 0 cm /Im0 Do Q Q BT /F1 12 Tf 10 90 Td (External Text) Tj ET";
+    let mut doc = nested_soft_mask_page(content);
+    let initial = images::page_images(&doc, 0).unwrap();
+    assert_eq!(initial.len(), 1);
+    assert_eq!(initial[0].smask, Some(images::MaskIdentity::Group(10, 0)));
+
+    images::arrange_image(&mut doc, 0, 0, images::ArrangeOrder::BringToFront).unwrap();
+
+    let content_bytes = page_content_bytes(&doc, 0);
+    let str_content = String::from_utf8_lossy(&content_bytes);
+
+    // 1. Assert emitted stream order: outer /GSA gs must precede inner /GSB gs, which precedes /Im0 Do
+    let after_text = str_content.find("External Text").unwrap();
+    let after_a = str_content.rfind("/GSA gs").unwrap();
+    let after_b = str_content.rfind("/GSB gs").unwrap();
+    let after_do = str_content.rfind("/Im0 Do").unwrap();
+    assert!(after_text < after_a, "Position: /GSA gs relocated past external text");
+    assert!(after_a < after_b, "Order: outer /GSA gs must be replayed before inner /GSB gs");
+    assert!(after_b < after_do, "Order: inner /GSB gs must immediately precede /Im0 Do");
+
+    // 2. Assert effective mask upon saving and reopening is correctly identified as B
+    let reopened_doc = reopen(&doc);
+    let reopened_images = images::page_images(&reopened_doc, 0).unwrap();
+    assert_eq!(reopened_images.len(), 1);
+    assert_eq!(reopened_images[0].smask, Some(images::MaskIdentity::Group(10, 0)));
+
+    // 3. Rendered-pixel assertion: render output and verify Mask B (left 50x100) is effective, not Mask A (bottom 100x50)
+    let pdf_bytes = write_incremental(&doc, &SaveOptions::default()).unwrap();
+    let mut renderer = pdfcraft_render::PageRenderer::new(Arc::new(pdf_bytes), pdfcraft_render::RenderConfig::default());
+    let page = renderer.render(pdfcraft_render::RenderRequest {
+        page: 0,
+        kind: pdfcraft_render::RequestKind::Pixels,
+        scale: 1.0,
+        ..Default::default()
+    });
+    let px = |x: u32, y: u32| {
+        let idx = ((y * page.width + x) * 4) as usize;
+        page.rgba[idx..idx + 4].to_vec()
+    };
+    // Left half (inside Mask B): image is rendered black
+    assert_eq!(px(25, 25), vec![0, 0, 0, 255], "point (25, 25) is inside Mask B: image rendered");
+    assert_eq!(px(25, 75), vec![0, 0, 0, 255], "point (25, 75) is inside Mask B: image rendered");
+    // Right half (outside Mask B): masked out to white page background
+    assert_eq!(px(75, 25), vec![255, 255, 255, 255], "point (75, 25) is outside Mask B: masked out to white");
+    assert_eq!(px(75, 75), vec![255, 255, 255, 255], "point (75, 75) is outside Mask B: masked out to white");
+}
+
+#[test]
+fn destination_differing_mask_pollution_is_prevented() {
+    // /Im0 Do has no mask; destination has active top-level /GSA gs (Mask A)
+    let content = "100 0 0 100 0 0 cm /Im0 Do /GSA gs BT /F1 12 Tf 10 90 Td (Masked Section) Tj ET";
+    let mut doc = nested_soft_mask_page(content);
+    let initial_bytes = page_content_bytes(&doc, 0);
+
+    let res = images::arrange_image(&mut doc, 0, 0, images::ArrangeOrder::BringToFront);
+    assert!(res.is_err(), "must return an explicit Err rather than corrupting mask state");
+    let err_msg = res.unwrap_err().to_string();
+    assert!(err_msg.contains("unsupported graphics state"), "error describes unsupported graphics state: {err_msg}");
+
+    // Preservation: document contents are completely unmodified
+    let doc = reopen(&doc);
+    let after_bytes = page_content_bytes(&doc, 0);
+    assert_eq!(initial_bytes, after_bytes, "Preservation: content stream remained completely unmodified");
+}
+
 
 #[test]
 fn form_artwork_edits_only_the_selected_placement() {