ScanJPEGBlocks() finds too many "Start of Image" markers

[cobalto]

in NITFDataset::ScanJPEGBlocks(), the entire file is scanned to find every instance of marker 0xff,0xd8. However, these bytes can also occur by chance inside of application-specific data payload sections, where they should not be interpreted as block starts. I made a crude patch to search for these application-specific sections, and ignore their data during the block scan.

my patch replaces

        for( i = 0; i < nReadSize-1; i++ )
        {
            if( abyBlock[i] == 0xff && abyBlock[i+1] == 0xd8 )
            {
                CPLAssert( iNextBlock 
                          <= psImage->nBlocksPerRow*psImage->nBlocksPerColumn );

                panJPEGBlockOffset[iNextBlock++] 
                    = panJPEGBlockOffset[0] + iSegOffset + i; 
            }
        }

with

for( i = 0; i < nReadSize-1; i++ )
{
	if (ignoreBytes == 0)
	{
		if( abyBlock[i] == 0xff )
		{
			if ( abyBlock[i+1] == 0xd8 )
			{
				CPLAssert( iNextBlock 
					<= psImage->nBlocksPerRow*psImage->nBlocksPerColumn );

				panJPEGBlockOffset[iNextBlock++] 
				= panJPEGBlockOffset[0] + iSegOffset + i; 
			}
			else if ( abyBlock[i+1] >= 0xe0 && abyBlock[i+1] < 0xf0 ) 
			{
				ignoreBytes = -2;
			}
		}
	}
	else if (ignoreBytes < 0)
	{
		if (ignoreBytes == -1)
			ignoreBytes = abyBlock[i]*256 + abyBlock[i+1];
		else
			ignoreBytes++
	}
	else
	{
		ignoreBytes--;
	}
}

where ignoreBytes = 0; is defined before the while loop.

[cobalto]

I should also add that the additional start-of-image markers are interpreted as JPEG block beginnings, and completely mess up the block indexing of all blocks ocurring after an extraneous marker.

Also note that the JPEG standard defines 0xff,0xe? as application-specific data markers, which are followed by a 2-byte payload length.

[Even Rouault]

I've commited in trunk (r16709) and branches/1.6 (r16710) a variation of your patch that hopefully should be equivalent and a bit more readable ;-) I've tested that it works with the nitf_online_9 test that contains an application data marker. I put a "0xff 0xd8 0xff 0xc2" sequence inside the application data. Without the patch, the decoding fails. With it, this is properly skipped.

[cobalto]

One functional difference in your patch concerns me: it appears to skip over at most nReadBytes after the app data marker. What happens if the app data payload is larger than nReadBytes AND a "0xff 0xd8" occurs towards the end of the app data?

[Even Rouault]

You're absolutely right of course. I totally missed the point that the application data could be across several abyBlock chuncks !

I've commited your original patch in trunk (r16716) and branches/1.6 (r16717)