ECW driver enhancements.

[jcztery]

I work for Intergraph in the team that works on ECWJP2 SDK. The driver has been integrated with our internal driver version, and also:

1. Improved performance of supersampling. Actually ECWJP2 SDK supports it. Supersampled requests are now 50% faster.
   

2.Fixed performance problem when reading single band unadvised region line by line. Reading single band line by line is now 10 or more times faster.
3.Added partial support for ECWJP2SDK 5.0.

[jcztery]

Changed files.

[Even Rouault]

jcztery,

I have done a preliminary review of the code contribution, and have already a few comments/questions :

1) Did you test compiling and running against various ECW SDK versions ? For example with the 3.3 version. This is still an important use case for many people, because it is the latest version available for Linux and MacOSX.

2) Related to 1), with which ECW SDK version have the performance improvements been benchmarked ? Are they true for all versions ? Do you have some benchmark scripts that could be used to reproduce them ?

3) Did you test running the autotest/gdrivers/ecw.py regression tests ?

4) I think there's perhaps an issue related to error messages management. In ecwcreatecopy.cpp, in the ECWSDK_VERSION >= 50 case, I see oError.GetErrorMessage(), whereas in ecwdataset.cpp, it is oError.GetErrorMessage().a_str(). I think that a utility function to emit a CPLError() from an SDK error could be usefull to avoid those multiples #ifdef . Or perhaps NCSGetErrorText(oError.GetErrorNumber()) should be used if it is equivalent? (and as it seems to be unchanged between SDK versions).

5) You have changed NCS_ECWSDK_VERSION_STRING to NCS_ECWJP2_VERSION_STRING, but in the include files of the 4.2 SDK I only see NCS_ECWSDK_VERSION_STRING. Should be something like the following I believe :

#ifdef NCS_ECWJP2_VERSION_STRING
        /* SDK >= 5.0 */
        osLongName += NCS_ECWJP2_VERSION_STRING;
#elif defined(NCS_ECWSDK_VERSION_STRING)
        /* SDK 4.X */
        osLongName += NCS_ECWSDK_VERSION_STRING;
#else
        osLongName += "3.x";
#endif    

6) In the CreateCopy() case with SDK >= 50, I see "psClient->nFormatVersion = 3". So I assume this is the new version of the ECW file format, that can only be read with SDK >= 50 ? Would it be possible for SDK >= 50 to still produce the old ECW file format by setting psClient->nFormatVersion = 2 ? That could be proposed as a creation option for people that need format backward compatibility.

7) I see that occurrences of "int nResFactor = 1 << (iOverview+1);" have disappeared from ECWRasterBand::AdviseRead() and ECWRasterBand::IRasterIO(). This looks suspicious to me that the overview factor is no longer taken into account when (overview) bands delegate their work to the dataset. Did you make tests with overview bands ?

8) The following test is probably not sufficient. I think that, in addition to eBufType == eRasterDataType, the nPixelSpace should also be tested to be equal to nDataTypeSize.

	bool direct = (eBufType == eRasterDataType);
	if (direct)
	{
		return ReadBandsDirectly(pData, nBufXSize, nBufYSize,eBufType, nBandCount, nPixelSpace, nLineSpace, nBandSpace);
	}

9) About supersampling, are you confident that the removal of the special case will also work for 3.3 SDK ? I imagine that FrankW ran into an issue at that time.

10) To which extent is the support for ECWJP2SDK 5.0 partial ?

[Even Rouault]

jcztery, did you have the opportunity to have a look at the above questions in the ticket ?

[jcztery]

Replying to rouault:

jcztery, did you have the opportunity to have a look at the above questions in the ticket ?

rouault, sorry for the slow response. Please find answer below.

1. I tested it on version 4.3 of ECWJP2 SDK. Some of the tests were not passing on previous SDK versions (4.2, 3.3) (even with unmodified ECW GDAL Driver).
2. 4.3 ECWJP2 SDK - tested both Reads and Writes. Tested reads on 5.0 as well.
3. Yes, see 1 however. Also there are some tests that depends on bits being the same after compression. This is something that is going to fail since the encoding/decoding algorithm may change slightly from version to version. And this might result in pixel values being changed. After all the compression is lossy.
4. Yes. This is one thing that should have been done better.
5. Yes. We have changed it in 5.0. So the easiest change would be to define NCS_ECWJP2_VERSION_STRING as NCS_ECWSDK_VERSION_STRING in versions prior 5.
6. You are right, this should be an option when 5.0 is fully supported.
7. We wanted to delegate each request to the SDK. The logic of selecting overview band and then requesting it for data might have been useful for TIFF files, but it is not the case in ECW/JP2 files. These requests eventuated in calling SetView/ReadLine anyway. So what I did is just make the path to ECWJP2 SDK shorter.
8. I think nPixelSpace is ok, as it is derived from buffType.
9. I am not sure about that.
10. It should compile against 5.0 and reads should work. Actually everything SHOULD work on 5.0, but it hasn't been tested.

Please note, that we would like to update again when 5.0 SDK is out.

[Even Rouault]

OK, trunk r25104 : "ECW: use ECW SDK to do super-sampling for SDK >= 4.X; expose 256x256 block dimension instead of scanline; add partial support for ECW SDK 5.0 (mostly based on the patch provided in #4822, but with a few non-trivial changes and fixes)"

Some of my previous comments turned to be true (but admitedly not checked in the regression tests, that I've also extended to catch those errors ). Here's the list of the main changes in the above commit w.r.t to the submitted patch :

• Doing supersampling with SetView() didn't work in ECW SDK 3.3, so for that case, I've reverted to using the old ECWRasterBand::IRasterIO() implementation to workaround the issue. Supersampling wasn't tested in ecw.py; it is now.
• The removal of the nResFactor in the raster band methods made calls from overview bands to misbehave. I/O from raster bands wasn't tested either; it is now
• The IReadBlock() implementation was still assuming blocks to be one scanline. IReadBlock() wasn't tested either; it is now.
• I've added a ECWReportError() method to centralize calls to CNCSError::GetErrorMessage()
• I've added a ECW_FORMAT_VERSION creation option (for SDK >= 5.0)
• I've extended the scope (and simplified the test) of the "tricky" optimization in ECWDataset::IRasterIO(), so as to have scanline by scanline reads to perform as well as before. With the submitted patch, I couldn't verify the claim that "Fixed performance problem when reading single band unadvised region line by line. Reading single band line by line is now 10 or more times faster". On the contrary I found the proposed implementation to be much slower (twice slower and even more) when doing a "gdalinfo -checksum the.ecw". I may have missed the particular case you tried to optimize, so for a next time, please provide replicable tests that can be used to check the improvements.

Please test and report if it works, especially with SDK 5.0. I've tested successfully with SDK 3.3 on Linux and SDK 4.x on Windows.

(Please base your next contributions on the updated trunk)

[Even Rouault]

r25105 "Move ECW_FORMAT_VERSION creation option to ECW driver (instead of JP2ECW)"

[warmerdam]

On a loosely related point I have substantially reworked the GDAL configuration logic on unix so it should work essentially out of the box with the 5.0 SDK - see r25591.