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

IIIF Support #2098

Merged
merged 14 commits into from
Mar 18, 2020
Merged

IIIF Support #2098

merged 14 commits into from
Mar 18, 2020

Conversation

edsilv
Copy link
Contributor

@edsilv edsilv commented Feb 23, 2020

Starting a PR to include libvips' new IIIF support as per: libvips/libvips#1465 (comment)

However, with the changes included in this PR as per #1335 (comment) it doesn't generate the tiles and info.json...

I thought passing the correct layout should now work? e.g.

    sharp("test.jpg")
    .jpeg()
    .tile({
      size: 512,
      layout: "iiif"
    })
    .toFile("info.json");

As far as I understand it, the layout should be getting passed here?

https://github.com/lovell/sharp/blob/master/src/pipeline.cc#L966

@coveralls
Copy link

coveralls commented Feb 23, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 1305bdb on edsilv:iiif into cf39fc4 on lovell:master.

@edsilv edsilv requested a review from lovell February 24, 2020 08:55
@lovell
Copy link
Owner

lovell commented Feb 24, 2020

Hi Edward, thank you for the PR, I think we'll need to change this logic too:

sharp/src/pipeline.cc

Lines 1380 to 1387 in d5ecc53

std::string tileLayout = AttrAsStr(options, "tileLayout");
if (tileLayout == "google") {
baton->tileLayout = VIPS_FOREIGN_DZ_LAYOUT_GOOGLE;
} else if (tileLayout == "zoomify") {
baton->tileLayout = VIPS_FOREIGN_DZ_LAYOUT_ZOOMIFY;
} else {
baton->tileLayout = VIPS_FOREIGN_DZ_LAYOUT_DZ;
}

Rather than if-else-if-else... we should probably change this whole block to something simpler like the following (untested):

baton->tileLayout = static_cast<VipsForeignDzLayout>(
    vips_enum_from_nick(nullptr, VIPS_TYPE_FOREIGN_DZ_LAYOUT,
    sharp::AttrAsStr(options, "tileLayout").data()));

If you're able to add a test or two as well, that would be perfect.

@lovell
Copy link
Owner

lovell commented Feb 24, 2020

Please can you target the yield branch to ensure your changes are compatible with the forthcoming N-API support.

@edsilv edsilv changed the base branch from master to yield February 24, 2020 14:31
@lovell
Copy link
Owner

lovell commented Mar 6, 2020

I simplified the tile option parsing in commit 51b1432 on the yield branch, so if you rebase against upstream/yield you should hopefully start to see this new option work.

@lovell lovell changed the base branch from yield to master March 7, 2020 14:53
@lovell
Copy link
Owner

lovell commented Mar 7, 2020

v0.25.0 is now available so I've moved this PR back to the master branch.

@edsilv
Copy link
Contributor Author

edsilv commented Mar 8, 2020

Hi, nice! Do you need anything further from me? I was a bit out of my depth on the c++ stuff and wound up needing dynamic tiling using serverless-iiif (also libvips-based). Looking forward to getting IIIF tiling in Sharp and updating biiif!

@lovell
Copy link
Owner

lovell commented Mar 8, 2020

Thanks for the update. Please can you rebase against the latest upstream/master branch and you should see the proposed changes in the PR start to work. If you're able to add a unit test or two as well, that would be great, then we can get this merged and out in the next release.

@edsilv
Copy link
Contributor Author

edsilv commented Mar 8, 2020

I've pushed the beginnings of a unit test and tried the outputted tiles here: https://repl.it/@edsilv/osd-test

You can see that some tiles are missing when fully zoomed out.

I've also had to add "preferredFormats": [ "jpeg" ] to the generated info.json as otherwise OSD will look for files ending .jpg by default. To get around that you probably would have to do some gymnastics with IIIFTileSource, but really it would be better if it just worked out of the box with no customisation.

Any chance we could change the default outputted tile format to .jpg not .jpeg?

@lovell
Copy link
Owner

lovell commented Mar 9, 2020

The JPEG suffix logic at https://github.com/lovell/sharp/blob/master/src/pipeline.cc#L936 will need updating to also include VIPS_FOREIGN_DZ_LAYOUT_IIIF in the list of layouts for which .jpg is used.

@lovell
Copy link
Owner

lovell commented Mar 14, 2020

I had a quick look at this and inverted the JPEG extension logic via commit cf39fc4 to make this a bit simpler, which should hopefully allow you to continue with this PR.

@edsilv
Copy link
Contributor Author

edsilv commented Mar 15, 2020

Have tested that locally and the tiles are now .jpg and the generated info.json works without any modifications. However, I'm getting a timeout error when running npm test, even if I set the timeout to 5 mins: IIIF layout:Error: Timeout of 300000ms exceeded.
Could this be why I'm seeing missing tiles when fully zoomed out?

@lovell
Copy link
Owner

lovell commented Mar 15, 2020

The "IIIF layout" test is failing as the call to done() is commented out (as are the assertions).

https://travis-ci.org/github/lovell/sharp/jobs/662551580#L1596

@edsilv
Copy link
Contributor Author

edsilv commented Mar 15, 2020

d'oh - sorry, it was a late night!
That's fixed, but still seeing the bug in OSD: https://youtu.be/69oYSDI22ik

@lovell
Copy link
Owner

lovell commented Mar 15, 2020

Could libvips/libvips#1530 and/or libvips/libvips#1472 relate to this problem?

@edsilv
Copy link
Contributor Author

edsilv commented Mar 15, 2020

Some tiles are not available:
image

@edsilv
Copy link
Contributor Author

edsilv commented Mar 15, 2020

(changing scalefactors to scaleFactors didn't make a difference)

@edsilv
Copy link
Contributor Author

edsilv commented Mar 15, 2020

The input image 2569067123_aca715a2ee_o.jpg is 2725px in width. The first failing tile in the image above has an xywh region of 2048,0,678,2048. x:2048 + w:678 = 2726px, 1px more than the full width of the image. Could this be the problem? Libvips is failing to cut that tile because its bounds exceed the width of the image?

@lovell
Copy link
Owner

lovell commented Mar 16, 2020

Yes, I think this might be a libvips thing. Are you able to reproduce it via the command line?

vips dzsave input output --layout iiif

@edsilv
Copy link
Contributor Author

edsilv commented Mar 17, 2020

I downloaded v8.9.1 and am getting the same issue via the command line. I suppose we should flag this with libvips? The explanation above still applies.

@lovell
Copy link
Owner

lovell commented Mar 17, 2020

Yes please, if you could open an upstream issue in libvips that would be great.

It doesn't stop this PR so when you find a moment, please can you uncomment that lovely unit test code, get the assertions passing then we're good to merge, thank you.

@edsilv
Copy link
Contributor Author

edsilv commented Mar 17, 2020

Have fixed the tests and left a comment here: libvips/libvips#1472 (comment)

Copy link
Owner

@lovell lovell left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, a couple of comments inline then this is good to merge.

package.json Outdated Show resolved Hide resolved
test/unit/tile.js Outdated Show resolved Hide resolved
@lovell lovell merged commit eff36dc into lovell:master Mar 18, 2020
@lovell lovell added this to the v0.25.2 milestone Mar 18, 2020
lovell added a commit that referenced this pull request Mar 18, 2020
@lovell
Copy link
Owner

lovell commented Mar 18, 2020

Marvellous, thank you Edward, this will be in v0.25.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants