Opened 16 years ago

Closed 16 years ago

#2477 closed defect (fixed)

SWIG: GetHistogram() broken, GetDefaultHistogram() missing

Reported by: warmerdam Owned by: hobu
Priority: normal Milestone: 1.6.0
Component: SWIG (all bindings) Version: 1.5.0
Severity: normal Keywords: histogram
Cc: Ari Jolma, tamas, vjetly

Description

The SWIG bindings lack access to GDALGetDefaultHistogram() though it did exist in the "old gen" python bindings.

The GetHistogram() method on the band in Band.i (for non-c# languages) is declared:

  CPLErr GetHistogram( double dfMin=-0.5,
                     double dfMax=255.5,
                     int nBuckets=255,
                     int bIncludeOutOfRange = 0,
                     int bApproxOk = 1,
                     GDALProgressFunc callback = NULL,
                     void* callback_data=NULL ) {

   int* panHistogram = (int *) CPLCalloc(sizeof(int),nBuckets);
    CPLErr err;
    CPLErrorReset();

    err = GDALGetRasterHistogram(  self, 
                                dfMin,
                                dfMax,
                                nBuckets,
                                panHistogram,
                                bIncludeOutOfRange,
                                bApproxOk,
                                callback, 
                                callback_data);
    return err;
  }

This discards the computed histogram making it useless.

Change History (14)

comment:1 by warmerdam, 16 years ago

I would note that the current situation makes the gdal/swig/python/samples/histrep.py script rather useless.

comment:2 by hobu, 16 years ago

Cc: Ari Jolma tamas added
Milestone: 1.6.0

Added in r14941. I changed the signature of GetHistogram to be more in line with what Tamas was doing in the C# version. Maybe we can consolidate these now?

Adding Ari and Tamas for comments...

in reply to:  2 comment:3 by tamas, 16 years ago

Replying to hobu:

Added in r14941. I changed the signature of GetHistogram to be more in line with what Tamas was doing in the C# version. Maybe we can consolidate these now?

Adding Ari and Tamas for comments...

Which typemap you use for this value by default? I could easily return to the common implementation in case if we agree on the desired typemap name. I see no %apply statement in the common stuff at the moment.

comment:4 by hobu, 16 years ago

As you can see in r14941, I used %typemap(in, numinputs=1) (int nBuckets, int* panHistogram), and I changed the signature of the GetHistogram function to include the int* panHistogram parameter (it didn't before), with the expectation that a typemap with a in,numinputs=1, argout specification would handle the marshalling. Do you know of any other scenarios that behave like this (take in a list of numbers and allocate an int* and use it in GDAL)?

comment:5 by tamas, 16 years ago

I cannot do exactly the same in c# since the multi argument typemaps are not working well in these bindings. But anyway I don't have to do extra allocations the csharp allocated intarray can be used at the c side fairly well. Therefore I only have to apply a typemap against int* panHistogram by mapping the C#/C int array types. Since the signature of GetHistogram is the same we can safely remove the #ifdef SWIGCSHARP and I will declare the %apply statement somewhere at typemaps_csharp.i

BTW: We are tending to change the policy by using unique parameter names instead of inline %apply typemap assignments, yes?

in reply to:  5 ; comment:6 by hobu, 16 years ago

Replying to tamas:

BTW: We are tending to change the policy by using unique parameter names instead of inline %apply typemap assignments, yes?

I don't think so, but I tend to make very specific parameter names/typesmaps for at least the first incarnation of the typemap so as to make sure not to step on anything. As I was asking earlier, if we know of any other functions that exhibit this pattern, the typemap should be made more general.

in reply to:  6 comment:7 by tamas, 16 years ago

Replying to hobu:

I don't think so, but I tend to make very specific parameter names/typesmaps for at least the first incarnation of the typemap so as to make sure not to step on anything. As I was asking earlier, if we know of any other functions that exhibit this pattern, the typemap should be made more general.

Good, but for the same type we will have to use the same typemap (or argument name) so that the bindings would take care of it automatically and shouldn't require to make additional changes.

comment:8 by Ari Jolma, 16 years ago

Typemaps

%typemap(in,numinputs=1) (int len, int *output)
%typemap(argout) (int *len, int *output)

seem to me rather general, but they seem not to be used so far - maybe they are even swig stock typemaps. AFAIU this is the case here (len is the number of buckets wished and output is the outputted list of bucket totals). I suggest using %apply in this case and requiring this typemap.

BTW I commented out the overview methods from Operations for Perl bindings as they require thought and they were not even compiling with swig in Perl bindings now. Those methods should return a new band object or a list of new band objects (bit like this histogram method). But, then, the overview band can also be an input parameter if the overview band is wished to be in a separate file(??).

comment:9 by Ari Jolma, 16 years ago

Arggh! Again bitten by the the default argument values.

If I create a typemap for creating the histogram array (int *output), it gets to be put into a if(len){} block and in the case of default 255 the array is not malloced. Thus it seems to me that it should be already in the bindings, i.e., in the Band.i. We have an example of this in WriteRaster (Dataset.i).

Thus also the typemap I'm suggesting above is of not use if default argument values are used.

comment:10 by Ari Jolma, 16 years ago

Sorry for flooding but I've finally got it working for the Perl bindings. I'm not yet commiting this, take a look:

%apply (int len, int **output) {(int nBuckets, int **panHistogram)};
  CPLErr GetHistogram( double dfMin=-0.5,
                     double dfMax=255.5,
                     int nBuckets=255,
                     int **panHistogram = NULL,
                     int bIncludeOutOfRange = 0,
                     int bApproxOk = 1,
                     GDALProgressFunc callback = NULL,
                     void* callback_data=NULL ) {

    CPLErr err;
    CPLErrorReset();
    if (nBuckets < 1) nBuckets = 1; /* stop idiocy */
    *panHistogram = (int *)CPLMalloc( nBuckets * sizeof(int) );
    err = GDALGetRasterHistogram(  self, 
                                dfMin,
                                dfMax,
                                nBuckets,
                                *panHistogram,
                                bIncludeOutOfRange,
                                bApproxOk,
                                callback, 
                                callback_data);
    return err;
  }
%clear (int nBuckets, int **panHistogram);

I.e. I had to change the outer API to contain int since the malloc is done within this while the free must wait until typemap out.

The Perl typemaps for this are:

%typemap(in,numinputs=1) (int len, int **output) (int *histogram)
{
  /* %typemap(in,numinputs=1) (int len, int **output) */
  /* malloc of *output is done in wrapper fct */
  $1 = SvIV($input);
}
%typemap(arginit) (int len, int **output)
{
  /* %typemap(arginit) (int len, int **output) */
  $2 = &histogram$argnum;
}
%typemap(argout,fragment="CreateArrayFromIntArray") (int len, int **output)
{
  /* %typemap(argout) (int len, int **output) */
  $result = CreateArrayFromIntArray( *$2, $1 );
  argvi++;
  CPLFree(*$2);
}

comment:11 by Ari Jolma, 16 years ago

I consolidated the two calls in r15016, the differences between languages is now in which typemaps they use. I also changed the default nBuckets to 256. I think this is fixed now.

comment:12 by warmerdam, 16 years ago

Cc: vjetly added

I am finding that currently in trunk the following python script:

om osgeo import gdal

ds = gdal.Open('utm.tif')
ds.GetRasterBand(1).GetHistogram()

crashes like this (note panHistogram is NULL):

(gdb) where 5
#0  0x00002b223f50acdc in GDALRasterBand::GetHistogram (this=0x801f90, 
    dfMin=-0.5, dfMax=255.5, nBuckets=256, panHistogram=0x0, 
    bIncludeOutOfRange=0, bApproxOK=1, 

I'm going to dig into this a bit.

comment:13 by warmerdam, 16 years ago

I see there is still no typemap being applied for nBuckets/panHistogram in Band.i. In gdal_wrap.cpp it comes through like this:

  if (obj3) {
    {
      /* %typemap(in) int nBuckets, int* panHistogram -> list hobujunk*/
      arg5 = (int *) CPLCalloc(sizeof(int),arg4);
    }
  }

Note that the expectation in Python is that GetHistogram() should return an array object with the computed histogram.

comment:14 by warmerdam, 16 years ago

Resolution: fixed
Status: newclosed

In trunk (r15322) I have added typemap improvements to get GetHistogram() working in Python.

I have also (in that commit) added GetDefaultHistogram() and SetDefaultHistogram(). The GetDefaultHistogram() is now working in Python, but possibly not in other bindings. The SetDefaultHistogram() is pretty simple, but I don't know how to do the typemap for Python.

I have also added autotest coverage for GetHistogram() and GetDefaultHistogram in trunk (r15321).

I have confirmed that the histrep.py sample script now works for some cases at least.

Note: See TracTickets for help on using tickets.