[Mono-winforms-list] libgdiplus/System.Drawing patch: nativesupport for indexed Bitmaps

[Jonathan Gilbert]

At 12:41 AM 07/03/2005 -0500, Miguel de Icaza wrote:
>Hello,
>
>> Please find attached the following files:
>> 
>> * libgdiplus.diff (patch -p0 in ./libgdiplus)
>> * System.Drawing.diff (patch -p0 in ./mcs/class/System.Drawing)
>> * lockbits.tgz (tar zvfx in ./)
>
>From my quick review of the code, this looks great.   
Thanks! I tried to follow the existing code style as closely as possible.
It's quite different from my own personal preferences, but I would be less
than a man if I let that interfere with my programming :-)

>The has_next and stream processing routines seem to only be used in one
>routine, so am left wondering if it might not be best to have those as
>static functions so the compiler could inline some of it.
It undoubtedly would perform better with inlining than without :-) One of
the things that ran through my mind with the pixel stream functions is that
possibly other people doing work on other parts of libgdiplus might find
the functionality useful, and that it would thereby end up being a publicly
exported function. However, I didn't really spend too much time thinking
about the future of pixel_stream stuff; I was more interested in getting
the bugs ironed out :-)

Is GCC not able to inline functions that are not marked 'static', even when
they are in the same source code file? I could investigate..

>What is the performance impact on applications after this change?
I don't have much to test with. However, the majority of applications will
be using a 32-bit RGB or ARGB pixel format, and will be locking with the
same format as the bitmap. With such a format, gdip_can_window_without_copy
will always return true; the pixels are always byte-aligned (DWORD-aligned,
even), and no format conversion is required. As such, it will simply create
a window onto the same bits, much as the old code did it, so performance
should not really change. (The code that does the windowing is much more
direct now, though; GdipBitmapLockBits itself checks if it can do the
windowing, and creates the window in the 'true' path of the 'if' statement.
With the old code, it was buried away inside
gdip_bitmap_change_rect_pixel_format, about 100 lines into the function. In
fact, until I got around to renovating that function, I wasn't even aware
that it DID use the windowing technique :-)

Performance comparisons for programs using indexed pixel formats aren't
really possible, because without the patch, such programs can't run at all
under mono :-)

>In any case, the code looks very solid, I would like to offer you an SVN
>account as well.
That would be nice :-) Where do I sign up?

Jonathan Gilbert