Bad alignment in the software renderer

[mongoose7]

Has anyone noticed in CBlit.h that executeBlit_Color_16_to_16() has

Code: Select all

u16 *dst = (u16*) job->dst;
u32 c = c0 | c0 << 16;

but it then calls

Code: Select all

memset32( dst, c, job->srcPitch );

memset32 tries to write the 32-bit quantity to the non-aligned 'dst'. (job->dst is actually void *).

I guess everyone is on Intel? Specifically, just before the SIGSEGV, I have dst = 0xXXXXXXX2.

[hybrid]

Yes, this code stems from the software renderers originally, and especially burnings video was implemented and optimized for Intel architecture. But we can add work arounds for this problem. Moving to bug reports for now.

[hybrid]

I've tried for a better solution with the one additional u16 fill either before or after the memset. This should fix the alignment, but I'm not sure. Could you please give it a try? Last SVN trunk.

[mongoose7]

OK, but I'll need a bit of time.

[mongoose7]

Dear Hybrid, what were you smoking?

Is this your code (CBlit.h)?

Code: Select all

935 if ( 0 == (job->srcPitch & 3 ) )
935 {
936     for ( s32 dy = 0; dy != job->height; ++dy )
937     {
938           memset32( dst, c, job->srcPitch );
939         dst = (u16*) ( (u8*) (dst) + job->dstPitch );
940     }
941 }
942 else
943 {
944     const s32 dx = job->width - 1;
945
946     for ( s32 dy = 0; dy != job->height; ++dy )
947     {
948         if ((((long)dst)&0x10)==0)
949         {
950             memset32(dst, c, job->srcPitch);
951             dst[dx] = c0;
952         }
953         else
954         {
955             dst[0] = c0;
956             memset32(dst+1, c, job->srcPitch);
957         }
958         dst = (u16*) ( (u8*) (dst) + job->dstPitch );
959     }
960 }

The SIGBUS occurs at Line 938, the value of dst is XXXXXX02. This hasn't changed.

Line 948 should be

Code: Select all

if ((((long)dst)&3)==0)

If this is not true, you increment dst by 1, but this will only make it dst XXXXXX03 and won't fix anything.

Could I suggest, with all the u16's around, you assume double-byte alignment. So you will need

Code: Select all

else
{
    assert(((int) dst & 1) == 0)
    dst[0] = dst[1] = c0;
    memset32(dst + 2, c, job->srcPitch - 2);
}

Or do a proper job.

[hybrid]

No, it's not my code. Accoridng to the header it's Niko's and Thomas', but as it seems they don't care if someone else uses it. While you should be right with parts of your analysis, I feel like I suddenly dropped any interest in fixing a bit of it. Feel free to provide a working an dtested patch for the ogl-es branch, this change will be immediately reverted to avoid any drawbacks. Thanks for your kind support.

[mongoose7]

Well, I tried a simple fix to that function and it stopped in another. I searched for "(u32*)" and found a lot so it would be quite an effort to fix it for aligned architectures. Using memset32() is good because you can create the pixel colour and blit it. But to fix it requires knowing the endian (which you can do) and rotating the pixel.

It would be good to be able to write four bytes and then blit them with an overlapping memcpy. But those bloody gcc sh*ts ruined that with their bloody half-cocked optimisation. So I'm bowing out at this point. Burning's renderer will do me.

[CuteAlien]

Ok, I just spend a few hours on this because it makes some of our examples crash (when the gui is drawn without transparency in software renderer - example 05 and 25 can have this). Solutions all complicated... my proposal would be rather trivial - just don't try using a 32 bit memset for 16-bit values but use a simple loop instead:

Code: Select all

 
static void executeBlit_Color_16_to_16( const SBlitJob * job )
{
    u16 *dst = (u16*) job->dst;
 
    u16 c0 = video::A8R8G8B8toA1R5G5B5( job->argb );
    u32 pixels = job->srcPitch/2;
 
    for ( s32 dy = 0; dy != job->height; ++dy )
    {
        for ( u32 i=0; i< pixels; ++i )
        {
            dst[i] = c0;
        }
        dst = (u16*) ( (u8*) (dst) + job->dstPitch );
    }
}
 

I did not profile it, but as the old function crashes (not always, but I guess it's always wrong) and this one is just looping over memory (so should be pretty fast) can we go with this maybe for 1.8?