Repository navigation
fix(dds): swap red and blue back where ImageMagick reads them swapped (#621) - #627
Conversation
Uncompressed 32-bit DDS files stored R, G, B, A (masks 0xFF, 0xFF00, 0xFF0000, 0xFF000000, as DirectXTex writes R8G8B8A8 without a DX10 header) came out with red and blue swapped (#621). ImageMagick's DDS coder compares the alpha mask with a mistyped constant (0xff00000) and reads these pixels as B, G, R, A, with or without the alpha flag. After ImageMagick reads a DDS, Filee looks at the first 128 bytes of the file and, for exactly this layout, swaps red and blue back in every frame read: when converting (ImageReader) and in the crop tool's preview and save (CropService). Whether the bundled ImageMagick still reads the layout swapped is checked once per alpha flag on a 1 x 1 picture, so a fixed coder is left alone. Other files are not touched, and no file is accepted or refused differently.
rahul05ranjan
left a comment
There was a problem hiding this comment.
No .NET on my machine, so I traced this — but the premise is verifiable against ImageMagick's own source, and it holds up exactly as the comment in DdsChannelOrder.cs describes.
In current coders/dds.c on ImageMagick main, ReadUncompressedRGBAPixels (line 2677) tests the layout with:
is_rgba=IsBitMask(dds_info->pixelformat,0x000000ff,0x0000ff00,0x00ff0000,0xff00000)
0xff00000 is seven hex digits — 0x0FF00000, not 0xFF000000 — so an R8G8B8A8 file (R in the low byte, a_bitmask = 0xFF000000) fails the test, misses the R8G8B8A8 branch, and lands in the final else, which reads the bytes as B, G, R, A. That's the swap you're correcting, and the typo is still live upstream.
The subtle part you got right, which is easy to miss: the no-alpha variant also needs the fix, but for a different reason. DDPF_RGB without DDPF_ALPHAPIXELS dispatches to ReadUncompressedRGB, whose is_rgb test (line 2438) is IsBitMask(..., 0x000000ff, 0x0000ff00, 0x00ff0000, 0x0000000) — it expects the alpha mask to be zero, but these writers still declare a_bitmask = 0xFF000000, so the test fails and the else branch reads B, G, R again. Probing both layouts at runtime instead of assuming one bug was the right call — and it means the fix disarms itself per-layout if upstream corrects either constant. Worth reporting the 0xff00000 typo upstream, incidentally; your Lazy probe means a fixed ImageMagick needs no change on your side.
Other things I checked:
LayoutOf's offsets match the DDS header layout exactly (size at 4, pfflags at 80, bitcount at 88, masks at 92–104), and the layout match requires the full R,G,B,A mask set plusDDPF_RGBset andDDPF_FOURCCclear — so only the one DirectXTex-style uncompressed layout is touched. The 128-byte header read is bounded (ReadAtLeastwiththrowOnEndOfStream: false) and IO errors degrade toLayout.Other, i.e. unchanged behavior.- The swap math is right:
ToByteArray(PixelMapping.RGBA)→ImportPixelswith BGRA mapping (or RGB↔BGR whenHasAlphais false) reverses exactly the R/B bytes, nothing else. - In
CropService, the guardreadSettings.Format == MagickFormat.Ddsis safe —CreateReadSettingsalways returns an instance (the ternary above it is onFormat != Unknown, not on null). - The header/flags the
ReadsSwappedprobe writes (0x100F, size 124, pfsize 32, masks) are a valid minimal DDS, so the probe genuinely exercises the buggy branch rather than an error path;MagickException → falseis the safe default.
One boundary worth knowing, not a regression since the rest of the pipeline gates the same way: the correction fires only when the extension/format id says DDS. A file whose content is this DDS layout but whose extension maps to MagickFormat.Unknown gets read by content sniffing — ImageMagick still runs the DDS coder on it and still swaps it — but readSettings.Format won't be Dds, so Correct never runs. Same story for formatId == "dds" in ImageReader. If that case ever shows up in the wild, keying on image.Format after the read would cover it.
On the security lens: the new code reads a bounded 128-byte header from untrusted files with bounds and exception handling, and the "correction" only permutes in-memory pixel bytes — no new parsing of untrusted data beyond what ImageMagick already did. Clean.
|
@rahul05ranjan Thanks for tracing it through ImageMagick's source, and for checking the header offsets and the swap.
|
|
@SeoJCalive 님, 머지했어요. 고마워요! 번들 ImageMagick으로 1×1 그림을 직접 읽어 보고 바꿀지 정하는 방식이라, 나중에 Magick.NET이 고쳐져도 색이 다시 뒤집히지 않는 점이 좋네요. "Not in this PR"에 적어 주신 HTML/전자책/glTF 안의 DDS는 후속 이슈로 열어 주시면 좋겠어요. |
What does this change?
Fixes #621
Uncompressed 32-bit DDS files whose pixels are stored R, G, B, A (masks R=
0xFF, G=0xFF00, B=0xFF0000, A=0xFF000000, the layout DirectXTex writes for R8G8B8A8 without a DX10 header, common in game textures) came out with red and blue swapped. Filee now swaps them back for exactly this layout.The cause is in ImageMagick's DDS coder (
coders/dds.c):ReadUncompressedRGBAPixelschecks for this layout with the alpha mask written as0xff00000(seven hex digits), so a file with the alpha flag never matches and is read as B, G, R, A.ReadUncompressedRGBPixelsexpects an alpha mask of 0, so the same layout without the alpha flag is read as B, G, R.What changes:
DdsChannelOrder(Engines): after ImageMagick has read a DDS, it reads the first 128 bytes of the file. Only when the header is exactly this layout (signature, header size,DDPF_RGBwithout FourCC, 32 bits, the four masks) does it swap red and blue back, in every frame read.ImageReader.Read, which every conversion uses (including the multi-page TIFF merge), and from the crop tool's preview and save (CropService).Type of change
How was it tested?
dotnet format --verify-no-changesdotnet test:ImageFormatTests24/24 andCropServiceTests8/8 (new tests below), Core 635/635, App 1381 passed, 11 skipped, 1 failed:RenderTests.Engines_page_shows_versions_descriptions_and_download_speed, which also fails on main on this Linux machine because it finds the system Ghostscript and LibreOffice. Release build has the same warnings as main.ImageFormatTests: the layout with and without the alpha flag converts to PNG with its own colours; the usual B, G, R, A layout keeps its colours; the swap only happens when ImageMagick reads the layout swapped.CropServiceTests: both variants preview in their own colours, and red, green and blue 4 x 4 blocks cropped and saved (DXT) keep their channel.fileeCLI.Tex_0088_0.ddsto PNG: RGB equals the reporter's Photoshop PNG over the whole image (max difference 0; 58 before the fix), alpha equals the file's own.Screenshots (UI changes)
No UI change. The reporter's
Tex_0088_0.ddsconverted to PNG:The after image is identical to the reporter's Photoshop export (max difference 0 in every channel).
Checklist
src/Filee.App/Assets/i18n/*.jsonfiles (none added)THIRD-PARTY-NOTICES.md(none added)Not in this PR
<img>turned into DOCX/HWPX, pictures going throughEbookFiles.CommonImage, glTF textures from OBJ/FBX) are opened with ImageMagick directly and are still swapped.DdsChannelOrder.Correctcan be called there in a follow-up.