Opened 12 years ago

Closed 10 years ago

#4575 closed defect (fixed)

Kakadu 7 requires changes to jp2kakdataset

Reported by: Ari Jolma Owned by: warmerdam
Priority: normal Milestone:
Component: ConfigBuild Version: unspecified
Severity: normal Keywords: jp2kak
Cc: rprinceley, bledoux

Description

jp2kakdataset.cpp:2037: lines[c].pre_create() requires two new arguments extend_left and extend_right (see kdu_sample_processing.h:433)

jp2kakdataset.cpp:2150: engines[c].push() does not accept boolean argument after line (see kdu_sample_processing.h:901)

I tried setting extend_left and extend_right to 0 and removing the boolean argument and the driver works but there may be subtle issues.

Change History (18)

comment:1 by rprinceley, 12 years ago

Cc: rprinceley added

comment:2 by bledoux, 11 years ago

We ran into the same issue as ajolma while upgrading to Kakadu 7.2 and had to step back to version 6.4. Are there any plans to provide a fix soon ?

Best Regards.

comment:3 by warmerdam, 11 years ago

Cc: bledoux added
Component: defaultConfigBuild
Keywords: jp2kak added
Status: newassigned

I am not seeing this in trunk on 64bit linux with Kakadu 7.2.2:

warmerdam@gdal:~/gdal/frmts/jp2kak$ make
g++ -fPIC  -g -DDEBUG -DHAVE_SSE_AT_COMPILE_TIME  -Wall  -I/usr/local/google/home/warmerdam/gdal/port -I/usr/local/google/home/warmerdam/gdal/gcore -I/usr/local/google/home/warmerdam/gdal/alg -I/usr/local/google/home/warmerdam/gdal/ogr -I/usr/local/google/home/warmerdam/gdal/ogr/ogrsf_frmts -I/usr/local/google/pkg/kakadu-7.2.2/coresys/common -I/usr/local/google/pkg/kakadu-7.2.2/apps/compressed_io -I/usr/local/google/pkg/kakadu-7.2.2/apps/jp2 -I/usr/local/google/pkg/kakadu-7.2.2/apps/image -I/usr/local/google/pkg/kakadu-7.2.2/apps/args -I/usr/local/google/pkg/kakadu-7.2.2/apps/support -I/usr/local/google/pkg/kakadu-7.2.2/apps/kdu_compress -I. -DOGR_ENABLED -D_REENTRANT  -I/usr/local/google/home/warmerdam/gdal/port -I/usr/include -I/usr/local/google/bld/include  -c -o jp2kakdataset.o jp2kakdataset.cpp
warmerdam@gdal:~/gdal/frmts/jp2kak$ 

The line number quoted does not appear to be current trunk. Could someone encountering the problem quote their exact compile line and error with trunk?

Note that the pre_create line looks like this in trunk:

#if KAKADU_VERSION >= 700
        lines[c].pre_create(&allocator,nXSize,bReversible,bReversible,0,0);
#else
        lines[c].pre_create(&allocator,nXSize,bReversible,bReversible);
#endif

Near the top of the file version is determined like this:

// Generally Kakadu does not advertise its version well, so we look for a
// clue to V6 vs V7, the main difference in api.
#ifndef KAKADU_VERSION
#  ifdef KDU_TARGET_CAP_SEQUENTIAL
#    define KAKADU_VERSION 700
#  else
#    define KAKADU_VERSION 600
#  endif
#endif

comment:4 by bledoux, 11 years ago

Hello Frank,

Here is the complete compilation error we get with gdal 1.9.2 :

g++ -m32 -fPIC  -Wall  -I./gdal-1.9.2/port -I./gdal-1.9.2/gcore -I./60_gdal-1.9.2/gdal-1.9.2/alg -I./gdal-1.9.2/ogr -I./gdal-1.9.2/ogr/ogrsf_frmts -I./kakadu/coresys/common -I./kakadu/apps/compressed_io -I./kakadu/apps/jp2 -I./kakadu/apps/image -I.kakadu/apps/args -I./kakadu/apps/support -I.kakadu/apps/kdu_compress -I. -DOGR_ENABLED -m32 -I./gdal-1.9.2/port  -c -o ../o/jp2kakdataset.o jp2kakdataset.cpp [2013-06-24 12:44:22] jp2kakdataset.cpp: In function Ôint JP2KAKCreateCopy_WriteTile(GDALDataset*, kdu_tile&, kdu_roi_image*, int, int, int, int, bool, int, GDALDataType, kdu_codestream&, int, kdu_long*, int, GDALProgressFunc, void*, bool)Ô: [2013-06-24 12:44:22] jp2kakdataset.cpp:2016:70: error: no matching function for call to Ôkdu_line_buf::pre_create(kdu_sample_allocator*, int&, bool&, bool&)Ô [2013-06-24 12:44:22] jp2kakdataset.cpp:2016:70: note: candidate is: [2013-06-24 12:44:22] ./kakadu/coresys/common/kdu_sample_processing.h:430:10: note: void kdu_line_buf::pre_create(kdu_sample_allocator*, int, bool, bool, int, int) [2013-06-24 12:44:22] ./kakadu/coresys/common/kdu_sample_processing.h:430:10: note:   candidate expects 6 arguments, 4 provided

Best Regards,

Bruno

comment:5 by Even Rouault, 11 years ago

Bruno, Frank mentionned trunk and not GDAL 1.9.2. Actually I can see that the support for Kakadu 7 was done in r25950, which was even after GDAL 1.10 release, so this is expected.

comment:6 by Robert Coup, 10 years ago

Building against v7.1, KDU_TARGET_CAP_SEQUENTIAL doesn't appear to be defined in the Kakadu headers, but manually adding CXXFLAGS := $(CXXFLAGS) -DKAKADU_VERSION=710 to frmts/jp2kak/GNUMakefile works fine.

Will re-check with 7.2.2 in a few days.

comment:7 by Kurt Schwehr, 10 years ago

version 7.3 of kakadu also has a change to allocator.finalize():

source:trunk/gdal/frmts/jp2kak/jp2kakdataset.cpp#L1896

comment:8 by Kurt Schwehr, 10 years ago

r27264 handles the v7's allocator.finalize(oCodeStream). Tested with v7_3_3.

comment:9 by Kurt Schwehr, 10 years ago

If r27264 looks good, I'd like to also push it to the 1.11 tree.

comment:10 by Robert Coup, 10 years ago

@goatbear shouldn't it be #if KAKADU_VERSION >= 730? Not that there's a way of specifying it at compile time except via Makefile hacks. Maybe I should fix that :)

I might give lobbying Kakadu to at least add a version identifier a go... these API changes are getting kind of silly now, and it feels like we'll (easily) be breaking older versions without knowing if it keeps up.

comment:11 by Even Rouault, 10 years ago

r27264 breaks compilation with Kakadu 7.3 (.0?)

jp2kakdataset.cpp: In function ‘int JP2KAKCreateCopy_WriteTile(GDALDataset*, kdu_tile&, kdu_roi_image*, int, int, int, int, bool, int, GDALDataType, kdu_codestream&, int, kdu_long*, int, int (*)(double, const char*, void*), void*, bool)’:
jp2kakdataset.cpp:1900: error: no matching function for call to ‘kdu_sample_allocator::finalize(kdu_codestream&)’
/home/foo/kakadu/coresys/common/kdu_sample_processing.h:305: note: candidates are: void kdu_sample_allocator::finalize()

comment:12 by Kurt Schwehr, 10 years ago

Reverted in r27265. I only have kakadu v7_3_2, v7_3_3, and v6_4_1. We have upstreamed almost all of our local fixes of kakadu, but have not got them to do a matching version number.

I do see this in coresys/common/kdu_compressed.h, but it's not in the most useful format:

#define KDU_CORE_VERSION "v7.3.3"

comment:13 by Robert Coup, 10 years ago

fwiw, we first ran into the finalize()issue with a 7.3.2 build.

AFAIK splitting the KDU_CORE_VERSION string back into number(s) is Really Hard(TM) at compile time (I looked into it), would be magnitudes easier for them to add a KDU_MAJOR_VERSION, KDU_MINOR_VERSION KDU_BUILD_VERSION etc

Another option is for the gdal jp2kak Makefile to figure out the version by parsing that header file/string, then define KAKADU_VERSION from it?

comment:14 by Even Rouault, 10 years ago

One idea would indeed to have a KAKADU_VERSION macro. Unix configure could detect the version with by compiling tiny .cpp that would try to use a function (see how it is done for poppler that has the same issue). And for Windows, people would have to define it manually in nmake.local.

comment:15 by Kurt Schwehr, 10 years ago

Is there a way in the configure step to set KAKADU_VERSION in cpl_config.h? I can think of two approaches off hand:

  • use the preprocessor to get the KDU_CORE_VERSION from kdu_compress.h
  • use a small c or c++ program to get an/or write the version info

I've checked back to v5_1-00418C:

#define KDU_CORE_VERSION "v5.1"
#define KDU_CORE_VERSION "v6.4.1"
#define KDU_CORE_VERSION "v7.3.3"

Probably not the best ever code, but here is some quick c++ that could output what would need to be added to config.h. I'm not sure how to do this within autoconf and the I am not attached to the names of the defines. And... The KAKADU_VERSION will fail to work is the MINOR or MINOR_MINOR is ever more than a single digit.

#include <iostream>
#include <sstream>
#include <string>
#include <vector>

#include <kdu_compressed.h>

using namespace std;

int main(int argc, char **argv) {
  stringstream stream(string(KDU_CORE_VERSION));
  vector<string> parts;
  string part;
  while (getline(stream, part, '.')) {
    parts.push_back(part);
  }

  if (parts.size()==2) parts.push_back(string("0"));
  parts[0].erase(0,1);
  cout << "#define KDU_MAJOR_VERSION " << parts[0] << endl;
  cout << "#define KDU_MINOR_VERSION " << parts[1] << endl;
  cout << "#define KDU_MINOR_MINOR_VERSION " << parts[2] << endl;
  cout << "#define KAKADU_VERSION " << parts[0] << parts[1] << parts[2] << endl;

  return 0;
}

comment:16 by Kurt Schwehr, 10 years ago

Here is an example of using the preprocessor to get a version number:

http://git.savannah.gnu.org/cgit/autoconf-archive.git/tree/m4/ax_path_bdb.m4#n181

HEADER_VERSION=`eval "$ac_cpp conftest.$ac_ext" 2> /dev/null \
        | grep AX_PATH_BDB_STUFF | sed 's/[[^0-9,]]//g;s/,/./g;1q'`
    ],[])

And there is also ax_absolute_header.m4:

http://git.savannah.gnu.org/cgit/autoconf-archive.git/tree/m4/ax_absolute_header.m4

Basically, I was thinking something along the lines of

grep KDU_CORE_VERSION kdu_compressed.h | grep define | cut -dv -f2 | cut -d. -f1
grep KDU_CORE_VERSION kdu_compressed.h | grep define | cut -dv -f2 | cut -d. -f2
grep KDU_CORE_VERSION kdu_compressed.h | grep define | cut -dv -f2 | cut -d. -f3 | cut -d\" -f1

comment:17 by Robert Coup, 10 years ago

Progress upstream!

https://groups.yahoo.com/neo/groups/kakadu_jpeg2000/conversations/messages/6928

We will add such #defines, exactly as suggested, to all future releases.

#define KDU_CORE_VERSION="v7.3.3"  // existing
#define KDU_MAJOR_VERSION=7
#define KDU_MINOR_VERSION=3
#define KDU_PATCH_VERSION=3

comment:18 by Robert Coup, 10 years ago

Resolution: fixed
Status: assignedclosed

r27787 (trunk) starts using the new official Kakadu version headers available from v7.5

Update the JP2KAK and JPIPKAK drivers for compatibility with Kakadu v7.5.

This essentially is just the new kdu_core and kdu_supp C++ namespaces.

Consistently use the new KDU_{MAJOR,MINOR,PATCH}_VERSION defines instead of the compile-time KAKADU_VERSION define that we unofficially supported and tried badly to autodetect. For older Kakadu releases, require that the user specify the new-style defines at compile-time.

For older v7.x versions the autodetection was so badly broken, I think it's safer and better to go back to specifying it at compile time:

CXXFLAGS="KDU_MAJOR_VERSION=7 KDU_MINOR_VERSION=3 KDU_PATCH_VERSION=0" ./configure

If someone feels particularly strongly I guess we could add something to the configure script to specify a version and define those variables? ala ./configure --with-kakadu=/path/to/kakadu --kakadu-version=7.3.0

I've updated all the #ifdef logic to use the new defines as well.

Note: See TracTickets for help on using tickets.