From 5ee6aa1e5036ed74bed9987ef3e3f3a91d375960 Mon Sep 17 00:00:00 2001 From: yenru0 Date: Sat, 3 Oct 2026 20:09:25 +0900 Subject: [PATCH] image_viewer: Fix swapped red and blue channels in PDF pages GPUI's RenderImage contract is BGRA (assets.rs) and every platform atlas uploads the bytes raw into BGRA8 textures. All other decode paths swap R/B before building a RenderImage; the PDF path fed pdfium's RGBA buffer straight through, so PDF pages rendered with swapped channels on every platform (white/gray unaffected, colors wrong). Swap R/B at both RenderImage construction sites (page view + thumbnails) and pin pdf_viewer's RGBA raster contract with a native channel-order regression test (skips when no libpdfium is present). --- crates/image_viewer/src/image_viewer.rs | 16 +++- crates/pdf_viewer/tests/integration_tests.rs | 83 +++++++++++++++++++- 2 files changed, 96 insertions(+), 3 deletions(-) diff --git a/crates/image_viewer/src/image_viewer.rs b/crates/image_viewer/src/image_viewer.rs index 7e9ae12561..87084982ff 100644 --- a/crates/image_viewer/src/image_viewer.rs +++ b/crates/image_viewer/src/image_viewer.rs @@ -1789,10 +1789,16 @@ impl ImageView { .render_page_from_bytes(pdf_bytes, page_index, options) .ok()?; let byte_size = rendered.byte_size(); + // `RenderImage` carries BGRA bytes (see `gpui::assets`); like every other + // decode path, swap R/B before handing pdfium's RGBA output to the atlas. + let mut frame_buffer = rendered.rgba_buffer.as_ref().clone(); + for pixel in frame_buffer.chunks_exact_mut(4) { + pixel.swap(0, 2); + } let image_buffer = image::ImageBuffer::, _>::from_raw( rendered.width, rendered.height, - rendered.rgba_buffer.as_ref().clone(), + frame_buffer, )?; let render_image = Arc::new(gpui::RenderImage::new(vec![image::Frame::new( image_buffer, @@ -2881,10 +2887,16 @@ impl ImageView { if let Ok(rendered) = engine.render_page_from_bytes(pdf_bytes, page_index, options) { let byte_size = rendered.byte_size(); + // `RenderImage` carries BGRA bytes (see `gpui::assets`); like every other + // decode path, swap R/B before handing pdfium's RGBA output to the atlas. + let mut frame_buffer = rendered.rgba_buffer.as_ref().clone(); + for pixel in frame_buffer.chunks_exact_mut(4) { + pixel.swap(0, 2); + } if let Some(img_buf) = image::ImageBuffer::, _>::from_raw( rendered.width, rendered.height, - rendered.rgba_buffer.as_ref().clone(), + frame_buffer, ) { let render_image = Arc::new(gpui::RenderImage::new(vec![image::Frame::new(img_buf)])); diff --git a/crates/pdf_viewer/tests/integration_tests.rs b/crates/pdf_viewer/tests/integration_tests.rs index 3ce13e6358..da38f7f07a 100644 --- a/crates/pdf_viewer/tests/integration_tests.rs +++ b/crates/pdf_viewer/tests/integration_tests.rs @@ -1,4 +1,5 @@ -use pdf_viewer::{PageLayoutMode, PdfToolbarState, PdfView, PdfViewerSettings}; +use pdf_viewer::rasterizer::RasterizerOptions; +use pdf_viewer::{PageLayoutMode, PdfToolbarState, PdfView, PdfViewerSettings, PdfiumEngine}; use std::time::Duration; const MINIMAL_PDF: &[u8] = b"%PDF-1.7\n1 0 obj\n<< /Type /Catalog /Pages 2 0 R >>\nendobj\n2 0 obj\n<< /Type /Pages /Kids [3 0 R] /Count 1 >>\nendobj\n3 0 obj\n<< /Type /Page /Parent 2 0 R /MediaBox [0 0 612 792] >>\nendobj\nxref\n0 4\n0000000000 65535 f \n0000000010 00000 n \n0000000060 00000 n \n0000000117 00000 n \ntrailer\n<< /Size 4 /Root 1 0 R >>\nstartxref\n185\n%%EOF\n"; @@ -105,3 +106,83 @@ fn test_toolbar_synchronization_with_view_state() { assert!(toolbar_state.dark_mode); assert_eq!(toolbar_state.layout_mode, PageLayoutMode::Continuous); } + +/// Builds a single-page PDF whose entire canvas is filled pure red, with a fully +/// computed xref table so no repair parsing is involved. +fn red_page_pdf() -> Vec { + let content = b"1 0 0 rg 0 0 612 792 re f"; + let objects: Vec> = vec![ + b"<< /Type /Catalog /Pages 2 0 R >>".to_vec(), + b"<< /Type /Pages /Kids [3 0 R] /Count 1 >>".to_vec(), + b"<< /Type /Page /Parent 2 0 R /MediaBox [0 0 612 792] /Resources << >> /Contents 4 0 R >>" + .to_vec(), + format!( + "<< /Length {} >>\nstream\n{}\nendstream", + content.len(), + String::from_utf8_lossy(content) + ) + .into_bytes(), + ]; + + let mut out = b"%PDF-1.7\n".to_vec(); + let mut offsets = Vec::with_capacity(objects.len()); + for (index, object) in objects.iter().enumerate() { + offsets.push(out.len() as u32); + out.extend_from_slice(format!("{} 0 obj\n", index + 1).as_bytes()); + out.extend_from_slice(object); + out.extend_from_slice(b"\nendobj\n"); + } + + let xref_offset = out.len() as u32; + out.extend_from_slice(format!("xref\n0 {}\n", objects.len() + 1).as_bytes()); + out.extend_from_slice(b"0000000000 65535 f \n"); + for offset in &offsets { + out.extend_from_slice(format!("{offset:010} 00000 n \n").as_bytes()); + } + out.extend_from_slice( + format!( + "trailer\n<< /Size {} /Root 1 0 R >>\nstartxref\n{}\n%%EOF", + objects.len() + 1, + xref_offset + ) + .as_bytes(), + ); + out +} + +/// Guards against PDFium's native BGRA bitmap order leaking through: the rasterizer +/// must hand out RGBA buffers, so a red page must read as high-R/low-B. The mock +/// renderer also draws red, so this only proves something when a native engine +/// bound; without libpdfium the test cannot exercise the real byte order. +#[test] +fn test_native_render_outputs_rgba_channel_order() { + let engine = PdfiumEngine::new(); + if !engine.is_native_available() { + eprintln!("skipping: native libpdfium not available on this machine"); + return; + } + + let rendered = engine + .render_page_from_bytes(&red_page_pdf(), 0, RasterizerOptions::default()) + .expect("native render should succeed"); + + let stride = rendered.width as usize * 4; + assert_eq!(rendered.rgba_buffer.len(), stride * rendered.height as usize); + + for (x, y) in [ + (rendered.width as usize / 2, rendered.height as usize / 2), + (rendered.width as usize / 4, rendered.height as usize / 4), + ( + rendered.width as usize * 3 / 4, + rendered.height as usize * 3 / 4, + ), + ] { + let pixel = &rendered.rgba_buffer[y * stride + x * 4..y * stride + x * 4 + 4]; + let (r, g, b, a) = (pixel[0], pixel[1], pixel[2], pixel[3]); + assert!( + r > 200 && b < 60, + "buffer is not RGBA-ordered at ({x},{y}): R={r} G={g} B={b} A={a}" + ); + assert_eq!(a, 255, "opaque page should carry alpha 255"); + } +}