Band.pm DESTROY method needs to check ref counts about geo-gdal-ffi HOT 13 CLOSED

I'm working on code to reproduce this, but in the meantime I wonder if the Geo::GDAL::FFI::parent approach needs to be modified?

At the moment it is a hash where the keys are the gdal references (memory addresses?) and the values are the perl scalar references. But the values are never accessed anywhere, so it could store a counter variable instead. Then the destroy methods could decrement the counter, deleting the entry when it reaches zero. Alternately it could store an array of refs, with the destroy method splicing out the relevant entry.

The latter might be better if the perl refs are stored for perl's garbage collection processes.

from geo-gdal-ffi.

Code to reproduce is below. It looks like Band.pm is not the culprit - it is in the Geo::GDAL::FFI::Dataset::DESTROY method.

The code below runs without issue.

However, if you comment out the line my $target_ds2 = then the result of the Rasterize function has the DESTROY method called, so the object that was passed as the Destination object now points to a closed dataset and seg faults when accessed.

use 5.010;
use Geo::GDAL::FFI;

local $| = 1;


my $sr = Geo::GDAL::FFI::SpatialReference->new(EPSG => 3067);
my $source_ds = Geo::GDAL::FFI::GetDriver('ESRI Shapefile')
    ->Create('/vsimem/test.shp');
my $layer = $source_ds->CreateLayer({
        Name => 'test',
        SpatialReference => $sr,
        GeometryType => 'Polygon',
        Fields => [
        {
            Name => 'name',
            Type => 'String'
        }
        ]
    });
my $f = Geo::GDAL::FFI::Feature->new($layer->GetDefn);
$f->SetField(name => 'a');
my $g = Geo::GDAL::FFI::Geometry->new('Polygon');
my $poly = 'POLYGON ((1 2, 2 2, 2 1, 1 1, 1 2))';
$f->SetGeomField([WKT => $poly]);
$layer->CreateFeature($f);


my $x_min = 0;
my $y_max = 2;
my $pixel_size = 1;

my $fname = '/vsimem/test_' . time() . '.tiff';
my $target_ds = Geo::GDAL::FFI::GetDriver('GTiff')->Create($fname, 3, 2);
my $transform = [$x_min, $pixel_size, 0, $y_max, 0, -$pixel_size];
$target_ds->SetGeoTransform($transform);

#### COMMENT OUT THIS LINE TO CRASH
my $target_ds2 =
$source_ds->Rasterize({
    Destination => $target_ds,
    Options     => [
        -b    => 1,
        -burn => 1,
        -at,
    ],
});


my $band_r1 = $target_ds->GetBand;

say 'Reading band data';
my $arr_ref = $band_r1->Read;

say 'Got to end';

from geo-gdal-ffi.

Having the parents as values keeps them from being destroyed. Beneath, in GDAL, when for example a dataset is destroyed, its raster datasets are destroyed and that would leave the Perl child objects without GDAL object. In the case of multiple Perl variables to a single GDAL object there must be another solution.

from geo-gdal-ffi.

I think the array of perl refs is a solution that would work. That way one could have one key based on the GDAL object, and then as many array entries as there are perl refs.

It could all be wrapped in a Geo::GDAL::FFI function to handle the addition and removal. That way the hash could also be a package lexical instead of a package global, reducing the chances of outside code messing with it.

Finding the ref to splice when a data set is destroyed can be a simple loop.

from geo-gdal-ffi.

I don't understand why Dataset::DESTROY is called twice. Since

use feature 'say';
{
    package A;
    sub new {
        my $pkg = shift;
        my $self = {};
        say "new $self";
        bless $self, $pkg;
    }
    sub DESTROY {
        say "DESTROY $self";
    }  
}

$a = A->new();
$b = $a;

undef $a;
undef $b;

Outputs

new HASH(0x130f578)
DESTROY 

i.e., one DESTROY, not two.

from geo-gdal-ffi.

The issue in my example is that $target_ds and $target_ds2 are different refs that both point to the same GDAL object. When $target_ds2 goes out of scope or is otherwise destroyed then it calls the gdal cleanup code. The next access to $target_ds then points to a deleted gdal object and a seg fault ensues.

I have some WIP code to use arrays which I will push shortly to a branch on my fork. It appears to work. Some geometry tests fail, but that could be a missing parent registration call.

from geo-gdal-ffi.

It looks like the array approach does not work, as the calls to remove a ref need to know which parent to remove. The objects do not store this.

One possibility is to use Tie::RefHash to index the referents as keys. I'm not convinced this would work, though, as one still needs to know which ref to delete if there are several, unless we simply track ref counts indexed by gdal object.

Another alternative is to use perl objects for the heavy lifting, e.g. Geo::GDAL::FFI::Layer becomes a normal perl object that stores the gdal reference and passes methods on as needed. These objects can store lists of parents as perl refs, and these would automatically be cleaned up as the object goes out of scope. That would also avoid the need for a global hash to track things.

Another thought - how do the python bindings handle this? Or the old SWIG perl bindings?

from geo-gdal-ffi.

I found a better approach for the Rasterize failure in my example. The crash manifests when it is called in void context, as the DESTROY method of the newly created object is immediately called since it is not needed.

The solution is to not return anything in void context.

If that makes sense then I'll update the other utilities and submit a PR.

[ Update - no, this just delays things ]

from geo-gdal-ffi.

If you do it like in the swig bindings:

use Scalar::Util 'blessed';
sub Rasterize {
    my ($self, $args) = @_;
    #my ($path, $dst) = destination($args->{Destination});
    my $options = new_options(\&Geo::GDAL::FFI::GDALRasterizeOptionsNew, $args->{Options});
    set_progress($options, $args, \&Geo::GDAL::FFI::GDALRasterizeOptionsSetProgress);
    my $e = 0;
    my $result;
    my $dst = $args->{Destination};
    if (blessed($dst)) {
        Geo::GDAL::FFI::GDALRasterize(undef, $$dst, $$self, $options, \$e);
    } else {
        $result = Geo::GDAL::FFI::GDALRasterize($dst, undef, $$self, $options, \$e);
    }
    Geo::GDAL::FFI::GDALRasterizeOptionsFree($options);
    if (defined $result) {
        confess Geo::GDAL::FFI::error_msg() // 'Rasterize failed.' if !$result || $e != 0;
        return bless \$result, 'Geo::GDAL::FFI::Dataset';
    }
}

Your code gets to the end.

from geo-gdal-ffi.

Thanks, that works.

I get a failure in my updated test (shawnlaffan@5d0f45e) where I pass $dst and also use the result.

Since the result of that call is undef, the next call to GetBand fails.

That could be addressed through documentation, though.

from geo-gdal-ffi.

It could also throw an exception if $dst is passed but wantarray is defined.

from geo-gdal-ffi.

These subs also look like they need the changes, as they return an object but also support a destination argument:

Warp
VectorTranslate
NearBlack

from geo-gdal-ffi.

Fixes implemented via PRs

from geo-gdal-ffi.