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

Fix Overviews Read Incorrectly when Per Dataset Masks Present bug #3271

Merged
merged 3 commits into from
Jul 16, 2020

Conversation

pomadchin
Copy link
Member

@pomadchin pomadchin commented Jul 15, 2020

Overview

With this PR GeoTiff reader will skip all IFDs that are not a ReducedImage.

Checklist

  • ./CHANGELOG.md updated, if necessary. Link to the issue if closed, otherwise the PR.
  • Unit tests added for bug-fix or new feature

Closes #3269

@pomadchin pomadchin force-pushed the fix/overviews-reader branch 2 times, most recently from 69bcb2f to 7b0181c Compare July 15, 2020 21:33
@pomadchin pomadchin changed the title Overviews Read Incorrectly when Per Dataset Masks Present bug Fix Overviews Read Incorrectly when Per Dataset Masks Present bug Jul 15, 2020
// TIFF Reader supports only overviews at this point
// Overview is a reduced-resolution IFD
val subfileType = ifdTiffTags.nonBasicTags.newSubfileType.flatMap(NewSubfileType.fromCode)
if(subfileType.contains(ReducedImage)) tiffTagsBuffer += ifdTiffTags
Copy link
Member Author

Choose a reason for hiding this comment

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

The idea is that we're skipping everything that is not a ReducedImage (anyway we can't work with anything else)

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 but also noting that this kind of comment is super helpful for a user to see in the method docstring. It would be helpful for the user to know at a glance something like "Only SubfileType.ReducedImage tiffs are supported".

Comment on lines 34 to 45
/** Reduced-resolution version of a transparency mask */
case object MaskReducedImage extends NewSubfileType { val code = 5 }
/** Transparency mask of multi-page image */
case object MaskMultiPage extends NewSubfileType { val code = 6 }
/** Transparency mask of reduced-resolution multi-page image */
case object MaskMultiPageReducedImage extends NewSubfileType { val code = 7 }
/** Depth map */
case object Depth extends NewSubfileType { val code = 8 }
/** Depth map of reduced-resolution image */
case object DepthReducedImage extends NewSubfileType { val code = 9 }
/** Enhanced image data */
case object Enhanced extends NewSubfileType { val code = 10 }
Copy link
Member Author

Choose a reason for hiding this comment

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

I decided to cover it with more types, see https://exiftool.org/TagNames/EXIF.html

Copy link
Contributor

@CloudNiner CloudNiner left a comment

Choose a reason for hiding this comment

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

👍 after fixing that incorrect case match

// TIFF Reader supports only overviews at this point
// Overview is a reduced-resolution IFD
val subfileType = ifdTiffTags.nonBasicTags.newSubfileType.flatMap(NewSubfileType.fromCode)
if(subfileType.contains(ReducedImage)) tiffTagsBuffer += ifdTiffTags
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 but also noting that this kind of comment is super helpful for a user to see in the method docstring. It would be helpful for the user to know at a glance something like "Only SubfileType.ReducedImage tiffs are supported".

@@ -18,9 +18,7 @@ package geotrellis.raster.io.geotiff.tags

import ProjectionTypesMap._
import geotrellis.raster.io.geotiff.reader.MalformedGeoTiffException
import geotrellis.raster.io.geotiff.util._
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉 big fan of removal of unused imports whenever possible!

@pomadchin pomadchin merged commit 25876a5 into locationtech:master Jul 16, 2020
@pomadchin pomadchin deleted the fix/overviews-reader branch July 16, 2020 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Overviews Read Incorrectly when Per Dataset Masks Present
2 participants