fix: address reviewer feedback regarding [GS operator related]

crates/edit/src/images.rs

@@ -245,7 +245,7 @@ pub fn page_images(doc: &Document, page: usize) -> Result<Vec<PageImage>, EditEr
     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)?;
+            tracker.step(op, &ext_gstates, doc, state.placement.matrix)?;
             let ctm = state.placement.matrix;
             if op.is("Do") {
                 let Some(name) = op.name(0) else { continue };
@@ -649,13 +649,13 @@ impl EffectiveGState {
 
 #[derive(Clone, Default)]
 struct ExtendedGStateTracker {
-    stack: Vec<(EffectiveGState, Vec<Op>)>,
+    stack: Vec<(EffectiveGState, Vec<(Op, Matrix)>)>,
     current_state: EffectiveGState,
-    active_gs_ops: Vec<Op>,
+    active_gs_ops: Vec<(Op, Matrix)>,
 }
 
 impl ExtendedGStateTracker {
-    fn step(&mut self, op: &Op, ext_gstates: &Dict, doc: &Document) -> Result<(), EditError> {
+    fn step(&mut self, op: &Op, ext_gstates: &Dict, doc: &Document, ctm: Matrix) -> Result<(), EditError> {
         match op.op.as_slice() {
             b"q" => {
                 if self.stack.len() >= 4096 {
@@ -676,8 +676,8 @@ impl ExtendedGStateTracker {
                     } else if self.current_state.ca.is_none() {
                         self.current_state.ca = Some(0.5);
                     }
-                    self.active_gs_ops.retain(|o| o.name(0) != Some(name));
-                    self.active_gs_ops.push(op.clone());
+                    self.active_gs_ops.retain(|(o, _)| o.name(0) != Some(name));
+                    self.active_gs_ops.push((op.clone(), ctm));
                 }
             }
             _ => {}
@@ -897,13 +897,15 @@ pub fn arrange_image(doc: &mut Document, page: usize, index: usize, order: Arran
             break;
         }
         state.apply(op)?;
-        tracker.step(op, &ext_gstates, doc)?;
+        tracker.step(op, &ext_gstates, doc, state.placement.matrix)?;
     }
     if scan_target >= ops.len() {
         dest_effective_state = tracker.current_state.clone();
         dest_ctm = state.placement.matrix;
     }
-    let Some(dest_inv) = dest_ctm.invert() else { return Ok(()); };
+    if dest_ctm.invert().is_none() {
+        return Err(EditError::Invalid("unsupported graphics state: non-invertible CTM".into()));
+    }
 
     let (target_span, scope_start) = if let Ok(Some((sub_start, sub_end, _))) = image_exclusive_scope(&ops, target)
         && sub_start >= target_unit.start
@@ -924,7 +926,7 @@ pub fn arrange_image(doc: &mut Document, page: usize, index: usize, order: Arran
     let is_clipped = (img.visible_rect.is_some() && img.visible_rect != Some(img.rect)) || !img.clips.is_empty();
 
     let extracted = extract_scope_state(&ops, scope_start, target)?;
-    let Some(mut extracted) = extracted else {
+    let Some(extracted) = extracted else {
         return Err(EditError::Invalid("unsupported graphics state: unable to cleanly extract image scope state".into()));
     };
 
@@ -932,23 +934,11 @@ 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 {
-            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 non_gs_ops: Vec<Op> = extracted.state_ops.into_iter().filter(|o| !o.is("gs")).collect();
 
     let mut reconstructed_state = dest_effective_state.clone();
-    for op in &extracted.state_ops {
-        if op.is("gs")
-            && let Some(name) = op.name(0)
-        {
+    for (op, _) in &source_gs_ops {
+        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()) {
                 reconstructed_state.merge_dict(&dict, doc, Some(name));
             } else if reconstructed_state.ca.is_none() {
@@ -965,19 +955,50 @@ 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);
+    let mut current_ctm = dest_ctm;
+
+    for (gs_op, target_ctm) in &source_gs_ops {
+        if *target_ctm != current_ctm {
+            let Some(current_inv) = current_ctm.invert() else {
+                return Err(EditError::Invalid("unsupported graphics state: non-invertible CTM".into()));
+            };
+            let delta = target_ctm.then(&current_inv);
+            if !delta.0.iter().all(|v| v.is_finite()) {
+                return Err(EditError::Invalid("unsupported graphics state: invalid transform coordinates".into()));
+            }
+            if delta != Matrix::IDENTITY {
+                unit_ops.push(Op::new("cm", delta.0.map(pdfcraft_content::num).to_vec()));
+                current_ctm = *target_ctm;
+            }
+        }
+        unit_ops.push(gs_op.clone());
+    }
+
+    unit_ops.extend(non_gs_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()));
+        if current_ctm != Matrix::IDENTITY {
+            let Some(current_inv) = current_ctm.invert() else {
+                return Err(EditError::Invalid("unsupported graphics state: non-invertible CTM".into()));
+            };
+            unit_ops.push(Op::new("cm", current_inv.0.map(pdfcraft_content::num).to_vec()));
         }
         unit_ops.extend(extracted.clip_ops);
         if Matrix(img.draw_matrix) != Matrix::IDENTITY {
             unit_ops.push(Op::new("cm", img.draw_matrix.map(pdfcraft_content::num).to_vec()));
         }
     } else {
-        let local = Matrix(img.draw_matrix).then(&dest_inv);
-        if local != Matrix::IDENTITY {
-            unit_ops.push(Op::new("cm", local.0.map(pdfcraft_content::num).to_vec()));
+        if Matrix(img.draw_matrix) != current_ctm {
+            let Some(current_inv) = current_ctm.invert() else {
+                return Err(EditError::Invalid("unsupported graphics state: non-invertible CTM".into()));
+            };
+            let delta = Matrix(img.draw_matrix).then(&current_inv);
+            if !delta.0.iter().all(|v| v.is_finite()) {
+                return Err(EditError::Invalid("unsupported graphics state: invalid transform coordinates".into()));
+            }
+            if delta != Matrix::IDENTITY {
+                unit_ops.push(Op::new("cm", delta.0.map(pdfcraft_content::num).to_vec()));
+            }
         }
     }
     unit_ops.push(ops.get(target).cloned().unwrap_or_else(|| Op::new("Do", vec![Object::name(&img.name)])));

crates/edit/src/tests.rs

@@ -2169,6 +2169,113 @@ fn destination_differing_mask_pollution_is_prevented() {
     assert_eq!(initial_bytes, after_bytes, "Preservation: content stream remained completely unmodified");
 }
 
+fn soft_mask_with_alpha_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 200 200] /Contents 4 0 R /Resources << /Font << /F1 5 0 R >> /XObject << /Im0 6 0 R /GroupA 9 0 R >> /ExtGState << /GSA 7 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 /ca 0.25 /SMask << /Type /Mask /S /Alpha /G 9 0 R >> >>".into(),
+        "<< /Type /ExtGState /SMask /None >>".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(),
+    ];
+    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 non_identity_cm_before_gs_preserves_mask_coordinates_and_rendering() {
+    let content = "q 2 0 0 2 10 10 cm /GSA gs 50 0 0 50 0 0 cm /Im0 Do Q BT /F1 12 Tf 10 180 Td (External Text) Tj ET";
+    let mut doc = soft_mask_with_alpha_page(content);
+
+    // Initial state: image bounds are [10, 10, 110, 110]
+    let initial_images = images::page_images(&doc, 0).unwrap();
+    assert_eq!(initial_images.len(), 1);
+    assert!(close(initial_images[0].rect, [10.0, 10.0, 110.0, 110.0]));
+
+    // Render baseline before reordering
+    let pdf_bytes_before = write_incremental(&doc, &SaveOptions::default()).unwrap();
+    let mut renderer_before = pdfcraft_render::PageRenderer::new(Arc::new(pdf_bytes_before), pdfcraft_render::RenderConfig::default());
+    let page_before = renderer_before.render(pdfcraft_render::RenderRequest {
+        page: 0,
+        kind: pdfcraft_render::RequestKind::Pixels,
+        scale: 1.0,
+        ..Default::default()
+    });
+    let px_before = |x: u32, y: u32| {
+        let idx = ((y * page_before.width + x) * 4) as usize;
+        page_before.rgba[idx..idx + 4].to_vec()
+    };
+
+    // Arrange image: BringToFront
+    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 AST reordering succeeds and interleaves cm before gs
+    let text_pos = str_content.find("External Text").unwrap();
+    let cm_mask_pos = str_content.rfind("2 0 0 2 10 10 cm").unwrap();
+    let gs_pos = str_content.rfind("/GSA gs").unwrap();
+    let cm_img_pos = str_content.rfind("50 0 0 50 0 0 cm").unwrap();
+    let do_pos = str_content.rfind("/Im0 Do").unwrap();
+
+    assert!(text_pos < cm_mask_pos, "Image relocated after external text");
+    assert!(cm_mask_pos < gs_pos, "CTM for soft mask established before /GSA gs");
+    assert!(gs_pos < cm_img_pos, "/GSA gs executed before image cm");
+    assert!(cm_img_pos < do_pos, "image cm executed before /Im0 Do");
+
+    // 2. Assert image bounds remain unchanged after saving and reopening
+    let reopened_doc = reopen(&doc);
+    let reopened_images = images::page_images(&reopened_doc, 0).unwrap();
+    assert_eq!(reopened_images.len(), 1);
+    assert!(
+        close(reopened_images[0].rect, [10.0, 10.0, 110.0, 110.0]),
+        "Image bounds preserved: {:?}",
+        reopened_images[0].rect
+    );
+
+    // 3. Render reopened document and assert non-overlapping pixels remain unchanged
+    let pdf_bytes_after = write_incremental(&reopened_doc, &SaveOptions::default()).unwrap();
+    let mut renderer_after = pdfcraft_render::PageRenderer::new(Arc::new(pdf_bytes_after), pdfcraft_render::RenderConfig::default());
+    let page_after = renderer_after.render(pdfcraft_render::RenderRequest {
+        page: 0,
+        kind: pdfcraft_render::RequestKind::Pixels,
+        scale: 1.0,
+        ..Default::default()
+    });
+    let px_after = |x: u32, y: u32| {
+        let idx = ((y * page_after.width + x) * 4) as usize;
+        page_after.rgba[idx..idx + 4].to_vec()
+    };
+
+    // A point inside the masked image (e.g. at x=50, inside the image and bottom half of mask)
+    // Non-overlapping with text (text is at y=180).
+    // The pixel renders at [191, 191, 191, 255] (25% black over 100% white).
+    // In raster coordinates, raster y for PDF y=50 in a 200-high page is y=150.
+    let rendered_pixel_before = px_before(50, 150);
+    let rendered_pixel_after = px_after(50, 150);
+    assert_eq!(rendered_pixel_before, vec![191, 191, 191, 255], "baseline pixel rendered with 25% opacity mask");
+    assert_eq!(rendered_pixel_after, rendered_pixel_before, "rendered non-overlapping pixels remain unchanged");
+
+    // Outside the image: pure white background
+    assert_eq!(px_after(5, 5), vec![255, 255, 255, 255]);
+    assert_eq!(px_after(5, 5), px_before(5, 5));
+}
+
 
 #[test]
 fn form_artwork_edits_only_the_selected_placement() {