[gdal-dev] rfc 24 - jpip

[Even Rouault]

Norman,

Sorry for not having given feedback in a more timely way, but the amount of 
changes didn't encourage ;-)

Anyway, here are my comments & questions on the changes done in 
http://trac.osgeo.org/gdal/changeset/16796/sandbox/normanb:

* frmts/jp2kak/jp2kakdataset.cpp : I'm not sure if we must touch this driver. 
I understand that you want to redirect open calls with jpip:// to the 
jpipkakdataset.cpp but this could be disruptive for existing applications. 
Anyway, you should still allow http:// and https:// to be opened by JP2KAK.
* frmts/jp2kak/makefile.vc : should be reverted probably ?
* frmts/makefile.vc : FRMT_jpipkak is unconditionnaly defined. Should probably 
be defined conditionnaly like JP2KAK driver with a dependency on KAKADU and 
CURL.
* gcore/gdal.h :
      - the meaning of each of the 4 GARIO_* values should be Doxygen 
documented
      - the naming of the C mappings of GDALAsyncRasterIO do not look very 
appropriate : GDALGetGDALDataset, GDALGetXOffset, GDALGetYOffset, 
GDALGetXSize, GDALGetYSize, GDALGetBuffer, ... Much too general. They could 
apply to a GDALDataset, so you would need some prefix, like GDALAIO : 
GDALAIOGetDataset(), GDALAIOGetXOffset(), ...
      - I'm wondering if they are not too many of those getter methods. We 
could also have a single method that would update them in a C structure to 
avoid method proliferation. I've no strong opinion about this though.
      - what is the meaning of GDALGetNDataRead() ?
* gcore/gdal_priv.h. What is the justification of the change in 
IsInitialized() ?
* gcore/gdaldataset.cpp :
  - The default implementation of the methods should probably issue a 
CPLError(CE_Failure, CPLE_NotSupported, "Dataset does not support the foo() 
method")
  - All those methods should be carefully documented with Doxygen.
* nmake.opt : your local changes -> should be reverted
* port/cpl_http.cpp : the http_persistent_handle object is global but not 
protected by any mutex mechanism --> thread unsafe if two people use your 
driver from two concurrent threads. Unless you protect it by a lock in your 
driver, but it is a bit messy. A nicer design would be to add a 
CPLHTTPCreateHandle() that would return a curl_easy_init, a 
CPLHTTPFetchWithHandle() that would use that handle, and a 
CPLHTTPDestroyHandle() to clean it. That way we don't need a global variable 
at all in cpl_http.cpp
* port/makefile.vc : What is the justification of the change ?
* swig/include/cpl.i : should be reverted
* swig/include/ogr.i : should be reverted
* swig/java/* : should be reverted
* swig/python/extensions/* : should be reverted.
* swig/python/osgeo/* : should be reverted.
* sandbox/normanb/gdal/swig/java/apps/* : what are all those new files ? Looks 
like they should belong to the imageio-ext project I guess ?
* swig/include/python/gdal_python.i :  I don't understand this comment :  // 
if type is byte typeSize is GDT_Int32 (4) since these are packed into an int 
(BGRA).

On the API itself :
* is the following sequence allowed  ?
	h1 = ds->BeginAsyncRasterIO(...)
	h2 = ds->BeginAsyncRasterIO(...)
  or must h1 be closed before ?
  --> I don't know if is supported by the JPIPKAK driver, but even if it is 
not, should we allow it in the API spec ? exclude it ? leave the decision to 
each driver implementatation ? should be documented

* what is the locking policy of the LockBuffer() / UnlockBuffer() ? You've not 
implemented it in the JPIPKAK driver, but if we define the related functions, 
their precise semantics should be defined.

--> 2 possibilities :
	- we could completely remove them if the buffer is not touched by the driver 
except when GDALGetNextUpdatedRegion() is called. Advantage : no locking 
complexity exposed to the driver. Drawback : possible small loss of 
performance due to some extra buffer copying done in the driver.

	- or if we allow the driver to update the buffer outside of the context of a 
GDALGetNextUpdatedRegion() call, shouldn't they be used when the user wants 
to read from the buffer instead of using them to surround the call to 
GDALGetNextUpdatedRegion() (which should do the necessary locking itself).
      How would locking work with swig bindings ?

* unit of timeout in GetNextUpdatedRegion() : milliseconds ? 

* LockBuffer(int, int, int, int) looks complicated. I'm not sure how someone 
could implement that.

* I guess it is illegal to close a dataset while a BeginAsyncRasterIO() has 
not been terminated with EndAsyncRasterIO() ? should be documented

* we should add a metadata value at the driver level to indicate that a driver 
supports the AIO API. Something like
#define GDAL_DCAP_ASYNC_IO  "DCAP_ASYNC_IO" in gcore.h.

* the JPIP driver only supports AIO as it exposes 0 band. I guess this is OK. 
It could be later extended to support regular IO. 

* I'm wondering if we shouldn't standardize a few of the metadata item you 
currently return in the JPIP domain. I think any driver doing AIO would need 
to return the number of "bands" (JPIP_NCOMPS) and the prefered datatype/bit 
precision too (JPIP_SPRECISION). "COMPONENT_COUNT" and "SAMPLE_PRECISION" in 
a "AIO" domain ? Not sure about the JPIP_NRESOLUTIONLEVELS : equivalent to 
the notion of overviews for regular datasets ?
--> Calling code should ideally not need to be aware of the particularities of 
the driver. 

* stylistic remark : your changes are not always consistant with the general 
indentation of surrounding code. You should avoid using tab characters and 
use spaces instead (4 spacing for each indentation level)

* I hardly looked at jpipkakdataset.cpp. But this is just an 
implementation "detail" ;-) The more important is to design & define clearly 
the new API. 

That's all for now ! All of the above comments are done only from code review. 
Didn't/couldn't compile&test it.

Best regards,

Even

Le Thursday 18 June 2009 17:30:56 Norman Barker, vous avez écrit :
> Hi
>
>
>
> http://trac.osgeo.org/gdal/wiki/rfc24_progressive_data_support
>
>
>
> and the code at
>
>
>
> http://svn.osgeo.org/gdal/sandbox/normanb/
>
>
>
> has been available for some time without any significant comments, are
> we at a point to vote on this RFC and if not then what needs to be done?
>
>
>
> I would like to see JPIP as a format driver within the GDAL trunk, let
> me know if you have any comments or any code changes.
>
>
>
> Thanks,
>
>
>
> Norman