[cairo] [PATCH] OS/2 surface fixes

[Dave Yeo]

Ping (including Peter Weilbacher's quick review and Rich's fix for it.)

On 07/24/10 11:12 am, Rich Walsh wrote:
> Joonas,
>
> On Sun, 18 Jul 2010, M Joonas Pihlaja wrote:
>
>> I'd really prefer for the patch to be split up into several logically
>> independent patches at least.
>
> I've broken the previous patch into 3 parts and added another to deal
> with a regression.  All 4 apply to cairo-os2-surface.c;  one of them
> also patches cairo-os2.h.  They are:
>
> * os2_regression.diff - patch bed2701..f3dda3c removed a block of code in
>    error causing a SIG_SEGV.  Patch c10a5a9..56f888cc restored one line of
>    code to fix the immediate issue but failed to restore the associated
>    error checking.  This patch restores the code to its original state.
>
> * os2_cleanup.diff - this improves error checking, eliminates redundant
>    code, and ensures OS/2 APIs are used correctly.
>
> * os2_24bpp.diff - the existing code contains a work-around for older
>    video drivers that can't handle 32bpp bitmaps.  However, it doesn't
>    comply with the OS/2 requirement that the start of each scan line be
>    DWORD-aligned.  The result is an image that is either skewed or black.
>    This fixes the problem using a more efficient data-transfer method.
>
> * os2_newapi.diff - this introduces 3 new functions to reduce system
>    overhead for cairo surfaces that will be associated exclusively with
>    windows, and makes a minor change in another function to accommodate
>    this change.  It also adds members to cairo_os2_surface_backend that
>    were introduced in v1.9.
>
>> In the small, there are a few bits in the patch I'd not be comfortable
>> with commiting:
>>
>> - The introduction of an unindented do {} while (0) block so that new
>> code can break out of the nonloop.  Please use a goto and a
>> descriptive label instead.
>
> Done.
>
>> - There's a case of s/status = CAIRO_STATUS_SUCCESS/status = 0/.
>
> Done.
>
>> - Missing "Since: 1.10" in the docs (once, it says "Since: 1.6"
>> instead.)
>
> Done.
>
>> - The set_hps() method should return cairo_status_t since it can fail
>> rather than some bogus old-HPS.  Do we even need to provide access to
>> the old HPS at all?  If we really do, then we should use a separate
>> getter instead of conflating setting and getting.
>
> Done.  BTW... an HPS is a direct analog of a cairo surface and consumes
> considerable system resources.  It must be released when no longer needed.
>
>> - It would be good to have some documentation about the ownership
>> contracts of the OS/2 objects in the surface constructors and setters.
>
> Done.
>
>> - The set_hps() method doesn't check for the surface being finished.
>> Granted, neither do any of the other os2 surface methods, but they
>> ought to too.
>
> This is beyond the scope of the current set of patches and will be
> addressed separately.
>
> FWIW...  The patches were generated in the order they are described
> above.  However, each should apply cleanly on its own.
>
> Thanks for your assistance,
>
> Rich Walsh
>

On 07/26/10 01:27 am, Peter Weilbacher wrote:
 > Hi Rich,
 >
 > thanks for taking care of the patches.
 >
 > On 24.07.2010 20:12, Rich Walsh wrote:
 >> * os2_24bpp.diff - the existing code contains a work-around for older
 >>    video drivers that can't handle 32bpp bitmaps.  However, it doesn't
 >>    comply with the OS/2 requirement that the start of each scan line be
 >>    DWORD-aligned.  The result is an image that is either skewed or 
black.
 >>    This fixes the problem using a more efficient data-transfer method.
 >
 > The fUse24Bpp was not in your previous patch. I think this does not make
 > sense as a local static variable, as different surfaces could have
 > different needs regarding 32bit or 24bit drawing. That could happens (in
 > Firefox), if you surface was for onscreen display, the other for 
printing).
 > I think you should make that a flag of the surface structure instead.
 >
 > Everything looks good to me on a quick glance. :-)
 >
 > Cheers,
 >     Peter.
 >

Dave
-------------- next part --------------
A non-text attachment was scrubbed...
Name: os2_24bpp.diff
Type: text/x-patch
Size: 5827 bytes
Desc: not available
URL: <http://lists.cairographics.org/archives/cairo/attachments/20100804/4e962e17/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: os2_cleanup.diff
Type: text/x-patch
Size: 15770 bytes
Desc: not available
URL: <http://lists.cairographics.org/archives/cairo/attachments/20100804/4e962e17/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: os2_newapi.diff
Type: text/x-patch
Size: 7808 bytes
Desc: not available
URL: <http://lists.cairographics.org/archives/cairo/attachments/20100804/4e962e17/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: os2_regression.diff
Type: text/x-patch
Size: 797 bytes
Desc: not available
URL: <http://lists.cairographics.org/archives/cairo/attachments/20100804/4e962e17/attachment-0007.bin>