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 black line appearing in some images #49

Closed
wants to merge 1 commit into from
Closed

Fix black line appearing in some images #49

wants to merge 1 commit into from

Conversation

t11r
Copy link

@t11r t11r commented Sep 30, 2016

Restore the previous size calculation to prevent a black horizontal line appearing in some images. This was caused by the height being calculated as one pixel too short on certain zoom levels.

Restore the previous size calculation to prevent a black horizontal line appearing in some images. This was caused by the height being calculated as one pixel too short on certain zoom levels.
@mejackreed
Copy link
Owner

Thanks for the contribution @tschaef! Could you provide some examples so we can test against this in the future?

Copy link
Owner

@mejackreed mejackreed left a comment

Choose a reason for hiding this comment

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

This image url request would now create a non-canonical image api request.

Could you provide some examples where you are seeing this so we could address the issue at a larger level? I'm concerned about removing canonical urls as it would not allow Level 0 consumers.

@t11r
Copy link
Author

t11r commented Sep 30, 2016

Hi @mejackreed, thanks for the quick reply and the great work with the plugin. I will try to identify why the problem only appears with certain images and get back to you with some examples.

@t11r
Copy link
Author

t11r commented Oct 12, 2016

Sorry for the long wait, I was travelling. Could you explain how this would create non-canonical requests? The IIIF API allows to provide either width or height or both, even for Level 0, or did I miss something here?

@mejackreed
Copy link
Owner

Sure thing, the canonical uri syntax is described here: http://iiif.io/api/image/2.1/#canonical-uri-syntax

From that document in the size section:

the w, syntax for images that should be scaled maintaining the aspect ratio,
and the w,h syntax for explicit sizes that change the aspect ratio.

As I read this PR, it seems like it introduces size requests that have ,h which would not conform to that. Since Leaflet-IIIF doesn't modify any aspect ratios, we aren't going to support the w,h parameters either.

The behavior you are describing I believe has been removed in the v2.1 image API. See: http://iiif.io/api/image/2.1/compliance/#size

cc @azaroth42 to make sure I'm reasoning correctly

@t11r
Copy link
Author

t11r commented Oct 12, 2016

I understood this as just an example, since ,h is still a valid size in v2.1.

As for a demo showing the black line: Check the Leaflet-IIIF example page. With the lowest zoom setting, it appears below the image (at least in current Chrome and Firefox). When using tiles, it could appear right in the middle.

@mejackreed
Copy link
Owner

Thanks @tschaef . The way I understand the canonical uri is that w, should only be requested. This allows adopters to pregenerate tiles nicely. See http://mejackreed.github.io/Leaflet-IIIF/examples/example.html "A static tile source".

As far as the line goes, I think this is due to rounding implementation differences in Leaflet-IIIF and the server. It seems like using Math.round instead of Math.ceil may solve this problem in both cases. What do you think?

@t11r
Copy link
Author

t11r commented Oct 13, 2016

Unfortunately, this does not work, since the height could still assume non-integer values with no control over how they are rounded. Maybe @azaroth42 could actually give us a hint?

@t11r
Copy link
Author

t11r commented Dec 20, 2017

The issue seems to be fixed, closing this.

@t11r t11r closed this Dec 20, 2017
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.

None yet

2 participants