Opened 16 years ago

Closed 16 years ago

#2039 closed defect (fixed)

RawRasterBand::IWriteBlock not testing for complex

Reported by: JaredRubinAZ Owned by: Mateusz Łoskot
Priority: normal Milestone: 1.5.1
Component: GDAL_Raster Version: unspecified
Severity: normal Keywords: VRT raw
Cc: warmerdam, Even Rouault

Description (last modified by warmerdam)

In RawRasterBand::IWriteBlock there is a test for complex data type when copying into the buffer, but no test for complex data at the end of the method when reaccessing the buffer. This looks like a simple fix...unless I don't understand the purpose of rawdataset.cpp

/* -------------------------------------------------------------------- */
/*      Byte swap (if necessary) back into disk order before writing.   */
/* -------------------------------------------------------------------- */
    if( !bNativeOrder && eDataType != GDT_Byte )
    {
        if( GDALDataTypeIsComplex( eDataType ) )
        {

...
...
...



/* -------------------------------------------------------------------- */
/*      Byte swap (if necessary) back into machine order so the         */
/*      buffer is still usable for reading purposes.                    */
/* -------------------------------------------------------------------- */
    if( !bNativeOrder && eDataType != GDT_Byte )
    {

so need to added the following:

       if( GDALDataTypeIsComplex( eDataType ) )
        {
            int nWordSize;

            nWordSize = GDALGetDataTypeSize(eDataType)/16;
            GDALSwapWords( pLineBuffer, nWordSize, nBlockXSize, nPixelOffset );
            GDALSwapWords( ((GByte *) pLineBuffer)+nWordSize, 
                           nWordSize, nBlockXSize, nPixelOffset );
        }

Attachments (2)

vrtrawlink.py (7.4 KB ) - added by Even Rouault 16 years ago.
Modified version with vrtrawlink_5 added
cfloat32.raw (16 bytes ) - added by Even Rouault 16 years ago.
New data file for vrtrawlink_5

Download all attachments as: .zip

Change History (9)

comment:1 by warmerdam, 16 years ago

Cc: warmerdam added
Component: defaultGDAL_Raster
Description: modified (diff)
Keywords: VRT raw added
Milestone: 1.5.0
Owner: changed from warmerdam to Mateusz Łoskot
Priority: highestnormal

Mateusz,

Could you address this? We will need to add a test of raw complex files (using the VRT "raw dataset" support would likely be best) for the test suite to know if there is future reversion in handling of complex raw files.

comment:2 by Even Rouault, 16 years ago

I've commited the fix in r13200.

I'm attaching a version of rawdatalink.py with a new test (vrtrawlink_5) and a new data file (gdrivers/data/cfloat32.raw) that enable to test the bug. The bug is very difficult to hit indeed. The principle of this test is that : 1) we write a block 2) we exhaust the block cache by writing a huge amount of the data 3) we reread the block written at step 1) by computing its checksum. In fact, it's not reread from disk, but from the temporary buffer maintained by the rawrasterband.

As it, this test is probably not appropriate for autotest, but it demonstrates the correctness of the fix.

by Even Rouault, 16 years ago

Attachment: vrtrawlink.py added

Modified version with vrtrawlink_5 added

by Even Rouault, 16 years ago

Attachment: cfloat32.raw added

New data file for vrtrawlink_5

comment:3 by Even Rouault, 16 years ago

Oh, "of course", if you run vrtrawlink_5 on a big-endian machine, it will not hit the bug. You'd have to change line 162.

comment:4 by Even Rouault, 16 years ago

Cc: Even Rouault added

comment:5 by Mateusz Łoskot, 16 years ago

If I understand the status correctly, Even has already fixed (Thanks Even!) this bug and committed fix to trunk (r13200). Looks like Even has taken over and cleaned this issue, so I'm not sure what I could add to it. Is it ready to close?

comment:6 by Mateusz Łoskot, 16 years ago

Status: newassigned

Honestly, I have no idea on how to test this issue well. Obviously, Even's vrtrawlink_5 test does not come in handy for autotest, but as Even explained on the #gdal:

a simple test for complex data could be fine, but will not propably test what was the bug. I've run out of concepts about how to test this issue it properly.

Any comments would be helpful.

comment:7 by warmerdam, 16 years ago

Resolution: fixed
Status: assignedclosed

Ok, we will skip preparing a regression test for this.

Note: See TracTickets for help on using tickets.