4 ms·

To add to this, libraries aren't consistent about convention either. Take the common "RGBA". Is this describing the byte order? Or describing most-significan

To add to this, libraries aren't consistent about convention either. Take the common "RGBA". Is this describing the byte order? Or describing most-significant to least-significant 0xRRGGBBAA, which might be little-endian encoded into a "ABGR" byte order? If you think that's stupid idea and that I'm an idiot for suggesting it, what about 16-bit RGB formats where the G component straddles both bytes? Okay, so those aren't so common anymore - but what about DXT/BC compressed formats which encode 4x4 pixel chunks at a time, and don't cleanly decompose into "channels" in the first place? D3DFMT_A8R8G8B8 (Direct3D 9) is equivalent to DXGI_FORMAT_B8G8R8A8_UNORM (DXGI / Direct3D 10+). Note that one enumeration lists the components in reverse order of the other, and that this is correct! https://docs.microsoft.com/en-us/windows/win32/direct3d10/d3d10-graphics-programming-guide-resources-legacy-formats https://docs.microsoft.com/en-us/windows/win32/direct3d10/d3... . Documentation often completely omits information endian or encoding - and when it doesn't, it's often hidden away where you'll never find it, and usually assumes x86/x64/little-endian processors. The behavior on big-endian machines is best found out through testing - even if the documentation is clear, the documentation stands a good chances of lying, and CI probably doesn't test on big-endian meaning bugs have likely arisen, and there's a good chance your copy of the library is old and doesn't contain any bugfixes. In light of all of the above, RGB vs BGR confusion is one of the most natural points of confusion to run across when dealing with image formats. "Just use the type system!" ignores where these bugs crop up - (de)serialization, converting streams of bytes to types or vicea versa. Someone must write the code, declaration, whatever - and the type system has no means of ensuring that correctly matches whatever not-computer-readable spec that the (de)serialization is supposed to match - and so it will, frequently, be understandably incorrect.

jeffbee 5y ago
Ok, but bring it back to this article. It is marshaling typed struct fields to typed parameters, but because the types are all primitive, the author entered them backwards and the language platform didn’t notice. The type system can prevent this flaw.
- MaulingMonkey 5y ago
  > the author entered them backwards You're mistaken. The author originally suspected - or hypothesized - that was possibly the bug, but the actual cause detailed in the post ended up being different (nondeterministic hashmap iteration over components) and the final patch does not include the initially proposed "fix" of swapping the red and blue components as was shown towards the very top of the article. The original order was correct. Swapping red/blue would've just been canceling out one bug by adding another. https://github.com/SerenityOS/serenity/commit/a10ad24c760bfe713f1493e49dff7da16d14bf39 https://github.com/SerenityOS/serenity/commit/a10ad24c760bfe... > The type system can prevent this flaw. The type system can add some roadbumps to encourage centralizing the conversion of "untyped" disk bytes to typed r/g/b components or a typed color component, but that conversion must still occur somewhere, and it can be written backwards at that somewhere, and one of those somewheres will be inside JPGLoader.cpp. That's the fundamental job of a JPEG loader - conversion of an untyped byte stream into useful types for the rest of the program to use. Additional use of the type system might limit where within JPGLoader.cpp such a component swapping flaw might be likely, but it's never going to prevent it for all of JPGLoader.cpp. The existence of the Color type already makes such a flaw fairly unlikely outside of JPGLoader.cpp and similar conversion points. That's sufficient use of the type system. More within JPGLoader.cpp, while possible, would be overkill based on my own experience with color conversion and swapped component bugs. Hell, to play devil's adovcate, further use of the type system could cause such flaws! The extra code is likely to cause additional reviewer fatigue in code reviews. Reviewer fatigue causes inattention. Inattention causes the reviewer to miss swapped or nondeterministics components. Perhaps such inattention caused this bug! I would be in favor of replacing: const Color color { (u8)block.y[pixel_index], (u8)block.cb[pixel_index], (u8)block.cr[pixel_index] }; with: const auto color = Color::from_rgb((u8)block.r[pixel_index], (u8)block.g[pixel_index], (u8)block.b[pixel_index]); To make it clearer that this code is correct as is. Doesn't add any new types though, just names a constructor in such a way as to imply an argument order, and uses the existing r/g/b union aliases of y/cb/cr in macroblocks (r/g/b is more correct at that point in the code, since ycbcr_to_rgb should've been called already - it's possible this code was written before those union aliases were added?) It's still possible to have a copy+paste bug with my replacement by using r/g/g as input instead of r/g/b or similar, but I've seen such bugs even with per-component types - just need the first copy to be mistaken and copy+pasted all over.

jeffbee 5y ago

Ok, but bring it back to this article. It is marshaling typed struct fields to typed parameters, but because the types are all primitive, the author entered them backwards and the language platform didn’t notice. The type system can prevent this flaw.

MaulingMonkey 5y ago
> the author entered them backwards You're mistaken. The author originally suspected - or hypothesized - that was possibly the bug, but the actual cause detailed in the post ended up being different (nondeterministic hashmap iteration over components) and the final patch does not include the initially proposed "fix" of swapping the red and blue components as was shown towards the very top of the article. The original order was correct. Swapping red/blue would've just been canceling out one bug by adding another. https://github.com/SerenityOS/serenity/commit/a10ad24c760bfe713f1493e49dff7da16d14bf39 https://github.com/SerenityOS/serenity/commit/a10ad24c760bfe... > The type system can prevent this flaw. The type system can add some roadbumps to encourage centralizing the conversion of "untyped" disk bytes to typed r/g/b components or a typed color component, but that conversion must still occur somewhere, and it can be written backwards at that somewhere, and one of those somewheres will be inside JPGLoader.cpp. That's the fundamental job of a JPEG loader - conversion of an untyped byte stream into useful types for the rest of the program to use. Additional use of the type system might limit where within JPGLoader.cpp such a component swapping flaw might be likely, but it's never going to prevent it for all of JPGLoader.cpp. The existence of the Color type already makes such a flaw fairly unlikely outside of JPGLoader.cpp and similar conversion points. That's sufficient use of the type system. More within JPGLoader.cpp, while possible, would be overkill based on my own experience with color conversion and swapped component bugs. Hell, to play devil's adovcate, further use of the type system could cause such flaws! The extra code is likely to cause additional reviewer fatigue in code reviews. Reviewer fatigue causes inattention. Inattention causes the reviewer to miss swapped or nondeterministics components. Perhaps such inattention caused this bug! I would be in favor of replacing: const Color color { (u8)block.y[pixel_index], (u8)block.cb[pixel_index], (u8)block.cr[pixel_index] }; with: const auto color = Color::from_rgb((u8)block.r[pixel_index], (u8)block.g[pixel_index], (u8)block.b[pixel_index]); To make it clearer that this code is correct as is. Doesn't add any new types though, just names a constructor in such a way as to imply an argument order, and uses the existing r/g/b union aliases of y/cb/cr in macroblocks (r/g/b is more correct at that point in the code, since ycbcr_to_rgb should've been called already - it's possible this code was written before those union aliases were added?) It's still possible to have a copy+paste bug with my replacement by using r/g/g as input instead of r/g/b or similar, but I've seen such bugs even with per-component types - just need the first copy to be mistaken and copy+pasted all over.