# HG changeset patch
# User magedrifaat <magedrifaat@gmail.com>
# Date 1660333843 -7200
# Node ID 27ed758c1688b8b3e39439506a5908d877fe5527
# Parent  f91cd5ceaae6ab560a7afa8c2bc6ae6f6e41d2ac
Tiff setTag: fixed bug for rational tags and special-case tags.

diff -r f91cd5ceaae6 -r 27ed758c1688 libinterp/corefcn/__tiff__.cc
--- a/libinterp/corefcn/__tiff__.cc	Fri Aug 12 14:36:48 2022 +0200
+++ b/libinterp/corefcn/__tiff__.cc	Fri Aug 12 21:50:43 2022 +0200
@@ -739,7 +739,7 @@
         }
       case TIFF_RATIONAL:
         {
-          error ("TIFF_RATIONAL should have at least 2 elements but got only 1");
+          retval = *(reinterpret_cast<float *> (data));
           break;
         }
       case TIFF_SBYTE:
@@ -774,7 +774,7 @@
         }
       case TIFF_SRATIONAL:
         {
-          error ("TIFF_SRATIONAL should have at least 2 elements but got only 1");
+          retval = *(reinterpret_cast<float *> (data));
           break;
         }
       case TIFF_IFD:
@@ -854,13 +854,10 @@
             }
           case TIFF_RATIONAL:
             {
-              // TODO(maged): This is incorrect, LibTIFF already handles
-              // rationals and converts them to floats
               NDArray arr (arr_dims);
-              for (uint32_t i = 0; i < count; i+=2)
+              for (uint32_t i = 0; i < count; i++)
                 {
-                  arr(i / 2) = static_cast<float> ((reinterpret_cast<uint32_t *> (data))[i])
-                               / static_cast<float> ((reinterpret_cast<uint32_t *> (data))[i+1]);
+                  arr(i) = (reinterpret_cast<float *> (data))[i];
                 }
               retval = octave_value (arr);
               break;
@@ -928,10 +925,9 @@
           case TIFF_SRATIONAL:
             {
               NDArray arr (arr_dims);
-              for (uint32_t i = 0; i < count; i+=2)
+              for (uint32_t i = 0; i < count; i++)
                 {
-                  arr(i / 2) = static_cast<float> ((reinterpret_cast<int32_t *> (data))[i])
-                               / static_cast<float> ((reinterpret_cast<int32_t *> (data))[i+1]);
+                  arr(i) = (reinterpret_cast<float *> (data))[i];
                 }
               retval = octave_value (arr);
               break;
@@ -1082,17 +1078,13 @@
 
           uint32_t count = 1 << bits_per_sample;
           uint16_t *ch1, *ch2, *ch3;
-          if (samples_per_pixel == 1)
-            {
-              validate_tiff_get_field (TIFFGetField (tif, TIFFTAG_COLORMAP, &ch1));
-              tag_data_ov = interpret_tag_data (ch1, count,
-                                                TIFFFieldDataType (fip));
-            }
+          validate_tiff_get_field (TIFFGetField (tif, tag_id,
+                                                 &ch1, &ch2, &ch3));
+          if (ch2 == NULL)
+            tag_data_ov = interpret_tag_data (ch1, count,
+                                              TIFFFieldDataType (fip));
           else
-            {
-              validate_tiff_get_field (TIFFGetField (tif, TIFFTAG_COLORMAP,
-                                                     &ch1, &ch2, &ch3));
-              
+            { 
               OCTAVE_LOCAL_BUFFER (uint16NDArray, array_list, 3);
               dim_vector col_dims(count, 1);
               array_list[0] = interpret_tag_data (ch1,
@@ -1189,12 +1181,15 @@
     if (TIFFFieldPassCount (fip))
       error ("Unsupported tag");
     
+    // According to matlab, the value must be a scalar double
+    if (! tag_ov.is_scalar_type () || ! tag_ov.is_double_type ())
+      error ("Tag value must be a scalar double");
+    
     TIFFDataType tag_datatype = TIFFFieldDataType (fip);
     switch (tag_datatype)
       {
       case TIFF_BYTE:
       case TIFF_UNDEFINED:
-        // TODO(maged): check if matlab errors for long data type/range
         TIFFSetField (tif, tag_id, tag_ov.uint8_scalar_value ());
         break;
       case TIFF_ASCII:
@@ -1242,6 +1237,16 @@
       }
   }
 
+  template <typename T>
+  void
+  set_array_field_helper (TIFF *tif, uint32_t tag_id, T data_arr)
+  {
+    const void *data_ptr = data_arr.data ();
+
+    if (! TIFFSetField (tif, tag_id, data_ptr))
+      error ("Failed to set tag");
+  }
+
   void
   set_array_field_data (TIFF *tif, const TIFFField *fip,
                         octave_value tag_ov, uint32_t count)
@@ -1249,51 +1254,64 @@
     uint32_t tag_id = TIFFFieldTag (fip);
     TIFFDataType tag_datatype = TIFFFieldDataType (fip);
 
-    // TODO(maged): validate count?
-    octave_unused_parameter (count);
+    // Array type must be double in matlab
+    if (! tag_ov.is_double_type ())
+      error ("Tag data must be double");
 
-    void *data_ptr;
+    // Matlab checks number of elements not dimensions
+    if (tag_ov.array_value ().numel () != count)
+      error ("Expected an array with %u elements", count);
 
     switch (tag_datatype)
       {
       case TIFF_BYTE:
       case TIFF_UNDEFINED:
-        // TODO(maged): check if matlab errors for long data type/dimensions
-        // TODO(maged): should we resize/reshape?
-        data_ptr = tag_ov.uint8_array_value ().fortran_vec ();
+        set_array_field_helper<uint8NDArray> (tif, tag_id,
+                                              tag_ov.uint8_array_value ());
         break;
       case TIFF_SHORT:
-        data_ptr = tag_ov.uint16_array_value ().fortran_vec ();
+        set_array_field_helper<uint16NDArray> (tif, tag_id,
+                                               tag_ov.uint16_array_value ());
         break;
       case TIFF_LONG:
-        data_ptr = tag_ov.uint32_array_value ().fortran_vec ();
+        set_array_field_helper<uint32NDArray> (tif, tag_id,
+                                               tag_ov.uint32_array_value ());
         break;
       case TIFF_LONG8:
-        data_ptr = tag_ov.uint64_array_value ().fortran_vec ();
+        set_array_field_helper<uint64NDArray> (tif, tag_id,
+                                               tag_ov.uint64_array_value ());
         break;
       case TIFF_RATIONAL:
-        data_ptr = tag_ov.float_array_value ().fortran_vec ();
+        set_array_field_helper<FloatNDArray> (tif, tag_id,
+                                               tag_ov.float_array_value ());
         break;
       case TIFF_SBYTE:
-        data_ptr = tag_ov.int8_array_value ().fortran_vec ();
+        set_array_field_helper<int8NDArray> (tif, tag_id,
+                                             tag_ov.int8_array_value ());
         break;
       case TIFF_SSHORT:
-        data_ptr = tag_ov.int16_array_value ().fortran_vec ();
+        set_array_field_helper<int16NDArray> (tif, tag_id,
+                                              tag_ov.int16_array_value ());
         break;
       case TIFF_SLONG:
-        data_ptr = tag_ov.int32_array_value ().fortran_vec ();
+        set_array_field_helper<int32NDArray> (tif, tag_id,
+                                              tag_ov.int32_array_value ());
         break;
       case TIFF_SLONG8:
-        data_ptr = tag_ov.int64_array_value ().fortran_vec ();
+        set_array_field_helper<int64NDArray> (tif, tag_id,
+                                              tag_ov.int64_array_value ());
         break;
       case TIFF_FLOAT:
-        data_ptr = tag_ov.float_array_value ().fortran_vec ();
+        set_array_field_helper<FloatNDArray> (tif, tag_id,
+                                              tag_ov.float_array_value ());
         break;
       case TIFF_DOUBLE:
-        data_ptr = tag_ov.array_value ().fortran_vec ();
+        set_array_field_helper<NDArray> (tif, tag_id,
+                                         tag_ov.array_value ());
         break;
       case TIFF_SRATIONAL:
-        data_ptr = tag_ov.float_array_value ().fortran_vec ();
+        set_array_field_helper<FloatNDArray> (tif, tag_id,
+                                              tag_ov.float_array_value ());
         break;
       case TIFF_IFD:
       case TIFF_IFD8:
@@ -1302,16 +1320,13 @@
       default:
         error ("Unsupported tag data type");
       }
-
-    if (! TIFFSetField (tif, tag_id, data_ptr))
-      error ("Failed to set tag");
   }
 
   void
   set_field_data (TIFF *tif, const TIFFField *fip, octave_value tag_ov)
   {
     uint32_t tag_id = TIFFFieldTag (fip);
-
+    
     // TODO(maged): find/create images to test the special tags
     switch (tag_id)
       {
@@ -1338,22 +1353,28 @@
                                        &bits_per_sample))
             error ("Failed to obtain the bit depth");
           
+          if (! tag_ov.is_double_type ())
+            error ("Tag data must be double");
+          
           // According to the format specification, this field should
           // be 8 or 16 only.
           if (bits_per_sample > 16)
             error ("Too high bit depth for a palette image");
 
-          // TODO(maged): what is matlab behavior for wrong input dimensions
           uint32_t count = 1 << bits_per_sample;
+
+          if (tag_ov.is_scalar_type ()
+              || tag_ov.array_value ().dim1 () != count
+              || tag_ov.array_value ().dim2 () != 3)
+            error ("Expected matrix with %u rows and 3 columns", count);
+
           NDArray array_data = tag_ov.array_value ();
           array_data *= UINT16_MAX;
           uint16NDArray u16_array = uint16NDArray (array_data);
-          uint16_t *data_ptr
-            = reinterpret_cast<uint16_t *> (u16_array.fortran_vec ());
-          uint16_t *red = data_ptr;
-          uint16_t *green = red + count;
-          uint16_t *blue = green + count;
-          if (! TIFFSetField (tif, tag_id, red, green, blue))
+          const uint16_t *data_ptr
+            = reinterpret_cast<const uint16_t *> (u16_array.data ());
+          if (! TIFFSetField (tif, tag_id, data_ptr, data_ptr + count,
+                              data_ptr + 2 * count))
             error ("Failed to set tag");
           break;
         }
@@ -1371,9 +1392,16 @@
 
           uint32_t count = 1 << bits_per_sample;
           
-          uint16_t *data_ptr
-            = reinterpret_cast<uint16_t *> (tag_ov.uint16_array_value ()
-                                            .fortran_vec ());
+          // An intermediate variable is required for storing the return of
+          // uint16_array_value so that the object remains in scope and the
+          // pointer returned from data () is not a dangling pointer
+          uint16NDArray data_arr = tag_ov.uint16_array_value ();
+
+          if (data_arr.numel () != count)
+            error ("Tag data should be and array of %u elements", count);
+
+          const uint16_t *data_ptr
+            = reinterpret_cast<const uint16_t *> (data_arr.data ());
           if (samples_per_pixel == 1)
             {
               if (! TIFFSetField (tif, tag_id, data_ptr))
@@ -1392,25 +1420,35 @@
       case TIFFTAG_DOTRANGE:
       case TIFFTAG_YCBCRSUBSAMPLING:
         {
-          // TODO(maged): what is matlab behavior for wrong dimensions
           uint16NDArray array_data = tag_ov.uint16_array_value ();
+          if (array_data.numel () != 2)
+            error ("Tag data should be an array with 2 elements");
+          
           if (! TIFFSetField (tif, tag_id, array_data (0), array_data (1)))
             error ("Failed to set field");
           break;
         }
       case TIFFTAG_SUBIFD:
         {
-          // TODO(maged): check if matlab supports setting this tag
+          // TODO(maged): Matlab doesnt error on setting this tag
+          // but returns 0 for getTag
           error ("Unsupported tag");
           break;
         }
       case TIFFTAG_EXTRASAMPLES:
         {
-          // TODO(maged): check if matlab validates dimensions/values of this tag
+          uint16_t samples_per_pixel;
+          if (! TIFFGetFieldDefaulted (tif, TIFFTAG_SAMPLESPERPIXEL,
+                                       &samples_per_pixel))
+            error ("Failed to obtain the number of samples per pixel");
+          
           uint16NDArray data_array = tag_ov.uint16_array_value ();
+          if (data_array.numel () > samples_per_pixel - 3)
+            error ("Failed to set field, too many values");
+          
           if (! TIFFSetField (tif, tag_id,
-                              static_cast<uint16_t> (data_array.dim1 ()),
-                              data_array.fortran_vec ()))
+                              static_cast<uint16_t> (data_array.numel ()),
+                              data_array.data ()))
             error ("Failed to set field");
           break;
         }
@@ -1502,7 +1540,8 @@
     perm(2) = 0;
     strip_data = strip_data.permute (perm);
 
-    void *data_vec = strip_data.fortran_vec ();
+    uint8_t *data_u8
+      = reinterpret_cast<uint8_t *> (strip_data.fortran_vec ());
     if (image_data->bits_per_sample == 8
         || image_data->bits_per_sample == 16
         || image_data->bits_per_sample == 32
@@ -1512,7 +1551,7 @@
         // can be smaller in size
         tsize_t strip_size = strip_data.numel ()
                              * image_data->bits_per_sample / 8;
-        if (TIFFWriteEncodedStrip (tif, strip_no, data_vec, strip_size) == -1)
+        if (TIFFWriteEncodedStrip (tif, strip_no, data_u8, strip_size) == -1)
           error ("Failed to write strip data to image");
         
       }
@@ -1526,7 +1565,6 @@
         std::unique_ptr<uint8_t []> strip_ptr
           = std::make_unique<uint8_t []> (TIFFStripSize (tif));
         uint8_t *strip_buf = strip_ptr.get ();
-        uint8_t *data_u8 = reinterpret_cast<uint8_t *> (data_vec);
         // According to the format specification, the row should be byte
         // aligned so the number of bytes is rounded up to the nearest byte
         uint32_t padded_width = (image_data->width + 7) / 8;
@@ -1604,13 +1642,13 @@
     
     // Octave indexing is 1-based while LibTIFF is zero-based
     tile_no--;
-    void *data_vec = tile_data.fortran_vec ();
+    uint8_t *data_u8 = reinterpret_cast<uint8_t *> (tile_data.fortran_vec ());
     if (image_data->bits_per_sample == 8
         || image_data->bits_per_sample == 16
         || image_data->bits_per_sample == 32
         || image_data->bits_per_sample == 64)
       {
-        if (TIFFWriteEncodedTile (tif, tile_no, data_vec,
+        if (TIFFWriteEncodedTile (tif, tile_no, data_u8,
                                   TIFFTileSize (tif)) == -1)
           error ("Failed to write tile data to image");
         
@@ -1625,7 +1663,6 @@
         std::unique_ptr<uint8_t []> tile_ptr
           = std::make_unique<uint8_t []> (TIFFTileSize (tif));
         uint8_t *tile_buf = tile_ptr.get ();
-        uint8_t *data_u8 = reinterpret_cast<uint8_t *> (data_vec);
         // Packing the pixel data into bits
         for (uint32_t row = 0; row < tile_height; row++)
           {
@@ -1782,7 +1819,8 @@
     if (row_per_strip > image_data->height)
       row_per_strip = image_data->height;
 
-    uint8_t *pixel_fvec = reinterpret_cast<uint8_t *> (pixel_data.fortran_vec ());
+    uint8_t *pixel_fvec
+      = reinterpret_cast<uint8_t *> (pixel_data.fortran_vec ());
     uint32_t strip_count = TIFFNumberOfStrips (tif);
     tsize_t strip_size;
     uint32_t rows_in_strip;
@@ -1957,10 +1995,17 @@
   void
   write_image (TIFF *tif, T pixel_data, tiff_image_data *image_data)
   {
-    // TODO(maged): matlab sets the remaining to zero for less width and height
-    // and issues a warning for larger widths but not for larger height? (we should error)
-    // and doesnt issue a warning for larger width for tiled images and produces zeros
-    // and errors for less or more number of channels
+    // This dimensions checking is intentially done in this non-homogeneous
+    // way for matlab compatibility.
+    // In Matlab R2022a, If the width or height of the input matrix is less
+    // than needed, the data is silently padded with zeroes to fit.
+    // If the width is larger than needed, a warning is issued and the excess
+    // data is truncated. If the height is larger than needed, no warning is
+    // issued and the image data is wrong. If the number of channels is less
+    // or more than needed an error is produced.
+    // We chose to deviate from matlab in the larger height case to avoid
+    // errors resulting from silently corrupting image data, a warning is
+    // produced instead.
     if ((image_data->samples_per_pixel > 1 && pixel_data.ndims () < 3)
         || (pixel_data.ndims () > 2
             && image_data->samples_per_pixel != pixel_data.dim3 ()))