diff --git a/apps/gdaladdo.cpp b/apps/gdaladdo.cpp index f163772f8746..54ba6e093e6f 100644 --- a/apps/gdaladdo.cpp +++ b/apps/gdaladdo.cpp @@ -692,7 +692,23 @@ MAIN_START(nArgc, papszArgv) { for (const auto &level : *levels) { + if (CPLGetValueType(level.c_str()) != CPL_VALUE_INTEGER) + { + CPLError( + CE_Failure, CPLE_IllegalArg, + "Value '%s' is not a positive integer subsampling factor", + level.c_str()); + std::exit(1); + } anLevels.push_back(atoi(level.c_str())); + if (anLevels.back() <= 0) + { + CPLError( + CE_Failure, CPLE_IllegalArg, + "Value '%s' is not a positive integer subsampling factor", + level.c_str()); + std::exit(1); + } if (anLevels.back() == 1) { printf( diff --git a/autotest/gcore/data/test_11555.tif b/autotest/gcore/data/test_11555.tif new file mode 100644 index 000000000000..4ae9ddecb0a6 Binary files /dev/null and b/autotest/gcore/data/test_11555.tif differ diff --git a/autotest/gcore/tiff_ovr.py b/autotest/gcore/tiff_ovr.py index 323fd5423e69..11644f738c50 100755 --- a/autotest/gcore/tiff_ovr.py +++ b/autotest/gcore/tiff_ovr.py @@ -165,6 +165,24 @@ def test_tiff_ovr_3(mfloat32_tif, both_endian): src_ds = None +############################################################################### +# + + +@gdaltest.enable_exceptions() +def test_tiff_ovr_invalid_ovr_factor(tmp_path): + tif_fname = str(tmp_path / "byte.tif") + + shutil.copyfile("data/byte.tif", tif_fname) + + ds = gdal.Open(tif_fname, gdal.GA_Update) + with pytest.raises( + Exception, + match=r"panOverviewList\[1\] = 0 is invalid\. It must be a positive value", + ): + ds.BuildOverviews(overviewlist=[2, 0]) + + ############################################################################### # Test generation @@ -1675,6 +1693,33 @@ def test_tiff_ovr_43(tmp_path, both_endian): assert cs == 642, "did not get expected checksum" +############################################################################### +# Test that we do not propagate PHOTOMETRIC=YCBCR on overviews when +# COMPRESS_OVERVIEW != JPEG + + +@pytest.mark.require_creation_option("GTiff", "JPEG") +@gdaltest.enable_exceptions() +def test_tiff_ovr_do_not_propagate_photometric_ycbcr_if_ovr_if_not_jpeg(tmp_path): + + tif_fname = str(tmp_path / "test.tif") + + ds = gdal.GetDriverByName("GTiff").Create( + tif_fname, 8, 8, 3, options=["COMPRESS=JPEG", "PHOTOMETRIC=YCBCR"] + ) + ds.GetRasterBand(1).Fill(255) + ds.GetRasterBand(2).Fill(255) + ds.GetRasterBand(3).Fill(255) + ds = None + + with gdal.Open(tif_fname, gdal.GA_Update) as ds: + with gdal.config_option("COMPRESS_OVERVIEW", "DEFLATE"): + ds.BuildOverviews("NEAR", [2]) + + with gdal.Open(tif_fname) as ds: + assert ds.GetRasterBand(1).GetOverview(0).ComputeRasterMinMax() == (255, 255) + + ############################################################################### # Test that we can change overview block size through GDAL_TIFF_OVR_BLOCKSIZE configuration # option @@ -2950,3 +2995,38 @@ def test_tiff_ovr_JXL_ALPHA_DISTANCE_OVERVIEW(tmp_vsimem): assert ds.GetRasterBand(4).Checksum() != cs4 del ds gdal.Unlink(tmpfilename + ".ovr") + + +############################################################################### +# Test fix for https://github.com/OSGeo/gdal/issues/11555 + + +def test_tiff_ovr_internal_mask_issue_11555(tmp_vsimem): + + if "debug build" in gdal.VersionInfo("--version") and "CI" in os.environ: + pytest.skip("test skipped on CI for debug builds (to keep things fast)") + + tmpfilename = str(tmp_vsimem / "test.tif") + gdal.FileFromMemBuffer(tmpfilename, open("data/test_11555.tif", "rb").read()) + + ds = gdal.Open(tmpfilename, gdal.GA_Update) + ds.BuildOverviews("bilinear", [2]) + del ds + + ds = gdal.Open(tmpfilename) + + # Check that we have non-zero data when mask = 255 + assert ds.GetRasterBand(1).GetOverview(0).ReadRaster(0, 5270, 1, 1) == b"\x7F" + assert ds.GetRasterBand(2).GetOverview(0).ReadRaster(0, 5270, 1, 1) == b"\x7F" + assert ( + ds.GetRasterBand(1).GetMaskBand().GetOverview(0).ReadRaster(0, 5270, 1, 1) + == b"\xFF" + ) + + # Check that we have zero data when mask = 0 + assert ds.GetRasterBand(1).GetOverview(0).ReadRaster(0, 5271, 1, 1) == b"\x00" + assert ds.GetRasterBand(2).GetOverview(0).ReadRaster(0, 5271, 1, 1) == b"\x00" + assert ( + ds.GetRasterBand(1).GetMaskBand().GetOverview(0).ReadRaster(0, 5271, 1, 1) + == b"\x00" + ) diff --git a/autotest/gcore/tiff_read.py b/autotest/gcore/tiff_read.py index 07ce7e15163f..a6f9352aaa59 100755 --- a/autotest/gcore/tiff_read.py +++ b/autotest/gcore/tiff_read.py @@ -5134,6 +5134,92 @@ def method(request): gdal.VSICurlClearCache() +############################################################################### +# Test that we honor GDAL_DISABLE_READDIR_ON_OPEN when working on a dataset opened with OVERVIEW_LEVEL open option + + +@pytest.mark.require_curl() +@pytest.mark.skipif( + not check_libtiff_internal_or_at_least(4, 0, 11), + reason="libtiff >= 4.0.11 required", +) +def test_tiff_read_multi_threaded_vsicurl_error_in_IsBlocksAvailable( + tmp_path, +): + + webserver_process = None + webserver_port = 0 + + (webserver_process, webserver_port) = webserver.launch( + handler=webserver.DispatcherHttpHandler + ) + if webserver_port == 0: + pytest.skip() + + gdal.VSICurlClearCache() + + try: + tmp_filename = str(tmp_path / "test.tif") + ds = gdal.GetDriverByName("GTiff").Create( + tmp_filename, 2001, 10000, 1, options=["SPARSE_OK=YES", "BLOCKYSIZE=1"] + ) + ds.GetRasterBand(1).SetNoDataValue(255) + ds.GetRasterBand(1).Fill(255) + ds.Close() + + filesize = gdal.VSIStatL(tmp_filename).size + handler = webserver.SequentialHandler() + handler.add("HEAD", "/test.tif", 200, {"Content-Length": "%d" % filesize}) + + def method(request): + # sys.stderr.write('%s\n' % str(request.headers)) + + if request.headers["Range"].startswith("bytes="): + rng = request.headers["Range"][len("bytes=") :] + assert len(rng.split("-")) == 2 + start = int(rng.split("-")[0]) + end = int(rng.split("-")[1]) + + request.protocol_version = "HTTP/1.1" + request.send_response(206) + request.send_header("Content-type", "application/octet-stream") + request.send_header( + "Content-Range", "bytes %d-%d/%d" % (start, end, filesize) + ) + request.send_header("Content-Length", end - start + 1) + request.send_header("Connection", "close") + request.end_headers() + with open(tmp_filename, "rb") as f: + f.seek(start, 0) + request.wfile.write(f.read(end - start + 1)) + + for i in range(2): + handler.add("GET", "/test.tif", custom_method=method) + handler.add("GET", "/test.tif", 404) + + with gdaltest.config_options( + { + "GDAL_NUM_THREADS": "2", + "CPL_VSIL_CURL_ALLOWED_EXTENSIONS": ".tif", + "GDAL_DISABLE_READDIR_ON_OPEN": "EMPTY_DIR", + } + ): + with webserver.install_http_handler(handler): + ds = gdal.OpenEx( + "/vsicurl/http://127.0.0.1:%d/test.tif" % webserver_port, + ) + with pytest.raises( + Exception, + match="_TIFFPartialReadStripArray:Cannot read offset/size for strile", + ): + ds.GetRasterBand(1).ReadRaster() + + finally: + webserver.server_stop(webserver_process, webserver_port) + + gdal.VSICurlClearCache() + + ############################################################################### # Test that a user receives a warning when it queries # GetMetadataItem("PIXELTYPE", "IMAGE_STRUCTURE") diff --git a/autotest/utilities/test_gdaladdo.py b/autotest/utilities/test_gdaladdo.py index d8d8821a8f4c..ef75ddc943d8 100755 --- a/autotest/utilities/test_gdaladdo.py +++ b/autotest/utilities/test_gdaladdo.py @@ -455,3 +455,33 @@ def test_gdaladdo_partial_refresh_from_source_timestamp_gti(gdaladdo_path, tmp_p ovr_data_refreshed[idx] = ovr_data_ori[idx] assert ovr_data_refreshed == ovr_data_ori ds = None + + +############################################################################### +# + + +def test_gdaladdo_illegal_factor(gdaladdo_path, tmp_path): + + shutil.copyfile("../gcore/data/byte.tif", f"{tmp_path}/byte.tif") + + _, err = gdaltest.runexternal_out_and_err( + f"{gdaladdo_path} -r average {tmp_path}/byte.tif invalid" + ) + assert "Value 'invalid' is not a positive integer subsampling factor" in err + with gdal.Open(f"{tmp_path}/byte.tif") as ds: + assert ds.GetRasterBand(1).GetOverviewCount() == 0 + + _, err = gdaltest.runexternal_out_and_err( + f"{gdaladdo_path} -r average {tmp_path}/byte.tif 0" + ) + assert "Value '0' is not a positive integer subsampling factor" in err + with gdal.Open(f"{tmp_path}/byte.tif") as ds: + assert ds.GetRasterBand(1).GetOverviewCount() == 0 + + _, err = gdaltest.runexternal_out_and_err( + f"{gdaladdo_path} -r average {tmp_path}/byte.tif -1" + ) + assert "Value '-1' is not a positive integer subsampling factor" in err + with gdal.Open(f"{tmp_path}/byte.tif") as ds: + assert ds.GetRasterBand(1).GetOverviewCount() == 0 diff --git a/frmts/gtiff/gtiffdataset.h b/frmts/gtiff/gtiffdataset.h index 3bca95d0ad33..8d8a543279ec 100644 --- a/frmts/gtiff/gtiffdataset.h +++ b/frmts/gtiff/gtiffdataset.h @@ -326,9 +326,8 @@ class GTiffDataset final : public GDALPamDataset int GetJPEGOverviewCount(); - bool IsBlockAvailable(int nBlockId, vsi_l_offset *pnOffset = nullptr, - vsi_l_offset *pnSize = nullptr, - bool *pbErrOccurred = nullptr); + bool IsBlockAvailable(int nBlockId, vsi_l_offset *pnOffset, + vsi_l_offset *pnSize, bool *pbErrOccurred); void ApplyPamInfo(); void PushMetadataToPam(); diff --git a/frmts/gtiff/gtiffdataset_read.cpp b/frmts/gtiff/gtiffdataset_read.cpp index 81c587b0f167..6bf8134c6374 100644 --- a/frmts/gtiff/gtiffdataset_read.cpp +++ b/frmts/gtiff/gtiffdataset_read.cpp @@ -142,7 +142,7 @@ CPLStringList GTiffDataset::GetCompressionFormats(int nXOff, int nYOff, vsi_l_offset nOffset = 0; vsi_l_offset nSize = 0; - if (IsBlockAvailable(nBlockId, &nOffset, &nSize) && + if (IsBlockAvailable(nBlockId, &nOffset, &nSize, nullptr) && nSize < static_cast(std::numeric_limits::max())) { @@ -222,7 +222,7 @@ CPLErr GTiffDataset::ReadCompressedData(const char *pszFormat, int nXOff, vsi_l_offset nOffset = 0; vsi_l_offset nSize = 0; - if (IsBlockAvailable(nBlockId, &nOffset, &nSize) && + if (IsBlockAvailable(nBlockId, &nOffset, &nSize, nullptr) && nSize < static_cast( std::numeric_limits::max())) { @@ -1305,6 +1305,7 @@ CPLErr GTiffDataset::MultiThreadedRead(int nXOff, int nYOff, int nXSize, nBlockId += asJobs[iJob].iSrcBandIdxSeparate * m_nBlocksPerBand; + bool bErrorInIsBlockAvailable = false; if (!sContext.bHasPRead) { // Taking the mutex here is only needed when bHasPRead == @@ -1314,13 +1315,22 @@ CPLErr GTiffDataset::MultiThreadedRead(int nXOff, int nYOff, int nXSize, std::lock_guard oLock( sContext.oMutex); - IsBlockAvailable(nBlockId, &asJobs[iJob].nOffset, - &asJobs[iJob].nSize); + CPL_IGNORE_RET_VAL(IsBlockAvailable( + nBlockId, &asJobs[iJob].nOffset, &asJobs[iJob].nSize, + &bErrorInIsBlockAvailable)); } else { - IsBlockAvailable(nBlockId, &asJobs[iJob].nOffset, - &asJobs[iJob].nSize); + CPL_IGNORE_RET_VAL(IsBlockAvailable( + nBlockId, &asJobs[iJob].nOffset, &asJobs[iJob].nSize, + &bErrorInIsBlockAvailable)); + } + if (bErrorInIsBlockAvailable) + { + std::lock_guard oLock( + sContext.oMutex); + sContext.bSuccess = false; + return CE_Failure; } // Sanity check on block size @@ -1344,7 +1354,7 @@ CPLErr GTiffDataset::MultiThreadedRead(int nXOff, int nYOff, int nXSize, std::lock_guard oLock( sContext.oMutex); sContext.bSuccess = false; - break; + return CE_Failure; } } @@ -6383,7 +6393,7 @@ const char *GTiffDataset::GetMetadataItem(const char *pszName, { vsi_l_offset nOffset = 0; vsi_l_offset nSize = 0; - IsBlockAvailable(0, &nOffset, &nSize); + IsBlockAvailable(0, &nOffset, &nSize, nullptr); if (nSize > 0) { const std::string osSubfile( diff --git a/frmts/gtiff/gtiffdataset_write.cpp b/frmts/gtiff/gtiffdataset_write.cpp index 095b9d19a9e3..de4c48002ed7 100644 --- a/frmts/gtiff/gtiffdataset_write.cpp +++ b/frmts/gtiff/gtiffdataset_write.cpp @@ -397,7 +397,7 @@ CPLErr GTiffDataset::FillEmptyTiles() } vsi_l_offset nOffset = 0; - if (!IsBlockAvailable(iBlock, &nOffset, &nRawSize)) + if (!IsBlockAvailable(iBlock, &nOffset, &nRawSize, nullptr)) break; // When using compression, get back the compressed block @@ -614,7 +614,7 @@ bool GTiffDataset::WriteEncodedTile(uint32_t tile, GByte *pabyData, /* -------------------------------------------------------------------- */ if (!m_bWriteEmptyTiles && IsFirstPixelEqualToNoData(pabyData)) { - if (!IsBlockAvailable(tile)) + if (!IsBlockAvailable(tile, nullptr, nullptr, nullptr)) { const int nComponents = m_nPlanarConfig == PLANARCONFIG_CONTIG ? nBands : 1; @@ -800,7 +800,7 @@ bool GTiffDataset::WriteEncodedStrip(uint32_t strip, GByte *pabyData, /* -------------------------------------------------------------------- */ if (!m_bWriteEmptyTiles && IsFirstPixelEqualToNoData(pabyData)) { - if (!IsBlockAvailable(strip)) + if (!IsBlockAvailable(strip, nullptr, nullptr, nullptr)) { const int nComponents = m_nPlanarConfig == PLANARCONFIG_CONTIG ? nBands : 1; @@ -2709,7 +2709,10 @@ bool GTiffDataset::GetOverviewParameters( /* -------------------------------------------------------------------- */ /* Determine photometric tag */ /* -------------------------------------------------------------------- */ - nPhotometric = m_nPhotometric; + if (m_nPhotometric == PHOTOMETRIC_YCBCR && nCompression != COMPRESSION_JPEG) + nPhotometric = PHOTOMETRIC_RGB; + else + nPhotometric = m_nPhotometric; const char *pszPhotometric = GetOptionValue("PHOTOMETRIC", "PHOTOMETRIC_OVERVIEW", &pszOptionKey); if (!GTIFFUpdatePhotometric(pszPhotometric, pszOptionKey, nCompression, diff --git a/frmts/gtiff/gtiffrasterband_read.cpp b/frmts/gtiff/gtiffrasterband_read.cpp index 63b381671375..c29be2bc98f4 100644 --- a/frmts/gtiff/gtiffrasterband_read.cpp +++ b/frmts/gtiff/gtiffrasterband_read.cpp @@ -983,8 +983,8 @@ void *GTiffRasterBand::CacheMultiRange(int nXOff, int nYOff, int nXSize, } else { - CPL_IGNORE_RET_VAL( - m_poGDS->IsBlockAvailable(nBlockId, &nOffset, &nSize)); + CPL_IGNORE_RET_VAL(m_poGDS->IsBlockAvailable( + nBlockId, &nOffset, &nSize, nullptr)); } if (nSize) { @@ -1071,9 +1071,17 @@ void *GTiffRasterBand::CacheMultiRange(int nXOff, int nYOff, int nXSize, VSILFILE *fp = VSI_TIFFGetVSILFile(th); - if (VSIFReadMultiRangeL(static_cast(anSizes.size()), + // An error in VSIFReadMultiRangeL() will not be criticial, + // as this method is an optimization, and if it fails, + // tile-by-tile data acquisition will be done, so we can + // temporary turn failures into warnings. + CPLTurnFailureIntoWarning(true); + const bool ok = + VSIFReadMultiRangeL(static_cast(anSizes.size()), &apData[0], &anOffsets[0], &anSizes[0], - fp) == 0) + fp) == 0; + CPLTurnFailureIntoWarning(false); + if (ok) { if (!oMapStrileToOffsetByteCount.empty() && !FillCacheStrileToOffsetByteCount(anOffsets, anSizes, @@ -1093,6 +1101,11 @@ void *GTiffRasterBand::CacheMultiRange(int nXOff, int nYOff, int nXSize, th, static_cast(anSizes.size()), &apData[0], &anOffsets[0], &anSizes[0]); } + else + { + CPLFree(pBufferedData); + pBufferedData = nullptr; + } } } } @@ -1129,8 +1142,12 @@ int GTiffRasterBand::IGetDataCoverageStatus(int nXOff, int nYOff, int nXSize, vsi_l_offset nOffset = 0; vsi_l_offset nLength = 0; bool bHasData = false; - if (!m_poGDS->IsBlockAvailable(nBlockId, &nOffset, &nLength)) + bool bError = false; + if (!m_poGDS->IsBlockAvailable(nBlockId, &nOffset, &nLength, + &bError)) { + if (bError) + return GDAL_DATA_COVERAGE_STATUS_UNIMPLEMENTED; nStatus |= GDAL_DATA_COVERAGE_STATUS_EMPTY; } else @@ -1584,7 +1601,8 @@ const char *GTiffRasterBand::GetMetadataItem(const char *pszName, } vsi_l_offset nOffset = 0; - if (!m_poGDS->IsBlockAvailable(nBlockId, &nOffset)) + if (!m_poGDS->IsBlockAvailable(nBlockId, &nOffset, nullptr, + nullptr)) { return nullptr; } @@ -1605,7 +1623,8 @@ const char *GTiffRasterBand::GetMetadataItem(const char *pszName, } vsi_l_offset nByteCount = 0; - if (!m_poGDS->IsBlockAvailable(nBlockId, nullptr, &nByteCount)) + if (!m_poGDS->IsBlockAvailable(nBlockId, nullptr, &nByteCount, + nullptr)) { return nullptr; } diff --git a/frmts/gtiff/gtiffrgbaband.cpp b/frmts/gtiff/gtiffrgbaband.cpp index d527cfd50de6..fdaed879a29f 100644 --- a/frmts/gtiff/gtiffrgbaband.cpp +++ b/frmts/gtiff/gtiffrgbaband.cpp @@ -66,13 +66,14 @@ CPLErr GTiffRGBABand::IReadBlock(int nBlockXOff, int nBlockYOff, void *pImage) for (int iBand = 0; iBand < m_poGDS->m_nSamplesPerPixel; iBand++) { int nBlockIdBand = nBlockId + iBand * m_poGDS->m_nBlocksPerBand; - if (!m_poGDS->IsBlockAvailable(nBlockIdBand)) + if (!m_poGDS->IsBlockAvailable(nBlockIdBand, nullptr, nullptr, + nullptr)) return CE_Failure; } } else { - if (!m_poGDS->IsBlockAvailable(nBlockId)) + if (!m_poGDS->IsBlockAvailable(nBlockId, nullptr, nullptr, nullptr)) return CE_Failure; } diff --git a/gcore/gdaldataset.cpp b/gcore/gdaldataset.cpp index ea56ac9038c5..ca17538cead6 100644 --- a/gcore/gdaldataset.cpp +++ b/gcore/gdaldataset.cpp @@ -2104,7 +2104,8 @@ CPLErr GDALSetGCPs2(GDALDatasetH hDS, int nGCPCount, const GDAL_GCP *pasGCPList, * "BILINEAR", "CUBIC", "CUBICSPLINE", "GAUSS", "LANCZOS", "MODE", "NEAREST", * or "NONE" controlling the downsampling method applied. * @param nOverviews number of overviews to build, or 0 to clean overviews. - * @param panOverviewList the list of overview decimation factors to build, or + * @param panOverviewList the list of overview decimation factors (positive + * integers, normally larger or equal to 2) to build, or * NULL if nOverviews == 0. * @param nListBands number of bands to build overviews for in panBandList. * Build for all bands if this is 0. @@ -2151,6 +2152,18 @@ CPLErr GDALDataset::BuildOverviews(const char *pszResampling, int nOverviews, if (pfnProgress == nullptr) pfnProgress = GDALDummyProgress; + for (int i = 0; i < nOverviews; ++i) + { + if (panOverviewList[i] <= 0) + { + CPLError(CE_Failure, CPLE_IllegalArg, + "panOverviewList[%d] = %d is invalid. It must be a " + "positive value", + i, panOverviewList[i]); + return CE_Failure; + } + } + // At time of writing, all overview generation options are actually // expected to be passed as configuration options. std::vector> apoConfigOptionSetter; diff --git a/gcore/overview.cpp b/gcore/overview.cpp index 84277ffddbd5..2996d017064c 100644 --- a/gcore/overview.cpp +++ b/gcore/overview.cpp @@ -4964,7 +4964,7 @@ CPLErr GDALRegenerateOverviewsEx(GDALRasterBandH hSrcBand, int nOverviewCount, poJob->nSrcHeight = nHeight; poJob->args.nChunkXOff = 0; poJob->args.nChunkXSize = nWidth; - poJob->args.nChunkYOff = nChunkYOff; + poJob->args.nChunkYOff = nChunkYOffQueried; poJob->args.nChunkYSize = nChunkYSizeQueried; poJob->nDstWidth = nDstWidth; poJob->args.nDstXOff = 0; diff --git a/ogr/ogrsf_frmts/parquet/ogrparquetwriterlayer.cpp b/ogr/ogrsf_frmts/parquet/ogrparquetwriterlayer.cpp index d6c6e34c4aeb..d647d355c88a 100644 --- a/ogr/ogrsf_frmts/parquet/ogrparquetwriterlayer.cpp +++ b/ogr/ogrsf_frmts/parquet/ogrparquetwriterlayer.cpp @@ -467,6 +467,12 @@ bool OGRParquetWriterLayer::SetOptions(CSLConstList papszOptions, if (!CPLTestBool(CSLFetchNameValueDef(papszOptions, "STATISTICS", "YES"))) m_oWriterPropertiesBuilder.disable_statistics(); +#if PARQUET_VERSION_MAJOR >= 12 + // Undocumented option. Not clear it is useful to disable it. + if (CPLTestBool(CSLFetchNameValueDef(papszOptions, "PAGE_INDEX", "YES"))) + m_oWriterPropertiesBuilder.enable_write_page_index(); +#endif + if (m_eGeomEncoding == OGRArrowGeomEncoding::WKB && eGType != wkbNone) { m_oWriterPropertiesBuilder.disable_statistics(