Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[gdalwarp] Fix -ovr #10193

Merged
merged 2 commits into from
Jun 12, 2024
Merged

[gdalwarp] Fix -ovr #10193

merged 2 commits into from
Jun 12, 2024

Conversation

elpaso
Copy link
Collaborator

@elpaso elpaso commented Jun 11, 2024

Fix #10174 -ovr {absolute_value} doesn't influence choice of output resolution

@elpaso elpaso added the bug label Jun 11, 2024
GDALDatasetH hDstDSOverview = nullptr;
if (psOptions->nOvLevel >= 0)
{
hDstDSOverview = GDALCreateOverviewDataset(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will leak hDstDSOverview, and more annoyingly this will crash if nSrcCount > 1.
I believe the call to GDALCreateOverviewDataset() should be moved to CreateOutput() and applied to all input datasets

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, it's much more complex that I imagined.

I think I have now the proper implementation but there is still a failing test that I don't know how to handle (what is the expected behavior):

    # Crates a full res output file
    gdaltest.runexternal(f"{gdalwarp_path} {src_tif} {dst_tif}")
    # The output file has now a "wrong" resolution and no overwrite was specified
    gdaltest.runexternal(f"{gdalwarp_path} {src_tif} {dst_tif} -ovr 0")
    ds = gdal.Open(dst_tif)
    # Fail! Full res is used instead of overview
    assert ds.GetRasterBand(1).Checksum() == cs_ov0

What are we supposed to do here?
Error out or silently overwrite the output file (that has full res instead of the ovr res)?

The code path in this case skips createOutput completely.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are we supposed to do here?
Error out or silently overwrite the output file (that has full res instead of the ovr res)?

There should already be a warning emitted at line 2860 (in master): "cannot get overview level ...". This is probably enough for now

@coveralls
Copy link
Collaborator

Coverage Status

coverage: 69.182% (-0.3%) from 69.455%
when pulling aa38443 on elpaso:bugfix-gdalwarp-ovr
into 99870b9 on OSGeo:master.

@elpaso elpaso force-pushed the bugfix-gdalwarp-ovr branch 3 times, most recently from e63a88d to f224d56 Compare June 12, 2024 08:39
Fix OSGeo#10174 -ovr {absolute_value} doesn't influence choice of output resolution
@rouault rouault merged commit 75c9020 into OSGeo:master Jun 12, 2024
35 of 36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gdalwarp -ovr {absolute_value} doesn't influence choice of output resolution
3 participants