[gdal-dev] json-c 0.10 update status and json-c copy removal proposed

[Even Rouault]

Hi Mateusz,

> Folks,
>
> I'm trying to update GDAL's copy of json-c library.
> The task is not trivial, due to the fact GDAL has applied
> some sort of improvements to json-c, and also due to
> significant changes to how json-c works, UTF-8 support, etc.

Just curious about the changes in UTF-8 support : does json-c do any transcoding
now ? I would expect that previous versions dealt with UTF-8 without doing
anything particular.

> Some of GDAL's fixes have been submitted and accepted to the upstream,
> but there are a few which hasn't, like custom function
> json_object_new_double_with_precision(), or use of CPL string functions.
>
> Personally, I don't really see benefits of using CPL functions in json-c.
> The json_object_new_double_with_precision could be moved out to GDAL
> source files.

Hum, I see I'm the one to blame for json_object_new_double_with_precision().
(This was done at a time where I wrongly assumed that there was no libjsonc
upstream activity). Unfortunately it won't be possible to implement that only in
a GDAL specific file, because it requires an additional field in a private
structure of libjson-c. The implementation as it is currently is a bit bad
looking, so I can understand it will be difficult to upstream. Perhaps a simpler
version that does not try to do clever truncation of trailing 0's could be
accepted. Something like that (uncompiled & untested, being only with an email
environmenent right now):

static int json_object_double_with_precision_to_json_string(struct json_object*
jso,
                                             struct printbuf *pb)
{
    char szFormat[16];
    sprintf(szFormat, "%%.%df", jso->_precision);
    return sprintbuf(pb, szFormat, jso->o.c_double);
}

struct json_object* json_object_new_double_with_precision(double d, int
nPrecision)
{
    struct json_object *jso = json_object_new(json_type_double);
    if(!jso) return NULL;
    jso->_to_json_string = &json_object_double_with_precision_to_json_string;
    jso->o.c_double = d;
    jso->_precision = (nPrecision < 32) ? nPrecision : 32;
    return jso;
}

or perhaps a version where instead of passing the precision, we would pass a
pointer to a callback and a user_data pointer, so that the formatting function
could be completely implemented on GDAL side ? something like :

typedef int (*json_double_custom_formatter_func_type)(char* szBuffer, size_t
nBufferLen, double d, void* user_data);

static int json_double_custom_formatter_trampoline(struct json_object* jso,
                                             struct printbuf *pb)
{
    char szBuffer[32];
    if( !json->pfnCustomFormatter(szBuffer, sizeof(szBuffer), jso->o.c_double,
jso->user_data) )
       return 0; /* not sure what value is error code */
    szBuffer[sizeof(szBuffer) - 1] = '\0';
    return sprintbuf(pb, "%s", szBuffer);
}

struct json_object* json_object_new_double_custom(double d,
json_double_custom_formatter_func_type pfnCustomFormatter, void* user_data)
{
    struct json_object *jso = json_object_new(json_type_double);
    if(!jso) return NULL;
    jso->_to_json_string = &json_double_custom_formatter_trampoline;
    jso->o.c_double = d;
    jso->pfnCustomFormatter = pfnCustomFormatter;
    jso->user_data = user_data;
    return jso;
}

Actually, after having written the above, I'm wondering if a version where we
would directly pass the custom string representation in
json_object_new_double_custom would not be even simpler !

static int json_object_double_custom_to_json_string(struct json_object* jso,
                                             struct printbuf *pb)
{
    return sprintbuf(pb, "%s", jso->pszFormattedDouble);
}

struct json_object* json_object_new_double_custom(double d, const char*
pszFormattedDouble)
{
    struct json_object *jso = json_object_new(json_type_double);
    if(!jso) return NULL;
    jso->_to_json_string = &json_object_double_custom_to_json_string;
    jso->o.c_double = d;
    jso->pszFormattedDouble = strdup(pszFormattedDouble); /* FIXME: to be freed
somewhere */
    return jso;
}

If it is not accepted, that's not critical. It was an enhancement (
http://trac.osgeo.org/gdal/ticket/4108 ), but we could
live without it and just revert to json_object_new_double().

> So, if we could compromise and give up on GDAL-specific json-c version,
> I'd like to propose to completely remove json-c sources from GDAL
> and rely on externally provided json-c (libjsonc)
> Especially, that building json-c is versy simple...if its native build
> configuration is used.

It would be good if you could coordinate with Tamas and OSGeo4W to make sure
that they have a libjson binary, so that the most popular builds for Windows
don't loose geojson support.

>
> Maintaining private and *changed* copy of json-c has become painful.
> json-c has grown and its configuration/building is based on two
> non-trivial config headers
> with plenty of #defines. Previously, we simply crafted our own version
> of static config.h
> that worked for all systems. It is not really feasible with new version.
> Also, hosting our own modified copy of json-c may lead to more GDAL-specific
> modifications increasing the pain of sync'ing it with upstream and
> maintenance in future.
>
> I don't see any reason why GDAL couldn't rely on external json-c.
> So, I'd like to propose to:
> - remove GDAL's copy of json-c from ogr/ogrsf_frmts/geojson/jsonc
> - configure Unix and Windows builds to use external binaries

I assume you will deal with the libjsonc dependency as an optional one ? I'm not
sure if you're aware of it, but in addition to the OGR GeoJSON et CouchDB
drivers, there's also the new GDAL ARG driver that depends on libjsonc. The
corresponding autotests of the 3 drivers should likely be modified to skip if
the drivers aren't compiled in.

For the ease of using on Linux, it would be good if libjson-c 0.9 could still be
used, being the version currently packaged.

>
> I'm volunteering to apply those changes.
> Please, shall I make RFC, call for votes or this e-mail is sufficient to
> discuss and make decision?

I believe this email is sufficient.

Best regards,

Even