Please do not reply to this email- if you want to comment on the bug, go to the URL shown below and enter your comments there.
Changed by [EMAIL PROTECTED] http://bugzilla.ximian.com/show_bug.cgi?id=79246 --- shadow/79246 2006-09-05 09:10:00.000000000 -0400 +++ shadow/79246.tmp.27323 2006-09-08 00:47:06.000000000 -0400 @@ -1,12 +1,12 @@ Bug#: 79246 Product: Mono: Class Libraries Version: 1.1 OS: All OS Details: -Status: NEW +Status: ASSIGNED Resolution: Severity: Unknown Priority: Normal Component: libgdiplus AssignedTo: [EMAIL PROTECTED] ReportedBy: [EMAIL PROTECTED] @@ -87,6 +87,220 @@ moving by Z bytes would results in a different X, Y pixel position (as this seems to be the main motivation of BitmapData). [3] Unless we're 100% sure about this being a bug we should strive for compatibility with MS implementation. People won't like having Mono/MS dependant code when using System.Drawing. + +------- Additional Comments From [EMAIL PROTECTED] 2006-09-08 00:47 ------- +In reply to [1]: There seem to be a number of confusions in the +namings of bitmap files, but the ones I put there are definitely of +the bit depth they claim to be -- 1bit.png and 4bit.png. Of course, +that doesn't help when you need to test the BMP codec ;-) +-------------------- +In reply to [2]: The design of BitmapData has one main motivation: to +be able to guarantee word-alignment of scan lines. With 32bpp and +64bpp pixel formats, this is trivial, because the exact number of +bytes needed will just happen to be an exact multiple of the word +size, but with smaller colour depths, any byte offset is possible. +With very low depths (anything less than 8), there isn't even a +guarantee that the end of a scan line will be on a byte boundary. + +The reason BitmapData wants to align each scan with a word boundary +is that the Windows GDI has that requirement (and remember, GDI+ and +System.Drawing were designed by Windows-centric programmers). So, +padding bytes will be used to separate the end of the scan line. So, +without documenting a strict method of computing the amount of +padding that will be used (and for sure, this wouldn't be impossible, +but it hasn't been done), how, then, do programs find the start of +each successive scan line? + +That's where the "Stride" member comes into play. No matter what the +codec actually does, as long as it places enough bytes for a scan at +each multiple of "Stride" within the range specified by the "Height" +member, a consumer can find the scans & process them. Assuming that +the size of the pixels is known (say, 8 bpp), the following code +snippet is a correct loop for processing a bitmap directly: + +Rectangle lock_rectangle = ...; + +BitmapData data = null; + +try +{ + data = bmp.LockBits( + lock_rectangle, + ImageLockMode.ReadWrite, + PixelFormat.Format8bppIndexed); + + byte *data_ptr = (byte *)data.Scan0.ToPointer(); + + for (int y=0; y < data.Height; y++) + { + byte *scan_ptr = data_ptr + y * data.Stride; // <-- KEY IDIOM + + for (int x=0; x < data.Width; x++) + scan_ptr[x] = process_pixel(x, y, scan_ptr[x]); + } +} +finally +{ + if (data != null) + bmp.UnlockBits(data); +} + +So, if you remove any assumption about how the codec is going to +compute the stride (maybe the author was lazy and chose to always +allocate the maximum required stride for the pixel formats, even if +the file in question used less -- not good code, but certainly not +incorrect as far as the BitmapData structure is concerned), you +really have no idea, short of doing complex calculations, where in +the bitmap an offset farther than the end of the current scan will +end up. In fact, the offset might not even be inside of the bitmap! +Almost certainly, the code which advances by 1009 bytes every +iteration will eventually hit unused bytes between scans. + +Anyway, to give a specific example, that code which +reads "almogaver24bits.bmp" gets a stride of 520 when run on MS's +runtime (the next multiple of 4 after 519, the number of bytes in +each scan). Therefore, the scan y=1 starts at offset 520 and runs up +to offset 1039. Offset 1009 into the bitmap data will be offset 489 +into the scan, which corresponds to the blue component of the pixel +at (163, 1) (assuming BGR order in-memory). + +When the same code is run on mono, a quirk of the bitmap codec +(actually quite possibly code I wrote :-) gives the data a stride of +692 bytes. This means that 173 bytes per row are unused. Yes, this is +probably a bug and probably should be fixed, and fixing it will +probably make the test case start working, but that doesn't make the +test case correct! Anyway, with a stride of 692, offset 1009 will be +317 bytes into the second scan line, which corresponds to the red +component of the pixel at (105, 1) (again, assuming BGR order in- +memory). + +To summarize: + + When the stride is 520, offset 1009 is in pixel (163, 1) + When the stride is 692, offset 1009 is in pixel (105, 1) +-------------------- +In reply to [3]: Okay. I will adjust the BMP codec so that 32bpp +images get loaded as PixelFormat.Format32bppRgb. I found +justification for this at: + +http://www.codeproject.com/cs/miscctrl/TransButtonNetDemo.asp + +The advantage of this approach is that it is compatible with the work- +around on that page: Though the pixel format is set to not use alpha, +the bytes are still loaded, and manually transferring them to a +bitmap with the correct pixel format will give people the +transparency they want. Default drawing functions will have the alpha +bytes forced to 255 when painting the bitmap. +-------------------- +Further notes: I check out some of the other [NotWorking] tests in +the ./System.Drawing/Test/System.Drawing.Imaging directory, and here +are my analyses: + +GifCodecTest.cs: + Bitmap8bitsData(): + - It is loading a file called "bitmaps/nature24bits.gif". One +thing I'm pretty sure of is that GIFs cannot be 24-bit. :-) Since I +added native support for indexed pixel formats, the bitmap will +actually be loaded at 8bpp. + - The Bitmap object is then locked at 24bpp. The pixel stream +coded I added will convert the data from 8bpp to 24bpp without +problems. Therefore, the test really isn't doing much of any 8bpp +testing. + - In addition to these issues, this test suffers the same stride +assumption as the NotWorking tests in TestBmpCodec.cs. + Save(): + - The test is constructing a 32-bpp RGB image and then saving it +using an 8bpp codec -- this will be troublesome at best, since we +have little or no control over the quantization to 8-bit data +(especially how the palette is generated). The test was probably +written before my indexed pixel formats patch, so it could be +rewritten now to create a Bitmap using PixelFormat.Format8bppIndexed +and then directly draw some things onto it by locking the bits and +setting a palette. The resulting data should round-trip perfectly. + - It saves the test image to an actual disk file -- why not save +it to a MemoryStream instead? I suppose one could argue that if the +test bombed catastrophically, it would leave a file around which +might contain evidence... + +IconCodecTest.cs: + The tests in this file are all marked as [Ignore] with the +message "NotWorking". This could be fixed by adding an icocodec.c to +libgdiplus :-) The following URL contains details about the layout +of .ICO files; it looks like it shouldn't be too hard to adapt +existing codecs, though some testing would be required to find out +exactly what MS's implementation does with regards to masks & +multiple icon sizes. + +http://msdn.microsoft.com/library/default.asp?url=/library/en- +us/dnwui/html/msdn_icons.asp + + After a codec has been created, these tests will still need to be +changed, because they suffer the same stride assumption as the +NotWorking tests in TestBmpCodec.cs. + +PngCodecTest.cs: + Bitmap1bitData() and Bitmap4bitData(): + - These tests suffer the same stride assumption as the NotWorking +tests in TestBmpCodec.cs. + +JpegCodecTest.cs: + Bitmap24bitData(); + - This test loads a BMP file, not a JPEG file! + - This test suffers the same stride assumption as the NotWorking +tests in TestBmpCodec.cs. + +TiffCodecTest.cs: + Bitmap32bitsData(): + - This test suffers the same stride assumption as the NotWorking +tests in TestBmpCodec.cs. + +There is clearly a common thread among the [NotWorking] tests :-) I +think a reasonable fix would be to alter the tests to read pixels by +(x, y) instead of locking the bits and randomly peeking at them. The +only question is how to pick the pixels. One way to avoid patterns +would be to use a pseudo-random number generator. Obviously, +System.Random is out of the question, because its implementation +differs between Microsoft's corlib and ours, but there are freely- +available alternatives to System.Random, and if those aren't +suitable, we could always roll our own -- after all, we don't need +cryptographically-sound numbers, but rather just a bit of +stochasticism to avoid possible patterns. + +One possible alternative is at: + +http://www.codeproject.com/csharp/FastRandom.asp + +One way to integrate this would be: + +FastRandom r = new FastRandom(0xCAFEBABE); + +int x=0, y=0; + +Color[] expected_pixels = new Color[] { <insert manually-obtained +values here> }; + +for (int i=0; i < expected_pixels.Length; i++) +{ + if (y >= bmp.Height) + Assert.Fail("Fell off the end of the bitmap; the data and test +code are probably out of sync"); + + Assert.AreEqual(bmp.GetPixel(x, y), expected_pixels[i], +string.Format("[{0}, {1}]", x, y)); + + x += r.Next(bmp.Width); + if (x >= bmp.Width) + { + // Wrap around to the next row + x -= bmp.Width; + y++; + } +} + +This particular approach would end up comparing roughly (Height * 2) +of the pixels in the bitmap. Adjust the upper bound of the r.Next() +call in the middle of the loop to compare greater or fewer pixels. + _______________________________________________ mono-bugs maillist - [email protected] http://lists.ximian.com/mailman/listinfo/mono-bugs
