[gdal-dev] ENVI Rotation Bug Fix

Wed Mar 1 07:03:04 PST 2017

Even,

I already do direct tests with jp2kakdataset_test.cc with bazel and it
works great.  I can directly probe the methods without worrying about any
other mechanism getting between the test and the class.  e.g. I can
directly mess with Identify, Open and any helpers.  I can also do things
like disable identify so that fuzzing can more quickly get into the guts
and find trouble:

https://trac.osgeo.org/gdal/browser/trunk/gdal/frmts/jp2kak/jp2kakdataset.cpp#L812

GDALDataset *JP2KAKDataset::Open( GDALOpenInfo * poOpenInfo ) {
#ifndef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
    // During fuzzing, do not use Identify to reject crazy content.
    if( !Identify(poOpenInfo) )
        return NULL;
#endif

We are putting resources behind this... we are running fuzzing on jp2kak
and related.  e.g. with >600K core hours of fuzzing this month (> 14e12
iterations).

If I don't break things apart, fuzzing a component goes much slower and is
far less likely to identify issues in the lower level components and libs.

On Wed, Mar 1, 2017 at 6:15 AM, Even Rouault <even.rouault at spatialys.com>
wrote:

> Kurt,
>
>
>
> I don't think the ENVIDataset class is exported (and that's a good thing),
> so direct testing of the class might only be possible if you build without
> --with-hide-internal-symbols.
>
>
>
> Why can't you test through the public GDAL API instead of directly
> depnding on implementation details ? Actually looking at the class
> definitions of the ENVI driver, the only public methods are virtual methods
> from the public API, so there's no advantage on directly depending on it.
>
>
>
> Even
>
>
>
> > Warning... I did a cleanup pass and split out the class definitions into
> a
>
> > header to allow direct C++ testing.
>
> >
>
> > https://trac.osgeo.org/gdal/changeset/37530
>
> > https://trac.osgeo.org/gdal/changeset/37514
>
> > https://trac.osgeo.org/gdal/changeset/37510
>
> >
>
> > I haven't written that testing yet, but it will be similar to this:
>
> >
>
> > https://github.com/schwehr/gdal-autotest2/blob/master/
> cpp/frmts/jp2kak/jp2ka
>
> > kdataset_test.cc
>
> >
>
> > -kurt
>
> >
>
> > On Wed, Mar 1, 2017 at 2:56 AM, Damian Dixon <damian.dixon at gmail.com>
> wrote:
>
> > > Hi Taylor,
>
> > >
>
> > > M_PI is defined pretty much everywhere. The only compiler that you
> have to
>
> > > jump through hoops to get it is Visual Studio where you have to define
>
> > > '#define _USE_MATH_DEFINES'.
>
> > >
>
> > > As Even has mentioned it's also defined in the GDAL portability header.
>
> > >
>
> > > The main reason the use of atan jumped out is that I've run into
> problems
>
> > > with atan on Intel CPUs and GPUs in the past. Also the use of trig' of
> any
>
> > > sort is expensive so I tend to take a look to see if it can be removed
> or
>
> > > moved.
>
> > >
>
> > > I spotted that this is a very old bug so I'm pleased that you've taken
> the
>
> > > time to fix it.
>
> > >
>
> > > Regards
>
> > > Damian
>
> > >
>
> > >
>
> > >
>
> > > On 27 February 2017 at 22:12, Brown, Taylor Alexander <
>
> > >
>
> > > browtayl at oregonstate.edu> wrote:
>
> > >> Damian,
>
> > >>
>
> > >> I agree that it would be preferable to use a predefined constant.
>
> > >> However, I was under the impression that M_PI is a compiler extension
> and
>
> > >> not part of standard C++, so I wanted to make sure it was compatible.
> If
>
> > >> using M_PI is acceptable, I would be happy to change it. I will be
>
> > >> submitting my pull request shortly.
>
> > >>
>
> > >>
>
> > >> Taylor
>
> > >>
>
> > >> On Mon, Feb 27, 2017 at 5:01 AM, Damian Dixon <damian.dixon at gmail.com
> >
>
> > >>
>
> > >> wrote:
>
> > >>> Hi Taylor,
>
> > >>>
>
> > >>> I'm just lurking on the mailing list myself.
>
> > >>>
>
> > >>> I had a look at the code changes, as this is one of the formats I'm
>
> > >>> interested in, and one point springs out at me:
>
> > >>>
>
> > >>> const double PI = atan(1.0) * 4;
>
> > >>>
>
> > >>> I find this line potentially a confusing way to define PI,
> particularly
>
> > >>> when you combine it with (PI/180) later.
>
> > >>>
>
> > >>> Personally I would use M_PI.
>
> > >>>
>
> > >>> Regards
>
> > >>> Damian
>
> > >>>
>
> > >>>
>
> > >>>
>
> > >>> On 27 February 2017 at 11:55, Taylor Alexander Brown <
>
> > >>>
>
> > >>> browtayl at oregonstate.edu> wrote:
>
> > >>>> Greetings GDAL Community,
>
> > >>>>
>
> > >>>> I am developing an application [0] to process georeferenced raster
>
> > >>>> hyperspectral imagery in ENVI format from NASA's Airborne
>
> > >>>> Visual/Infrared Imaging Spectrometer (AVIRIS) [1].
>
> > >>>>
>
> > >>>> The `map info` section of the header files contain an undocumented
> [2]
>
> > >>>> rotation field which is currently ignored by GDAL. As a result, the
>
> > >>>> image is rotated incorrectly when I process it with `gdalwarp` or
> the
>
> > >>>> GDAL Python API. Furthermore, the image is scaled incorrectly when I
>
> > >>>> try
>
> > >>>> to override the geotransform.
>
> > >>>>
>
> > >>>> I documented my experiences on the GIS Stack Exchange [3]. It has
>
> > >>>> previously been described on the GDAL mailing list [4] and bug
> tracker
>
> > >>>> [5]. I believe this issue also affects ASTER imagery [6][7].
>
> > >>>>
>
> > >>>> I have implemented a fix based off of branch `tags/2.1.3` and
> shared my
>
> > >>>> code on GitHub [8]. It reads the `units` and `rotation` parameter
> from
>
> > >>>> the `map info` section and uses these to calculate the geotransform.
>
> > >>>> The
>
> > >>>> scaling was off because the code expected the `units` parameter to
> be
>
> > >>>> the last element in the list, when in my case it was the second to
>
> > >>>> last.
>
> > >>>> You can verify this behavior with the single-band image I have been
>
> > >>>> using for testing [9].
>
> > >>>>
>
> > >>>> You are welcome to evaluate my code and contribute it back to the
>
> > >>>> library. I am a novice at both GDAL and GIS, so there may well be
>
> > >>>> problems. I would be willing to submit a pull request against your
>
> > >>>> GitHub mirror if desired.
>
> > >>>>
>
> > >>>>
>
> > >>>> Peace,
>
> > >>>>
>
> > >>>> Taylor Alexander Brown
>
> > >>>>
>
> > >>>>
>
> > >>>> [0] https://github.com/capstone-coal/pycoal
>
> > >>>> [1] https://aviris.jpl.nasa.gov/
>
> > >>>> [2] http://www.harrisgeospatial.com/docs/enviheaderfiles.html
>
> > >>>> [3]
>
> > >>>> http://gis.stackexchange.com/questions/229952/rotate-envi-hy
>
> > >>>> perspectral-imagery-with-gdal
>
> > >>>> [4] https://lists.osgeo.org/pipermail/gdal-dev/2013-
> January/035146.html
>
> > >>>> [5] https://trac.osgeo.org/gdal/ticket/1778
>
> > >>>> [6] http://yceo.yale.edu/opening-aster-files-envi
>
> > >>>> [7] http://yceo.yale.edu/faq-page#t2n504
>
> > >>>> [8]
>
> > >>>> https://github.com/OSGeo/gdal/compare/tags/2.1.3...browtayl:
>
> > >>>> fix-envi-rotation?expand=1
>
> > >>>> [9] https://drive.google.com/open?id=0BxysdOuBmaIGalY5dVhCa1Y5M0k
>
> > >>>> _______________________________________________
>
> > >>>> gdal-dev mailing list
>
> > >>>> gdal-dev at lists.osgeo.org
>
> > >>>> https://lists.osgeo.org/mailman/listinfo/gdal-dev
>
> > >
>
> > > _______________________________________________
>
> > > gdal-dev mailing list
>
> > > gdal-dev at lists.osgeo.org
>
> > > https://lists.osgeo.org/mailman/listinfo/gdal-dev
>
>
>
>
>
> --
>
> Spatialys - Geospatial professional services
>
> http://www.spatialys.com
>

-- 
--
http://schwehr.org
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osgeo.org/pipermail/gdal-dev/attachments/20170301/22e37532/attachment-0001.html>