simplify #317

openeo-geotrellis/src/main/scala/org/openeo/geotrellis/geotiff/GTiffOptions.scala

@@ -55,7 +55,8 @@ class GTiffOptions extends Serializable {
     xmlTags append asItemElement(tagName, value)
   }
 
-  def addBandTag(bandIndex: Int, tagName:String, value:String, role: Option[String]): Unit = {
+  // TODO: drop method with role
+  private def addBandTag(bandIndex: Int, tagName:String, value:String, role: Option[String]): Unit = {
     val emptyMap = Map.empty[String, String]
     var newBandTags = Vector.fill[Map[String,String]](math.max(bandIndex+1,tags.bandTags.size))(emptyMap)
     newBandTags =  newBandTags.zipAll(tags.bandTags,emptyMap,emptyMap).map(elem => elem._1 ++ elem._2)
@@ -65,6 +66,9 @@ class GTiffOptions extends Serializable {
     xmlTags append asItemElement(tagName, value, Some(bandIndex), role)
   }
 
+  def addBandTag(bandIndex: Int, tagName:String, value:String, role: String): Unit =
+    addBandTag(bandIndex, tagName, value, Some(role))
+
   def addBandTag(bandIndex: Int, tagName: String, value: String): Unit =
     addBandTag(bandIndex, tagName, value, role = None)
 
@@ -79,12 +83,27 @@ class GTiffOptions extends Serializable {
     } addBandTag(bandIndex, tagName, value)
   }
 
-  def toGdalMetadataXml: String = {
-    val buffer = new StringBuilder("<GDALMetadata>")
-    for (xmlTag <- xmlTags) buffer.appendAll(xmlTag)
-    buffer.appendAll("</GDALMetadata>")
-
-    buffer.toString
+  // TODO: rename
+  def toGdalMetadataXml: xml.Elem = {
+    val headTags = for {
+      (key, value) <- tags.headTags
+    }  yield <Item name={key}>{value}</Item>
+
+    val bandTags = for {
+      (tags, i) <- tags.bandTags.zipWithIndex
+      (key, value) <- tags
+    } yield {
+      if (Seq("description", "scale", "offset") contains key.toLowerCase) {
+        // TODO: DRY
+        <Item name={key} sample={i.toString} role={key.toLowerCase}>{value}</Item>
+      } else
+        <Item name={key} sample={i.toString}>{value}</Item>
+    }
+
+    <GDALMetadata>
+      {headTags}
+      {bandTags}
+    </GDALMetadata>
   }
 
   /**

openeo-geotrellis/src/main/scala/org/openeo/geotrellis/geotiff/package.scala

@@ -32,6 +32,7 @@ import software.amazon.awssdk.services.s3.model.PutObjectRequest
 import spire.math.Integral
 import spire.syntax.cfor.cfor
 
+import java.io.IOException
 import java.nio.channels.FileChannel
 import java.nio.file.{FileAlreadyExistsException, Files, NoSuchFileException, Path, Paths}
 import java.time.Duration
@@ -584,7 +585,7 @@ package object geotiff {
 
     val thegeotiff = new MultibandGeoTiff(tiffTile, croppedExtent, crs,formatOptions.tags,options,overviews = overviews.map(o=>MultibandGeoTiff(o,croppedExtent,crs,options = options.copy(subfileType = Some(ReducedImage)))))//.withOverviews(NearestNeighbor, List(4, 8, 16))
 
-    writeGeoTiff(thegeotiff, path)
+    writeGeoTiff(thegeotiff, path, Some(formatOptions))
   }
 
   private def toTiff(tiffs:collection.Map[Int, Array[Byte]] , gridBounds: GridBounds[Int], tileLayout: TileLayout, compression: DeflateCompression, cellType: CellType, detectedBandCount: Double, segmentCount: Int) = {
@@ -653,7 +654,7 @@ package object geotiff {
     val geoTiff = MultibandGeoTiff(adjusted, contextRDD.metadata.crs, GeoTiffOptions(compression))
       .withOverviews(NearestNeighbor, List(4, 8, 16))
 
-    writeGeoTiff(geoTiff, path)
+    writeGeoTiff(geoTiff, path, gtiffOptions = None)
     adjusted.extent
   }
 
@@ -742,7 +743,7 @@ package object geotiff {
     ) {
       geotiff = geotiff.withOverviews(NearestNeighbor, List(4, 8, 16))
     }
-    writeGeoTiff(geotiff, filePath)
+    writeGeoTiff(geotiff, filePath, Some(fo))
   }
 
   def saveSamples(rdd: MultibandTileLayerRDD[SpaceTimeKey],
@@ -825,20 +826,23 @@ package object geotiff {
       .toList.asJava
   }
 
-  def writeGeoTiff(geoTiff: MultibandGeoTiff, path: String): String = {
+  def writeGeoTiff(geoTiff: MultibandGeoTiff, path: String, gtiffOptions: Option[GTiffOptions]): String = {
     import java.nio.file.Files
+    // TODO: DRY
     if (path.startsWith("s3:/")) {
       val correctS3Path = path.replaceFirst("s3:/(?!/)", "s3://")
 
 
       val tempFile = Files.createTempFile(null, null)
       geoTiff.write(tempFile.toString, optimizedOrder = true)
+      gtiffOptions.foreach(options => embedGdalMetadata(options.toGdalMetadataXml, tempFile))
       uploadToS3(tempFile, correctS3Path)
 
     } else {
       val tempFile = getTempFile(null, ".tif")
       // TODO: Try to run fsync on the file opened by GeoTrellis (without the temporary copy)
       geoTiff.write(tempFile.toString, optimizedOrder = true)
+      gtiffOptions.foreach(options => embedGdalMetadata(options.toGdalMetadataXml, tempFile))
 
       // TODO: Write to unique path instead to avoid collisions between executors. Let the driver choose the paths.
       moveOverwriteWithRetries(tempFile, Path.of(path))
@@ -923,23 +927,24 @@ package object geotiff {
     print("test done")
   }
 
-  // TODO: favor dedicated XML type over String?
-  def embedGdalMetadata(gdalMetadataXml: String, geotiffPath: Path): Unit = {
+  // TODO: swap arguments
+  def embedGdalMetadata(gdalMetadata: xml.Elem, geotiffPath: Path): Unit = {
     import scala.sys.process._
     import java.nio.charset._
 
     val outputBuffer = new StringBuilder
     val processLogger = ProcessLogger(line => outputBuffer appendAll line)
 
+    // use temporary file to work around segfault in container
     val tempFile = Files.createTempFile("GDALMetadata_", ".xml.tmp")
     try {
-      Files.write(tempFile, s"$gdalMetadataXml\n".getBytes(StandardCharsets.US_ASCII))
+      Files.write(tempFile, s"$gdalMetadata\n".getBytes(StandardCharsets.US_ASCII)) // the newline is important
 
       val args = Seq("tiffset", "-sf", "42112", tempFile.toString, geotiffPath.toString)
       val exitCode = args ! processLogger
-      if (exitCode != 0) {
-        throw new Exception(s"${args mkString " "} failed; output: $outputBuffer")
-      }
+
+      if (exitCode == 0) logger.debug(s"wrote $gdalMetadata to $geotiffPath")
+      else throw new IOException(s"${args mkString " "} failed; output was: $outputBuffer")
     } finally Files.delete(tempFile)
   }
 }

openeo-geotrellis/src/test/scala/org/openeo/geotrellis/PackageTest.scala

@@ -28,7 +28,7 @@ class PackageTest {
       val dst = Path.of(p + f"/$i.tif")
       // Limit the amount of parallel jobs to avoid getting over the max retries
       (1 to 4).par.foreach { _ =>
-        writeGeoTiff(refTiff, dst.toString)
+        writeGeoTiff(refTiff, dst.toString, gtiffOptions = None)
       }
       assertTrue(Files.exists(dst))
       val refTiff2 = GeoTiff.readMultiband(dst.toString)

openeo-geotrellis/src/test/scala/org/openeo/geotrellis/geotiff/GTiffOptionsTest.scala

@@ -3,7 +3,6 @@ package org.openeo.geotrellis.geotiff
 import org.junit.Assert.assertEquals
 import org.junit.Test
 
-import scala.xml.XML
 import scala.xml.Utility.trim
 
 class GTiffOptionsTest {
@@ -16,7 +15,7 @@ class GTiffOptionsTest {
     val bandNames = Seq("VV", "VH", "mask", "local_incidence_angle")
 
     for ((bandName, index) <- bandNames.zipWithIndex) {
-      options.addBandTag(index, "DESCRIPTION", bandName, role = Some("description"))
+      options.addBandTag(index, "DESCRIPTION", bandName, role = "description")
     }
 
     assertEquals(Map("PROCESSING_SOFTWARE" -> "0.6.1a1"), options.tags.headTags)
@@ -36,6 +35,6 @@ class GTiffOptionsTest {
         <Item name="DESCRIPTION" sample="3" role="description">local_incidence_angle</Item>
       </GDALMetadata>
 
-    assertEquals(trim(expectedGdalMetadataXml), trim(XML.loadString(options.toGdalMetadataXml)))
+    assertEquals(trim(expectedGdalMetadataXml), trim(options.toGdalMetadataXml))
   }
 }

openeo-geotrellis/src/test/scala/org/openeo/geotrellis/geotiff/PackageTest.scala

@@ -5,6 +5,7 @@ import org.junit.jupiter.api.Assertions.{assertEquals, assertThrows, assertTrue}
 import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.io.TempDir
 
+import java.io.IOException
 import java.nio.file.{Files, Path, Paths}
 
 class PackageTest {
@@ -22,15 +23,15 @@ class PackageTest {
         <Item name="DESCRIPTION" sample="0">CO</Item>
       </GDALMetadata>
 
-    embedGdalMetadata(gdalMetadataXml.toString(), geotiffCopy)
+    embedGdalMetadata(gdalMetadataXml, geotiffCopy)
 
     assertEquals(Some("0.45.0a1"), processingSoftware(geotiffCopy))
   }
 
   @Test
   def testEmbedGdalMetadataFails(): Unit = {
-    val e = assertThrows(classOf[Exception], () =>
-      embedGdalMetadata(gdalMetadataXml = "", geotiffPath = Paths.get("doesnotexist.tif")))
+    val e = assertThrows(classOf[IOException], () =>
+      embedGdalMetadata(<GdalMetadata />, geotiffPath = Paths.get("doesnotexist.tif")))
 
     assertTrue(e.getMessage contains "doesnotexist.tif: No such file or directory")
   }