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.

Sign up for GitHub

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

Jump to bottom

Libisyntax integration #437

Open

alex-virodov wants to merge 11 commits into openslide:main from innovationcore:libisyntax-integration

+650 −2

alex-virodov commented Apr 21, 2023

This PR enables support of the .isyntax WSI format using amspath/libisyntax.

I am still using a submodule to include libisyntax, so please do git submodule update --init after checking out the PR branch.

Tested with:

~/openslide-isyntax-integration/builddir$ ./tools/openslide-write-png /home/user/openslide/testslide.isyntax 0 0 5 1000 1000 /home/user/openslide/output.png

Test slide used is from OpenPhi project (MIT license): https://zenodo.org/record/5037046/#.ZAn17IDMLJU

alex-virodov added 2 commits

April 21, 2023 15:13


          Added libisyntax submodule

086ff16

Signed-off-by: Alexandr Virodov <alexandr.virodov@uky.edu>


          Implementation of openslide-vendor-philips-isyntax.c using libisyntax…

efd9118

… C api.

Signed-off-by: Alexandr Virodov <alexandr.virodov@uky.edu>

github-actions bot commented Apr 21, 2023 •

edited

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 reviewed

View reviewed changes

Member

bgilbert left a comment

This is great, thanks for the PR! There's been an open RFE #145 for iSyntax support since 2014, and I'm looking forward to getting this in.

.gitmodules

		@@ -0,0 +1,3 @@
		[submodule "src/libisyntax"]

Member

bgilbert Apr 21, 2023

For the record, we'll want to switch to a Meson wrap before this lands.

src/openslide-vendor-philips-isyntax.c Outdated Show resolved Hide resolved

src/meson.build Show resolved Hide resolved

src/openslide-private.h Show resolved Hide resolved

src/openslide-vendor-philips-isyntax.c Outdated Show resolved Hide resolved

src/openslide-vendor-philips-isyntax.c Outdated Show resolved Hide resolved

src/openslide-vendor-philips-isyntax.c Outdated

+                  LOG_VAR("%d", (int)open_result);
+                  // LOG_VAR("%d", data->isyntax->image_count); // TODO(avirodov): getter.
+                  if (open_result != LIBISYNTAX_OK) {
+                      free(data);

Member

bgilbert Apr 21, 2023

This will cause a double free in openslide_close(), since we've already attached data to the openslide_t. Generally we don't attach anything to the openslide_t until we know the open will succeed.

Author

alex-virodov Apr 26, 2023

Done I think, please take a look at the flow again if you don't mind. Will keep open.

src/openslide-vendor-philips-isyntax.c Outdated Show resolved Hide resolved

src/openslide-vendor-philips-isyntax.c Outdated Show resolved Hide resolved

src/openslide-vendor-philips-isyntax.c Outdated

+                  philips_isyntax_t* data = malloc(sizeof(philips_isyntax_t));
+                  osr->data = data;
+                  isyntax_error_t open_result = libisyntax_open(filename, /*is_init_allocators=*/0, &data->isyntax);

Member

bgilbert Apr 21, 2023

Should use the OpenSlide VFS, rather than having libisyntax do its own I/O.

Author

alex-virodov Apr 27, 2023

I agree, but this will take a while to implement in libisyntax. Is this a requirement for Meson work/PR submission? If so, please let me know so I can prioritize.

Member

bgilbert May 10, 2023

It's not a prerequisite for Meson work, or for work to proceed on this PR. Ideally the PR wouldn't merge before VFS support is available, but I'd consider leaving VFS support to a followup PR if it's impractical to do at present. Eventually the test suite will be refactored in a way that will require the VFS, but that hasn't happened yet.

bgilbert reviewed

View reviewed changes

src/openslide-vendor-philips-isyntax.c Outdated

+                                                            &cache_entry);
+                  if (!tiledata) {
+                      int scale = libisyntax_level_get_scale(pi_level->isyntax_level);
+                      ASSERT_OK(libisyntax_tile_read(isyntax, data->cache->cache, scale, tile_col, tile_row, &tiledata));

Member

bgilbert Apr 23, 2023

With the current libisyntax implementation, we need to byteswap the result on big-endian systems. (Neither libisyntax nor OpenSlide are currently being tested on such systems, though, so it's possible that there are more fundamental issues as well.)

Author

alex-virodov Apr 27, 2023

Is there anything actionable I can do here? Just checking. Please unresolve if there is.

Maybe it will be somewhat addressed by having an output_format=RGBA or BGRA flag that we are discussing in libisyntax thread. That flag could include endianness later on (e.g. ABGR if needed).

Member

bgilbert May 10, 2023

Eventually big-endian systems should be handled either in libisyntax or here. libisyntax is probably the correct place for it, so hopefully OpenSlide won't have to do anything. I'll unresolve until we have a solid answer here.

I've acquired access to a s390x machine for big-endian testing. I can help run tests when this work gets further along.

alex-virodov added 2 commits

April 26, 2023 16:20


          Format changes only. Reformat file to 2 spaces, make function paramet…

6544f76

…ers on separate lines.

Signed-off-by: Alexandr Virodov <alexandr.virodov@uky.edu>


          First batch of changes to address comments from the PR.

fe26380

Signed-off-by: Alexandr Virodov <alexandr.virodov@uky.edu>

Author

alex-virodov commented Apr 26, 2023

Thank you for the review. I addressed the more editorial comments, I will continue with the logic-affecting ones next.


          Better error handling.

84004be

Signed-off-by: Alexandr Virodov <alexandr.virodov@uky.edu>

alex-virodov mentioned this pull request

Consider avoiding SATURATE operator when not needed #440

Open

alex-virodov added 2 commits

April 27, 2023 12:12


          Disable use of CAIRO_OPERATOR_OVER

dd981b3

Signed-off-by: Alexandr Virodov <alexandr.virodov@uky.edu>


          Added _openslide_hash_disable(quickhash1).

5342edd

Signed-off-by: Alexandr Virodov <alexandr.virodov@uky.edu>

Author

alex-virodov commented Apr 27, 2023

@bgilbert I believe I addressed all the comments. Please take a look again when you have a chance.

alex-virodov mentioned this pull request

Add pixel_format to libisyntax api. libisyntax_tile_read() now takes an external buffer. amspath/libisyntax#24

Merged

alex-virodov mentioned this pull request

Add a tool to query and write associated images. #445

Closed

alex-virodov added 4 commits

May 9, 2023 15:09


          Update openslide-vendor-philips-isyntax to latest libisyntax.

3e79e48

Signed-off-by: Alexandr Virodov <alexandr.virodov@uky.edu>


          Add a tool to query and write associated images.

a4d8160

Signed-off-by: Alexandr Virodov <alexandr.virodov@uky.edu>


          Fix associated image dimensions printing.

9ea6b76

Signed-off-by: Alexandr Virodov <alexandr.virodov@uky.edu>


          Expose isyntax associated images in OpenSlide.

530a4a3

Signed-off-by: Alexandr Virodov <alexandr.virodov@uky.edu>

Member

bgilbert commented May 10, 2023

Thanks for the updates! I'll circle back to this when I can.

Nathan-Thompson commented Jul 26, 2023

Hey @bgilbert @alex-virodov! Is there anything I can do to help move this PR forward? Thanks!

Member

bgilbert commented Jul 26, 2023

I still very much want to get back to this. Unfortunately, I won't be able to until at least September, but I'm hoping to dedicate some time to it then.

Author

alex-virodov commented Jul 26, 2023

TLDR: I'm no longer working on this, but I would be happy to answer questions and helping it get done.

Hello. We originally discussed this PR to be a proof of concept, and a proper integration would involve a separate libisyntax linux package. Since I am no longer working on this project, I won't be able to make that happen. Maybe @Falcury would want to pick that part up. Or @bgilbert approves and we add libisyntax as source/submodule (though this has downsides). Or any other acceptable short path to get it in (I listed the ones we discussed).

But in any case I don't think this PR will go in as-is. It currently uses libisyntax as a git submodule, and keeping it that way may need changes from the build system. Also some comments remain unaddressed (e.g. metadata implementation is missing), so there's some more work to be done, or to be deferred.

Member

bgilbert commented Jul 26, 2023

@alex-virodov Thanks for the update, and for your work on this! If @Falcury would like to take over this PR, he's welcome to of course, and otherwise I can plan to work on it myself. I'm still planning to get Meson support into libisyntax so we can wrap it here as a subproject (pending distro packages), similar to what we're doing with libdicom.

I appreciate everyone's patience with the longer review cycle on this one. The DICOM work and release stabilization had to take priority unfortunately.

Falcury commented Oct 8, 2023

I could give it a go to pick this up, however my time available to work on this PR (and libisyntax) wil be very limited until early next year. In any case, when I have time I will try to address some of the to-do items.

Member

bgilbert commented May 15, 2024

Initial libisyntax Meson port in amspath/libisyntax#40.

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