Even, Thank you! I was looking for such a review.
I will wait a few days for more comments and then I will go through you suggestions and change the code as required. Thanks, Norman -----Original Message----- From: [email protected] [mailto:[email protected]] On Behalf Of Even Rouault Sent: Thursday, June 18, 2009 2:11 PM To: [email protected] Cc: Norman Barker Subject: Re: [gdal-dev] rfc 24 - jpip 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 _______________________________________________ gdal-dev mailing list [email protected] http://lists.osgeo.org/mailman/listinfo/gdal-dev _______________________________________________ gdal-dev mailing list [email protected] http://lists.osgeo.org/mailman/listinfo/gdal-dev
