Fixes to support Ventana DP200 slides

[lukenovak]

This PR includes various fixes to support slides scanned by the Ventana DP200 scanner. These changes are:

• Changes to the tile struct and tile placement in openslide-vendor-ventana.c:
  • X and Y offset for each tile is calculated and then accumulated as the tiles are placed, instead of calculated an average x and y offset by weight and using that as the offset (by @maxbogue)
  • Tile ordering is flipped horizontally for DP200 slides since they use tile joint direction LEFT instead of RIGHT, starting in the bottom right and going left, up, right, up, etc instead of starting in the bottom left and starting going to the right. (by @maxbogue)
• Empty tiles are now detected and filled with transparent pixels. (by @maxbogue)
  • New private API function, _openslide_slice_alloc0(gsize len) to allocate this empty space for sparse AOIs (by @lukenovak)
• Set background color from iScan property if present. (by @maxbogue)
• Fixes to ensure that slide widths and AOI placements matches with Ventana's viewer (by @lukenovak)
  • Additional x offset to account for the cumulative effect of tile overlaps, removing incorrect sparse pixels between AOIs when prior areas had overlap.
  • Ensuring that all slide widths are multiples of 16px to mirror slide widths calculated by Ventana's viewer (Accomplished by rounding slide widths up to the nearest multiple of 16).
• Added test to ensure that the newly added DP200 test slide can be opened correctly by (@lukenovak)

[github-actions]

DCO signed off ✔️

All commits have been signed off. You have certified to the terms of the Developer Certificate of Origin, version 1.1. In particular, you certify that this contribution has not been developed using information obtained under a non-disclosure agreement or other license terms that forbid you from contributing it under the GNU Lesser General Public License, version 2.1.

[bgilbert]

Thanks for the PR! Since you'll need to rebase anyway to add the missing signoffs, please rebase onto current main and move the merge conflict fixes directly into the individual commits. That'll help keep the Git history clean.

[lukenovak]

Will do, thanks.

[bgilbert]

Thanks again for the PR! This is a great contribution that will improve the Ventana driver.

I've added some initial high-level comments, but some restructuring of the PR will be needed before I can do a full review. More details below.

[bgilbert]

If this was based on the Philips driver, note that b8fbb8d switched to not caching transparent tiles. We should do the same here, which would also obviate the box reshuffling at the end of this commit series.

[lukenovak]

I played around with this a bit when you initially left this comment, and the quick change I made produced some invalid JPEG tiles. On testing again, the change in the latest commit seems to work properly, so we're no longer allocating those empty tiles. This also might negate the need for the new API function (though its introduction might not hurt, it adds some new code that might not be necessary anymore).

[bgilbert]

There's a principle in web programming that websites should detect features rather than browsers. A similar thing is true here. Rather than making assumptions about the behavior of future scanner models, we should detect the characteristics of the slide file that require us to behave differently, and adjust our behavior based on that. If there are multiple behavioral differences, we should detect and respond to each one separately.

[lukenovak]

Okay so after some further research, it looks like the public file specification encourages using the scanner name field to determine how software should interpret the file (noted on pages 3 and 7). I have found a difference between metadata fields in DP 200 and older slides that I could also use to identify these differences (the presence of TileJointInfo is a defining characteristic of DP 200 slides as far as I can tell), but I'm curious what direction we'd like to head in given the file format mentions the currently implemented design.

[lukenovak]

Thanks for the initial look over! l will reorganize the commit history to address some of the early feedback. After that, each commit lines up with a concrete addition without any reversals in the commit history.

[lukenovak]

Okay- the commit history should make more sense now. I still need to address some of the comments but the commits have been split apart according to added functionality and the tests are now passing.

[Austinkf]

Is this pull request going to be merged into the main branch that is used by LibVips? I'm hoping this fixes the issue with the bif format bad direction. This probably isnt the correct place for this comment, but I'm not sure where else to ask this.

[lukenovak]

Not sure exactly what openslide ships with libvips by default (and the formal release schedule for openslide would be a question for Ben) but I will note that you can actually use this branch with libvips already if you build openslide locally before building libvips. I've done plenty of manual testing that way because libvips just calls down to openslide for these file formats.

[Austinkf]

That would be very good to try. I have a ubuntu instance set up and was actually looking to build the openslide repo, but, as I'm not super familiar with building repos and using them, I wasn't sure if what I was doing was correct to test it. I followed the instructions for building the repo on the main info page for openslide, but I'm not sure how to build the libvips repo as the same commands didn't work within that repo. I use libvips pretty much exclusively on windows through the command line for the dzsave function.

I would love to test that libvips functionality in the cli to see if the problem is solved. Would it be too much to ask how you usually build and test it? I am able to build the openslide repo, although I'm not sure how to pull this specific branch, so the only part I guess I need to know is whether with the libvips repo built can I use it as a cli?

[lukenovak]

Can't speak for windows, but I build libvips on MacOS and Ubuntu by running the following:
./configure
make
make install
ldconfig
after installing Openslide as specified in the README. From there I use pyvips to test things out in a python shell.

[lukenovak]

@bgilbert Going to be patching up some of the design-oriented comments over the next week or so. Regarding the review process and commits going forward: should I continue to alter the git history for organization and force push, or make the fixes as their own commits and squash things at the end of review?

[lukenovak]

Also worth mentioning, so that I can tie the two together: this MR solves issue #234

[Austinkf]

@lukenovak Overall the changes have been working well, but I am having an error in one bif file (from within vips). I'm not sure if it could be a read issue with the format or what, but its only happening on this one image.

I get the error using dzsave with vips: (vips.exe:7752): VIPS-WARNING **: 15:24:33.037: error in tile 30208 x 79872.

Is there a way to check if this is an openslide issue or a vips one?

[maxbogue]

@lukenovak Overall the changes have been working well, but I am having an error in one bif file (from within vips). I'm not sure if it could be a read issue with the format or what, but its only happening on this one image.

I get the error using dzsave with vips: (vips.exe:7752): VIPS-WARNING **: 15:24:33.037: error in tile 30208 x 79872.

Is there a way to check if this is an openslide issue or a vips one?

Hey @Austinkf! During the development of the original DP200 support in this PR we ran the code on several thousand BIF images and I spent a week or two trying to debug an error just like this that occurred on a higher percentage of them than expected. After substantial investigation we determined that the failing images actually contained corrupt tiles and the partner that provided them to us likely had a problematic scanner. Based on that experience I would suggest that your one image is likely corrupt.

dzsave has the fun property of needing to read every tile in the image, and therefore it will find any corrupt tiles like this. Software that renders the slide from the BIF directly will likely appear to work with this file but crash if you actually hit that specific tile.

[lukenovak]

@bgilbert I think I've either addressed or responded to all the comments you've left so far. Not sure how I should proceed from here but let me know what I should do to move this forward.

[bgilbert]

Thanks for the fixups! I'll need to circle back and do a more detailed review, but unfortunately I won't have a chance to do that until next month.

Regarding the review process and commits going forward: should I continue to alter the git history for organization and force push, or make the fixes as their own commits and squash things at the end of review?

I generally prefer a rewrite + force-push flow. That allows complex changes to have separate logical commits in the Git history, not just in the GitHub PR.

[Austinkf]

@maxbogue Is there a way to have the reader to continue to read tiles when there is a corrupted tile, or to have it return black or white tile when a tile is corrupted?

[lukenovak]

Thanks @bgilbert - I might have time to reorder the new commit to fit in with the history this weekend

[lukenovak]

Sorry for the delay here, got a bit lost in things headed into the holidays. But, I've just squashed the transparency MR and its reversion together, which should leave the commit history in a good state.

[Tato14]

Hi! Tried to install the the branch dp200-support-with-tests from Path-AI. The installation runs without issues but I am still unable to process DP200.

When I do:

joan@Alienware:~$ openslide-show-properties /home/joan/Baixades/slides/18B004098\ 1\ T2\ 1\ 1_1633523201205.bif 
openslide-show-properties: /home/joan/Baixades/slides/18B004098 1 T2 1 1_1633523201205.bif: Bad direction attribute "LEFT"

I am using the correct branch? Thanks

[lukenovak]

@Tato14 I'm not sure how you installed this particular branch, but we've been using it to read LEFT join slides for months without issue. The new test on the "Ventana-1.bif" slide also uses a slide with LEFT joins to test the new functionality. Are you sure that your openslide-show-properties is using the correct build?

[lukenovak]

Hi @bgilbert, any chance you have some time in the near future to look over the updated changes in this PR?

[Tato14]

@Tato14 I'm not sure how you installed this particular branch, but we've been using it to read LEFT join slides for months without issue. The new test on the "Ventana-1.bif" slide also uses a slide with LEFT joins to test the new functionality. Are you sure that your openslide-show-properties is using the correct build?

Yeap, seems that I should first uninstall and then install the branch. Now worked as expected. Thanks!

[lukenovak]

@bgilbert While there still aren't any conflicts with the main branch, do you want me to rebase to keep things up to date with the DICOM merge? Was thinking about doing that anyway.

[bgilbert]

Yeah, a rebase wouldn't hurt. You might also want to rebase on top of #435 when it lands. It looks like it may not cause conflicts here, but nevertheless it's a somewhat intrusive change.

Thanks for your patience with this PR. I haven't forgotten about it and do plan to get back to it, though it may take me a bit longer yet.

[lukenovak]

Okay @bgilbert, this should be properly rebased to the commit that went in yesterday.

[lukenovak]

@bgilbert just updated this with an additional commit that fixes a bug we noticed with some slides that had the iScan element located at a different place in the XML.

[lukenovak]

@bgilbert just pushed a fix for the broken tests- forgot to account for errors thrown when we can't get xml data at a directory. Should be good for a look over now.