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 out of range fetch isssue for Tiff #354

Merged
merged 2 commits into from
Jan 20, 2021

Conversation

ilan-gold
Copy link
Collaborator

@ilan-gold ilan-gold commented Jan 12, 2021

Addresses geotiffjs/geotiff.js#193 on our end. Here is the diff with our current branch: https://github.com/ilan-gold/geotiff.js/compare/ilan-gold/viv_release_080...ilan-gold:ilan-gold/viv_083?expand=1#

Basically, it is possible for tiff fetch sources to request data outside the size of the file which results in a 416 error from the server. This PR upgrades our geotiff version to one that makes sure that no requests are made for data outside that range.

You can test this out using http://localhost:8080/?image_url=https://vitessce-demo-data.storage.googleapis.com/test-data/VAN0006-LK-2-85-IMS_PosMode_multilayer.ome.tif?token= (and on Avivator to see the difference) and then selecting the last channel - in Avivator, it will not work but here it will.

@ilan-gold ilan-gold requested a review from manzt January 12, 2021 19:53
@ilan-gold ilan-gold marked this pull request as ready for review January 12, 2021 19:53
Copy link
Member

@manzt manzt left a comment

Choose a reason for hiding this comment

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

Ok this looks reasonable to me. The main concern (related to still using the geotiff fork in general) is that PRs like this look tiny, but really are much larger. I can't leave suggestions in the geotiff fork source, and at this point I don't know how our fork has diverged over time, but it seemingly is non-trivial.

@ilan-gold ilan-gold merged commit f11a3c2 into master Jan 20, 2021
@ilan-gold ilan-gold deleted the ilan-gold/fix_geotiff_outofrange_issue branch January 20, 2021 19:07
@constantinius
Copy link

We try to tackle this issue on geotiff.js itself.

Do you have a list of other places where your fork diverges from the upstream geotiff.js? I think it would be a shame if geotiff.js proper is not usable for you.

@ilan-gold
Copy link
Collaborator Author

@constantinius Thanks! geotiff.js is very usable for us, it's just that sometimes we need things faster than you can review PR's (we have made quite a few - it's understandable!), so no knock on you. Our work would be much much harder without geotiff.js and the library is really well-done so no complaints here.

The two PR's that you could review are geotiffjs/geotiff.js#190 and geotiffjs/geotiff.js#182 (which I see is now out of sync because of your bug fix for this issue).

In addition, we use the LZW decoder that @manzt mentioned in here but it doesn't seem to have ever made its way into your repo - Not sure what we want to do about that one, but am open to suggestions and happy to help as always!!!

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