Ticket #1723 (assigned defect)

Opened 1 year ago

Last modified 1 year ago

[PATCH] gdaladdo repeated on GTiff make image size increase

Reported by: rouault Assigned to: warmerdam (accepted)
Priority: normal Milestone:
Component: default Version: 1.4.2
Severity: normal Keywords: libtiff
Cc:

Description

Repeating the same gdaladdo command line (ex : 'gdaladdo -r average world_dted0.tiff 2') make the image size increase each time of 128x128xsizeof(pixel type).

I propose the following patch in libtiff to fix that (in TIFFAppendToStrip, in tiff_write.c): 

--- gdal-1.4.2.ori/frmts/gtiff/libtiff/tif_write.c      2007-07-28 13:59:18.000000000 +0200
+++ gdal-1.4.2/frmts/gtiff/libtiff/tif_write.c  2007-07-29 18:42:10.000000000 +0200
@@ -642,8 +642,8 @@
              * space, so such scheme is not too much effective.
              */
             if (td->td_stripbytecountsorted) {
-                if (strip == td->td_nstrips - 1
-                    || td->td_stripoffset[strip + 1] <
+                if (strip != td->td_nstrips - 1
+                    && td->td_stripoffset[strip + 1] <
                     td->td_stripoffset[strip] + cc) {
                     td->td_stripoffset[strip] =
                         TIFFSeekFile(tif, (toff_t)0,

Attachments

gdal-1.4.2-gdaladdo-tiff-keep-growing-fix.patch (0.7 kB) - added by rouault on 07/29/07 12:51:34.

Change History

07/29/07 12:51:34 changed by rouault

  • attachment gdal-1.4.2-gdaladdo-tiff-keep-growing-fix.patch added.

07/31/07 12:32:25 changed by warmerdam

  • status changed from new to assigned.

Hi,

Could you explain the rationale of the code change? This is a twitchy area of the libtiff code, and I've obviously botched it before.

Technically libtiff bug reports should go via the libtiff Bugzilla, but I can handle it this way for this problem.

07/31/07 12:33:29 changed by warmerdam

This may relate to #1688.

07/31/07 14:47:00 changed by rouault

It was the first time (and I hope the last) that I've put my nose in libtiff code... If I understand well, the point of the test is to be sure that one 'strip' doesn't overlap on the already following strips : "td->td_stripoffset[strip + 1] < td->td_stripoffset[strip] + cc". In my opinion (very humble on the subject), it doesn't make any sense to test this for the last strip : it can't overlap on anything following... That's probably the rational for the original test just before, and to make sure that td_stripoffset[strip + 1] is valid. My hypothese is that the logical test is inverted (my experience shows that it's a quite common programming error pattern). I mean 

(strip == td->td_nstrips - 1 || expression(td_stripoffset[strip + 1])

prevents from testing td_stripoffset[strip + 1] when strip == td->td_nstrips - 1. But 

(strip != td->td_nstrips - 1 && expression(td_stripoffset[strip+1])

too...

I don't know if there are regression tests in libtiff that could show if it introduces a regression.