[gdal-dev] rfc 24 - jpip
Norman Barker
nbarker at ittvis.com
Thu Jun 18 16:16:21 EDT 2009
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: gdal-dev-bounces at lists.osgeo.org [mailto:gdal-dev-bounces at lists.osgeo.org] On Behalf Of Even Rouault
Sent: Thursday, June 18, 2009 2:11 PM
To: gdal-dev at lists.osgeo.org
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
gdal-dev at lists.osgeo.org
http://lists.osgeo.org/mailman/listinfo/gdal-dev
More information about the gdal-dev
mailing list