Add read_region function

[jonasteuwen]

This is a draft for a read_region function. Currently the offsets are not yet taken into account, but that should be easy enough. Do I understand they are floats? Then we'd need to implement interpolation. I see indeed the white margin in other levels, but perhaps they also round it somehow.

Anyway, this is a draft. Let me know what you think and I can proceed with it. I think we need a function that checks if the tile is actually empty, so we don't need to go and fetch a huge amount of them.

[jonasteuwen]

@Falcury: I added a way to calculate the offset. It's slightly awkward because I need to use the mpp value, and there is an mpp_known flag, so in essence, that shouldn't be always available. The offset_in_pixels is likely defined in the highest level? Could we do use that dividing it by 2*level or something like that?

What else:

• Do not fetch empty tiles, but pass an empty tile from the function itself (use alpha channel)
• How do we compute the actual size of a level?

[Falcury]

Thank you! I'll test it as soon as I can. I'll ask @alex-virodov to review as well.

I think this could be a good starting point. I think the performance might be somewhat suboptimal (we're not yet pipelining it efficiently), but that's a broader problem that needs tackling.

Currently the offsets are not yet taken into account, but that should be easy enough. Do I understand they are floats?

Yes, when I wrote that code, I calculated the offset in microns (floating point) because Slidescape mostly keeps track of its coordinates in microns instead of pixels. However, there is no need to do this for libisyntax, it would be best to calculate the offset in pixels. I think the amount should be ((3 >> (scale-1)) - 2) for scale >= 1 (see here).

I think it's a good idea to have bgra_to_rgba() available as a helper function, however I was actually thinking more along the lines of adding an RGBA variant of convert_ycocg_to_bgra_block() and then have isyntax_load_tile() return RGBA pixels directly if so desired. The bool decode_rgb parameter could be maybe changed to an enum(-like) parameter to specify the desired pixel format.

[jonasteuwen]

Yes, sure, it’s a draft. Feedback how you want it implemented is useful! Good idea to change the mode with an enum. I can go and draft something, removing the function I made. Would that help?

I will replace the offset thing, but I think your formula doesn’t work (at least I don't understand yet how to use it). For scale = 7 it gives 382, and it is not clear to me what it should be. Do you know? Experimentally (that’s also what the computation gives using the mpp) I found 11 to work for level 7.

[Falcury]

I will replace the offset thing, but I think your formula doesn’t work (at least I don't understand yet how to use it). For scale = 7 it gives 382, and it is not clear to me what it should be. Do you know? Experimentally (that’s also what the computation gives using the mpp) I found 11 to work for level 7.

For level 7 I think it should be 190 == ((3 << (7-1)) - 2), in terms of pixels at the base level (level 0). Sorry, it should be shift left, not right (I messed up in my comment line there). To be honest for me this is also somewhat 'trial and error'!

We can change the isyntax_level_t.origin_offset_in_pixels field to be an integer instead of a float, then you can just use that directly.

[jonasteuwen]

I found it I think (and you are right)! It's int32_t offset = ((PER_LEVEL_PADDING << num_levels) - PER_LEVEL_PADDING) >> level;. Followed the documentation (should have done that immediately) and tested it with an isyntax image I have.

So basically, the value you meant. Would indeed good to change that to an integer. I couldn't immediately see the relationship between the value you refer to and the offset value above, but should be trivial.

Can we have a .width and .height property on the levels? I might be missing something, but it doesn't seem to be conveniently accessible. Openslide has openslide_get_level_dimensions, shall we implement one like this (It's not just num_tiles * tile_size is it?)

Update: I have added the height and width of the level as properties of the level, and added specific getters. Let me know if you think is the right location. If so perhaps we can merge that in a different PR keeping this one open until we converge on a proper design.

[jonasteuwen]

I've added a tool that will compile when LibTIFF is available on the system that's able to convert to tiff #6. It's reasonably fast (<15 minutes) on my M1 laptop for a normal size H&E.

One thing I encountered is that the offsets for the levels are only set for levels > 0. I need to verify if that is actually true (I don't think so, but need to double check!)

[jonasteuwen]

I added some of the changes I needed to do to make such a read_region to a different PR #10. I think we can probably agree faster on the implementation of these changes.

[jonasteuwen]

Converting an 1.2GB isyntax file to 1.89GB jpeg bigtiff (quality=80) gives visually good quality results. Conversion takes ±20 minutes. If performance improvements can be made to read_region that would be great :-)

[Falcury]

Converting an 1.2GB isyntax file to 1.89GB jpeg bigtiff (quality=80) gives visually good quality results. Conversion takes ±20 minutes. If performance improvements can be made to read_region that would be great :-)

I would use the PHOTOMETRIC_YCBCR colorspace as default instead of PHOTOMETRIC_RGB, because the chroma subsampling will help to reduce the filesize substantially without significant loss in quality (as far as my eye can tell).

[jonasteuwen]

That’s smart. I’ll test it as well.

[jonasteuwen]

I'm closing this for now, I will make a new PR that is up-to-date with the other changes.

[jonasteuwen]

I'm closing this for a new PR.