Opened 15 years ago

Closed 5 years ago

#2891 closed defect (wontfix)

RS2 driver: some questions after code review ?

Reported by: Even Rouault Owned by: pvachon
Priority: normal Milestone: closed_because_of_github_migration
Component: default Version: unspecified
Severity: normal Keywords:
Cc: antonio

Description

Phil,

here are a few observations on RS2CalibRasterBand::IReadBlock() while reviewing RS2 driver :

  • around line 431, we have 2 GDALSwapWords done on pImage buffer, but we have just read above in the pnImageTmp buffer. This looks like a bug.
  • I think that the pnImageTmp buffer is unnecessary. We could read directly in the pImage buffer and modify it afterwards.
  • The expression m_nfTable[nBlockXOff + j] looks suspicious because we add block offset and pixel offset. But I suspect that generally the underlying GeoTIFF is scanline oriented, so nBlockXOff would be always 0. However the code doesn't seem to be ready to accept a tiled GeoTIFF here contrary to RS2RasterBand::IReadBlock()
  • We don't check that the m_nfTable is large enough. (if the lut.xml was corrupted for example, we could read out of bounds)

Change History (3)

comment:1 by pvachon, 15 years ago

  1. Definitely a bug. Now, a better question is "is that swap necessary on a little endian machine;" I suspect the products are shipped in little endian byte order; naturally, the documentation from MDA doesn't cover this, go figure. However, at a glance, the results I get from polarimetric processing are correct, so I guess it's one of those harmless bugs. ;)
  1. No, I don't think this can't be trivially eliminated. The data being read from the disk is a pair of 16-bit signed ints/pixel, and the data being output to the client is actually a pair of 32-bit floats/pixel, once calibration has been applied.
  1. I think that's a bit of a red herring, since it should always be read as range lines, at least from what I've seen from real RS2 data sets. I suppose when I wrote the code I wanted to anticipate the case where a tiled TIFF image would be provided, since MDA was threatening to do so at some point (if they haven't already). I also somehow think that the code came as a part of a patch, but I might be mistaken.
  1. I would change ReadLUT to throw an exception if the LUT size is less than the width of the raster. If the LUT is corrupted, you can't calibrate the data anyways, so you're pretty much sewered at that point.

Cheers, P.

comment:2 by antonio, 13 years ago

Cc: antonio added

comment:3 by Even Rouault, 5 years ago

Milestone: closed_because_of_github_migration
Resolution: wontfix
Status: newclosed

This ticket has been automatically closed because Trac is no longer used for GDAL bug tracking, since the project has migrated to GitHub. If you believe this ticket is still valid, you may file it to https://github.com/OSGeo/gdal/issues if it is not already reported there.

Note: See TracTickets for help on using tickets.