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

TiffSaver: add overwriteIFDValue signature that takes an IFD offset #3410

Merged
merged 2 commits into from
Oct 23, 2019

Conversation

melissalinkert
Copy link
Member

@melissalinkert melissalinkert commented Jul 22, 2019

Backported from a private PR.

This is helpful for modifying thumbnail and sub-IFDs, which aren't
included in the list of offsets returned by TiffParser.getIFDOffsets().

I wouldn't expect this to have any impact on memo files or existing tests, but it arguably still requires a minor release since it's an API addition.

This is helpful for modifying thumbnail and sub-IFDs, which aren't
included in the list of offsets returned by TiffParser.getIFDOffsets().
@dgault dgault added this to the 6.3.0 milestone Jul 24, 2019
public void overwriteIFDValue(RandomAccessInputStream raf,
long ifdOffset, int tag, Object value) throws FormatException, IOException
{
raf.seek(0);
Copy link
Member

Choose a reason for hiding this comment

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

With this refactoring, does that mean any call to the current overwriteIFDValue(RandomAccessInputStream, int, int, Object) API will now perform a redundant check on the first IFD?

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 header will be checked twice, yes. Could refactor further to add a overwriteIFDValue(RandomAccessInputStream, long, int, Object, boolean skipHeaderCheck)? Otherwise not sure of a good and safe way to skip that check.

Copy link
Member

Choose a reason for hiding this comment

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

Since we're introducing new API anyways, I think introducing

public void overwriteIFDValue(RandomAccessInputStream raf, long ifdOffset, int tag, Object value, boolean skipskipHeaderCheck)

is acceptable. From there both public void overwriteIFDValue(RandomAccessInputStream raf, long ifdOffset, int tag, Object value) and public void overwriteIFDValue(RandomAccessInputStream raf, int ifd, int tag, Object value) can delegate to this API by passing the appropriate boolean value.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added in b43e748

@dgault
Copy link
Member

dgault commented Oct 23, 2019

With the latest update to add skipHeaderCheck the API updates here look to be good.

The existing unit tests, repo tests and workflows are unaffected by the changes. I reran some local tests using the PyramidOMETiffWriter and the generated images and metadata all looked correct and no regressions found from these additions.

@dgault dgault merged commit 6b30472 into ome:develop Oct 23, 2019
@melissalinkert melissalinkert deleted the tiff-overwrite-ifd-value-develop branch December 13, 2019 15:36
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.

3 participants