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

ISSUE-71: Make IABookreader aware of arguments in Image/Service @ids. #73

Merged
merged 1 commit into from
Mar 31, 2020

Conversation

DiegoPino
Copy link
Member

See #71

What is new here?

We use non standard IIIF extensions to be able to parse PDF pages from a single File as individual images. But in the IABookreader IIIF plugin we use the service @id of each image resource to build a full size image URL on the fly, to allow larger than desired Image sizes and zoom. NOTE: not sure that is really a good idea, we used the code IABookreader had for that and now looking this back in time it feels wrong.

In any case, by using the service URI we dismiss any argument, like ?page=1 that could be passed, which includes also video frame times, etc. This pull changes that by parsing out of the Image ID any arguments that could be passed and appending them to the final Image ID (URL) that is being built. This, in case of the absence of a Service and its @id we also know call directly the Image ID.

This needs some testing and also rethinking some IAbookreader logic, since we do ask for image sizes dynamically, but in the absence of a Service info.json, that makes no sense at all.

…nts over

This commit, for IIIF Display API, item manifests 2.1,  uses service @id only if present and passes any arguments the image could have to the Image URI builder (if not uses the given Resource/Image @id as instructed).

I'm still doubtfull this is the correct way, i feel we should always, like always use the Image ID. This current behaviour was inherited from the demo code that IABookreader had for IIIF, and it feels wrong.  Mostly because, if the IIIF manifest is already pointing to a given Image URL, then making an aditional call to a (maybe not even existing) info.json is overriding the users/creator wishes. Again, the original code was not done by us.

In any case, i need to also remove any calls to with and height if there is no service right? Maybe we need to test with some IIIF manifest (public?) where service is not present. @giancarlobi
@DiegoPino DiegoPino requested a review from giancarlobi March 30, 2020 18:05
@DiegoPino DiegoPino self-assigned this Mar 30, 2020
@DiegoPino DiegoPino added this to the 1.0.0-beta3 milestone Mar 30, 2020
@DiegoPino
Copy link
Member Author

@giancarlobi some code to discuss here. Let me know what do you think

@DiegoPino
Copy link
Member Author

@giancarlobi also: In case of lack of image width/height and no service, we can use JS like this

function getMeta(url) {
var img = new Image();
img.addEventListener("load", function() {
alert(this.naturalWidth + ' ' + this.naturalHeight);
});
img.src = url;
}

getMeta("http://archipelago.nyc:8183/iiif/2/8dc%2Fimage-page-4-ad397f4d-9dfd-4bbc-9e80-5b3516c213ae.jpg/full/1200,/0/default.jpg");

@DiegoPino
Copy link
Member Author

@giancarlobi also, this is the reason why mirador has issues (same ones). It depends completely on an existing Service key, seems like it can not use static Image resource @id to fetch an image, and will always do the following

https://github.com/ProjectMirador/mirador/blob/bc253b9331275d2f84ebb4961871109a9a11ac33/src/state/actions/infoResponse.js#L95

Which basically what we are trying to fix on IABook Reader. Tomorrow i will open an issue in Mirador for a) Static images without a service, pretty sure one is already open (2016 i think i saw something) and something about arguments passed around. Maybe they have an idea...

@DiegoPino
Copy link
Member Author

@giancarlobi @mitchellkeaney i also updated the manifest https://gist.github.com/DiegoPino/724ce3b703f870acb63c3340c7f9d897
With this code (once merged) and that manifest IABookreader can show a PDF. Still there are issues we need to deal with as image sizes (beta3)
Also, with this new manifest Mirador stopped freezing, shows every page as the same! but stopped freezing.

@giancarlobi
Copy link
Collaborator

giancarlobi commented Mar 31, 2020

@DiegoPino I confirm the fix works (see http://archipelago.byterfly.eu/node/29).
My morning with other issues to solve so I think and answer late this afternoon about your questions.

@DiegoPino DiegoPino linked an issue Mar 31, 2020 that may be closed by this pull request
@giancarlobi
Copy link
Collaborator

@giancarlobi also: In case of lack of image width/height and no service, we can use JS like this

function getMeta(url) {
var img = new Image();
img.addEventListener("load", function() {
alert(this.naturalWidth + ' ' + this.naturalHeight);
});
img.src = url;
}

getMeta("http://archipelago.nyc:8183/iiif/2/8dc%2Fimage-page-4-ad397f4d-9dfd-4bbc-9e80-5b3516c213ae.jpg/full/1200,/0/default.jpg");

@DiegoPino Yes, it seems a good solution.
Am I wrong or you need in call .../full/full/... not .../full/1200,/... ? Because we need original dimensions and not forced to 1200 width.

@giancarlobi
Copy link
Collaborator

@giancarlobi also, this is the reason why mirador has issues (same ones). It depends completely on an existing Service key, seems like it can not use static Image resource @id to fetch an image, and will always do the following

https://github.com/ProjectMirador/mirador/blob/bc253b9331275d2f84ebb4961871109a9a11ac33/src/state/actions/infoResponse.js#L95

Which basically what we are trying to fix on IABook Reader. Tomorrow i will open an issue in Mirador for a) Static images without a service, pretty sure one is already open (2016 i think i saw something) and something about arguments passed around. Maybe they have an idea...

@DiegoPino as for Presentation API 2.1, Service is not a MUST so I agree with you that the wrong assumption is in Mirador code.

@DiegoPino
Copy link
Member Author

@giancarlobi i added a comment here
ProjectMirador/mirador#2616 (comment)
But will probably have to open a new issue to get more support/feedback

Copy link
Collaborator

@giancarlobi giancarlobi left a comment

Choose a reason for hiding this comment

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

@DiegoPino I think we can merge because code solves main issue. Maybe open a new issue to solve width issue?

@DiegoPino
Copy link
Member Author

@DiegoPino Yes, it seems a good solution.
Am I wrong or you need in call .../full/full/... not .../full/1200,/... ? Because we need original dimensions and not forced to 1200 width.

Regarding this @giancarlobi reason for calling it with 1200 is because i assume we could to limit the access to a certain size (imagine the call is really to someurl/somefixedsizeimage.jp2) but also because the canvas width and height we need (by specs) are the aspect ratio. So with 1200 Javascript will be faster! (can be even super smaller) and we will get whatever height and then we can calculate the ratio. But yes, FULL can work, just slower. Does that make sense?

@DiegoPino
Copy link
Member Author

@giancarlobi yes! we can merge and then deal in another issue with sizes and the fact that we are always calling the info.json. Does it still work with normal manifests based on images after the change? I'm pretty good at breaking working code! jajajjaa

@giancarlobi
Copy link
Collaborator

@giancarlobi yes! we can merge and then deal in another issue with sizes and the fact that we are always calling the info.json. Does it still work with normal manifests based on images after the change? I'm pretty good at breaking working code! jajajjaa

@DiegoPino Yes!! Still works with standard book by Tiff and book by manifest, great!!

@giancarlobi
Copy link
Collaborator

@DiegoPino Yes, it seems a good solution.
Am I wrong or you need in call .../full/full/... not .../full/1200,/... ? Because we need original dimensions and not forced to 1200 width.

Regarding this @giancarlobi reason for calling it with 1200 is because i assume we could to limit the access to a certain size (imagine the call is really to someurl/somefixedsizeimage.jp2) but also because the canvas width and height we need (by specs) are the aspect ratio. So with 1200 Javascript will be faster! (can be even super smaller) and we will get whatever height and then we can calculate the ratio. But yes, FULL can work, just slower. Does that make sense?

@DiegoPino I understand, I was thinking into absolute values while we only need ratio, am I right?

@DiegoPino DiegoPino merged commit 9e8f6f9 into 8.x-1.0-beta3 Mar 31, 2020
@DiegoPino
Copy link
Member Author

@giancarlobi thanks so much for reviewing. Will open a new issue to handle IIIF Presentation Manifests without a service and the JS driven height/width to make the CANVAS/HTML/JAVASCRIPT/CSS God happy! 👍 Gracias!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request external bug It is not us, it is them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make IABookreader respect the IIIF manifest
2 participants