Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#3521 closed enhancement (fixed)

Avoid unnecessary memory buffer allocation/copy in Band.ReadRaster() bindings

Reported by: dron Owned by: dron
Priority: normal Milestone: 1.8.0
Component: SWIG (all bindings) Version: svn-trunk
Severity: normal Keywords: swig
Cc: Ari Jolma, hobu, warmerdam, tamas, Even Rouault

Description

There is a possibility for small optimization in Band.ReadRaster?() binding. As of now the ReadRaster_internal() function allocates a memory buffer filling it with data using GDALRasterIO() call. Afterwards SWIG typemaps for Python do a copy of that buffer (using PyString_FromStringAndSize() call) into the Python string object deallocating the initial buffer afterwards.

The attached patch aimed to get rid of the unnecessary buffer allocation. We are creating the Python string with PyString_FromStringAndSize() and reading data directly into its internal data buffer. Also there is no need for ReadRaster_internal() anymore.

My changes breaks the Perl bindings, so there should be some work for them too.

Ari, do you have any ideas how to do the same in Perl? I think it should be something like this:

%typemap(in,numinputs=0) ( void **pBuf ) ( void *pBuf = NULL )
{
  /* %typemap(in,numinputs=0) ( void **pBuf ) */
  $1 = &pBuf;
}
%typemap(argout) ( void **pBuf )
{
  /* %typemap(argout) ( void **pBuf ) */
  $result = sv_2mortal(*$1);
  argvi++;
}

...and in Band.i:

#if defined(SWIGPERL)
    *buf = (void *)newSV( buf_size - 1 );
    char *data = SvPV_nolen( (SV *)*buf );
#endif

Does it correct?

Attachments (1)

readraster.diff (4.1 KB) - added by dron 10 years ago.
Patch to avoid unnecessary memory buffer allocations

Download all attachments as: .zip

Change History (6)

Changed 10 years ago by dron

Attachment: readraster.diff added

Patch to avoid unnecessary memory buffer allocations

comment:1 Changed 10 years ago by warmerdam

Cc: tamas Even Rouault added

comment:2 Changed 10 years ago by tamas

The C# bindings already use direct raster IO and doesn't seem to be affected by this change.

comment:3 Changed 10 years ago by Ari Jolma

I think it is more like

#if defined(SWIGPERL)
     SV *sv = newSV(buf_size);
     *buf = SvPVX(sv);
     ... code to work on sv to make Perl understand what we will do with buf ...
#endif

and the existing typemaps should be changed.

But I don't like that. Messing with sv's internals like that and then giving it to the user is asking for trouble. Especially, when the user can't do anything sensible with it before she changes it into number arrays. I'd rather keep the current solution for Perl for now. Later I'd like to optimize the read/write tile API that Perl bindings have. I have also a good solution in libral+Geo::Raster for this problem.

I guess you can use #ifdefs to keep the current one for Perl and change the Python solution?

comment:4 Changed 10 years ago by Even Rouault

Milestone: 1.8.0
Resolution: fixed
Status: newclosed

r19369 /trunk/gdal/swig/include/ (4 files in 2 dirs): Python bindings: optimize Band.ReadRaster?() and Dataset.ReadRaster?() to avoid extra buffer copy; also add the capability to pass the result buffer such as result_buf = ' '; ReadRaster?(0, 0, 1, 1, buf_obj = result_buf) (#3521)

r19370 /trunk/autotest/gcore/rasterio.py: Test capability to pass the result buffer to Band.ReadRaster?() and Dataset.ReadRaster?() (#3521)

comment:5 Changed 10 years ago by Even Rouault

r19371 /trunk/gdal/swig/include/python/gdal_python.i: Revert partially r19369 : although it seems to work, Python doc states that it is illegal to modify a PyString? that has not just been allocated with PyString_FromStringAndSize(NULL, size) (#3521)

r19372 /trunk/autotest/gcore/rasterio.py: Revert r19370

Note: See TracTickets for help on using tickets.