Opened 6 years ago

Closed 6 years ago

#2512 closed defect (fixed)

r.out.gdal wrongly messes with SetColorInterpretation

Reported by: rouault Owned by: grass-dev@…
Priority: normal Milestone: 6.4.5
Component: Raster Version: svn-trunk
Keywords: r.out.gdal Cc: sprice
CPU: All Platform: All

Description

r.out.gdal export_band.c currently contains

    CPLPushErrorHandler(CPLQuietErrorHandler);
    GDALSetRasterColorInterpretation(hBand, GPI_RGB);
    CPLPopErrorHandler();

This is a wrong usage of the GDAL API. GDALSetRasterColorInterpretation() expects an enumerated value from the GDALColorInterp enumeration, whereas GPI_RGB is a value from the GDALPaletteInterp enumeration. Consequently, due to the fact that GPI_RGB = 1 and GCI_GrayIndex=1, the above snippet will effectively force the color interpreation to be Gray level.

On GeoTIFF this was without effect before GDAL 1.11, since in those versions, SetColorInterpration?() in the GeoTIFF driver has no effect. But in GDAL 1.11, the SetColorInterpration?() in the GeoTIFF driver can modify the default PHOTOMETRIC TIFF tag. For example, for a 3 band GeoTIFF file, at creation time, the GeoTIFF driver defaults to PHOTOMETRIC=RGB, but if later, SetRasterColorInterpretation?(GCI_GrayIndex) is called, it will undo that setting to fallback to PHOTOMETRIC=MINISBLACK.

All in all, I suggest just to remove those 3 lines. The CPLPushErrorHandler(CPLQuietErrorHandler) / CPLPopErrorHandler() surrounding is a hint that this call is wrong.

Attachments (1)

ticket_2512.patch (606 bytes) - added by rouault 6 years ago.

Download all attachments as: .zip

Change History (6)

Changed 6 years ago by rouault

Attachment: ticket_2512.patch added

comment:1 Changed 6 years ago by sprice

Cc: sprice added

comment:2 in reply to:  description Changed 6 years ago by mmetz

Replying to rouault:

r.out.gdal export_band.c currently contains

    CPLPushErrorHandler(CPLQuietErrorHandler);
    GDALSetRasterColorInterpretation(hBand, GPI_RGB);
    CPLPopErrorHandler();

This is a wrong usage of the GDAL API. GDALSetRasterColorInterpretation() expects an enumerated value from the GDALColorInterp enumeration, whereas GPI_RGB is a value from the GDALPaletteInterp enumeration. Consequently, due to the fact that GPI_RGB = 1 and GCI_GrayIndex=1, the above snippet will effectively force the color interpreation to be Gray level.

On GeoTIFF this was without effect before GDAL 1.11, since in those versions, SetColorInterpration?() in the GeoTIFF driver has no effect. But in GDAL 1.11, the SetColorInterpration?() in the GeoTIFF driver can modify the default PHOTOMETRIC TIFF tag. For example, for a 3 band GeoTIFF file, at creation time, the GeoTIFF driver defaults to PHOTOMETRIC=RGB, but if later, SetRasterColorInterpretation?(GCI_GrayIndex) is called, it will undo that setting to fallback to PHOTOMETRIC=MINISBLACK.

All in all, I suggest just to remove those 3 lines. The CPLPushErrorHandler(CPLQuietErrorHandler) / CPLPopErrorHandler() surrounding is a hint that this call is wrong.

Done in r63682,3 (trunk, relbr70).

comment:3 Changed 6 years ago by neteler

Done in 6.4.svn (relbranch64) in r63688.

comment:4 Changed 6 years ago by rouault

Thanks Markus !

comment:5 Changed 6 years ago by neteler

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.