Skip to content

8-bit image support #16

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

Closed
wants to merge 5 commits into from
Closed

Conversation

NicoKiaru
Copy link

@NicoKiaru NicoKiaru commented Jun 30, 2022

I've added an example with few dependencies. We can see the converter scaling issue with 8 bits cached images.

I'll try to put some more info soon... will be back on Wednesday.

image

@NicoKiaru
Copy link
Author

I finally found a bit of time to advance this PR.

Now cached and non-cached 8-bits images are ok. However there is still one issue. It looks like the current structure does not allow yet to combine 8-bit and 16-bit cached images. I'll work on that.

@NicoKiaru NicoKiaru changed the title first attempts at displaying 8-bits cached images 8-bits image support Apr 30, 2025
@NicoKiaru NicoKiaru changed the title 8-bits image support 8-bit image support Apr 30, 2025
…s, and of a mix between 8 bits and 16 bits volatile images.

The mix doesn't work.
@NicoKiaru
Copy link
Author

Hi @tpietzsch! I'm trying to look at being able to mix 8 bit and 16 bit blocked images.

Currently there's a single CacheSpec object per volumerenderer, and it should be specified with a single internal format:

cacheSpec = new CacheSpec( R16, cacheBlockSize );

So, I've been trying a little to release this constraint, but haven't been successful so far.

1 - Do you think it's possible to do that without a major structure change ?

2 - If yes, could you give me some pointers ? In particular, I'd like to know if there's a chance for this to work without touching any of the shader part.

@NicoKiaru
Copy link
Author

Alright, I think I won't be able to reach the previous goal - too many things are interconnected and I clearly grasp only a little percentage of how bvv works.

I tried a quick chat with claude: https://claude.ai/share/a1e77a42-f456-48c7-a383-0ee92c74d967

I think it reflects more or less what I tried to do and where I'm stuck.

One simple way forward could be to set the pixel type of bvv on startup and if a chunked cached source of different bit depth is added later on, then an error is thrown.

@ekatrukha
Copy link
Contributor

Hello Nicolas and Tobias,

I am surprised with the Claude conversation, seems very helpful.

I would also like to have this feature implemented. In addition, I think, this change should lead to the situation where each volume is rendered in a separate shader.

Right now all volumes are rendered in one "global" one. So for large number of volumes (sources), like 20 to 100, sometimes BVV hits the limit on possible OpenGL variables per shader (depending on a card).

I have some applications, where I want to see more then 100 volumes, so that is not possible yet.

And it would be nice if float supported too.

I think I can try to split Cache per source (what Claude said :) and submit a draft version. Not sure when this will happen.

@NicoKiaru
Copy link
Author

NicoKiaru commented May 1, 2025

Just FYI, I tried an approach consisting of making a cache for 8 bits data and a cache for 16 bits data (branch 8bits-dupcachespec in my remote):

NicoKiaru/bigvolumeviewer-core@NicoKiaru:bigvolumeviewer-core:8-bits...NicoKiaru:bigvolumeviewer-core:8bits-dupcachespec

But it ultimately fails because, AFAIU, the shader indeed needs to be changed. (It can work to some extent, but the texture is wrongly interpreted and it ultimatelly makes a hard jvm crash).

image

@ekatrukha
Copy link
Contributor

Hello @NicoKiaru
So I've made a version where 8-bit (green) works with 16-bit (red)
image

If you add me as a collaborator, I can push it to this branch.
Or should I try to make a separate PR to your fork? Whatever is more convenient.

@ekatrukha
Copy link
Contributor

ekatrukha commented May 2, 2025

Basically, instead of having cache details as global variables in the main shader, I've moved them into "per volume" shader segment, so that did the trick.

Even if it works, I do not like it that much, since (as I've mentioned before), it increases the number of variables in the "global multi volume" shader. Which in the end can lead to OpenGL stack overflow and errors, when the number of sources is high .
BTW, the "global" final volume shader code can be printed by uncommenting these lines.

In principle, we can generate many shaders, i.e. each per volume, and make BVV render them somewhere here.
The problem with it is that when generating Max proj, having all volumes in one shader allows this to happen:

		vec4 v = vec4(0);
		for (int i = 0; i < numSteps; ++i, step += nw + step * fwnw)
		{
			vec4 wpos = mix(wfront, wback, step);


			if (vis_x_7_x_)
			{
				v = max(v, convert_x_15_x_(sampleVolume_x_14_x_(wpos)));
			}
			if (vis_x_18_x_)
			{
				v = max(v, convert_x_26_x_(sampleVolume_x_25_x_(wpos)));
			}
		}

As I see it, this can be solved by introducing an extra "volume render" texture that will be shared by individual shaders. Then we would need to do extra pass to blend it with "scene" texture.

@ekatrukha
Copy link
Contributor

So this is the fork/branch with working Example08.

@NicoKiaru
Copy link
Author

@ekatrukha that's awesome, I invited you to my repo

@ekatrukha
Copy link
Contributor

Thanks, I've pushed changes now to 8bits-dupcachespec, I think you can rearrange things as you see better.

Should we also add float, while we are at it?

@NicoKiaru
Copy link
Author

NicoKiaru commented May 2, 2025

Just a quick heads-up, I had this issue with intellij: https://stackoverflow.com/questions/6047627/intellij-diagnosing-crash-problem

It's only for this repo... Anyway, I increased the RAM for intellij and it was fixed.

Should we also add float, while we are at it?

That would be nice! Maybe ARGB as well ? It's a bit more niche so I don't know.

I'll try to spare a bit of time next week to give it a shot.

Now I just noticed a small problem, I guess there's a need for re-indexing when the visibility is changed:

PbVisibility.mp4

@ekatrukha
Copy link
Contributor

ekatrukha commented May 2, 2025

I am using Eclipse, so it's a different story.

Good catch on the visibility trigger!
Yeah, I am not sure what is going on and how to do reindexing, that is the cache level details I am not familiar with. You and @tpietzsch would be experts on that.

I guess one needs to look at what happens during visibility off.
I think right now, the source is fully unloaded from GPU, which is not really necessary.

@NicoKiaru
Copy link
Author

NicoKiaru commented May 9, 2025

Side quest: I managed to fix the shader to work with planes (volumes where one of the dimension == 1):

ThinVolumes.mp4

Thanks @ekatrukha for your indication regarding the shader, that was super helpful.

What's crazy is that it's Claude who fixed the issue completely on its own. I printed the shader, explained the issue, and it identified the issue directly and suggested several fixes... This is nuts.

I'm still stuck on the visibility issue, but there's progress

@ekatrukha
Copy link
Contributor

Was it with sourcemin/sourcemax? There is a half pixel difference between multires and simplestack rendering, I've fixed it in bvv-playground, need to submit a PR.

@tpietzsch
Copy link
Member

I looked at ekatrukha/bigvolumeviewer-core:8and16bit, fixed whatever problems I saw, and cleaned up a bit.

I pushed my revised version to bigdataviewer/bigvolumeviewer-core:8and16bit.

@ekatrukha @NicoKiaru: Could you have a look and maybe try it in your respective context?

If everything is fine, I would merge it...


The only remaining issue that I would leave for future work is the following: With these changes, the uniforms

uniform sampler3D volumeCache;
uniform vec3 blockSize;
uniform vec3 paddedBlockSize;
uniform vec3 cachePadOffset;
uniform vec3 cacheSize;

are now replicated for each volume. In principle it would be better to just have them replicated for each volume cache (i.e., once for 8 bit and once for 16 bit) and reuse them in the sample_volume_blocks segment (binding each instantiation to the appropriate volume cache segment).
But we will in practice hit the texture unit limit much earlier than the uniform limit, so I'm ok with the inefficiency for now...

@NicoKiaru
Copy link
Author

@tpietzsch thanks a lot! I'll give it a try

@NicoKiaru
Copy link
Author

Hi @tpietzsch,

Unfortunately the Example09 and Example10 are giving me errors:

https://github.com/NicoKiaru/bigvolumeviewer-core/tree/more-examples/src/test/java/bvv/vistools/examples

It's because I send to bvv Volatile pixels types and not the Non Volatile types. I tried to fix it but couldn't because some part of the code explicitely require a UnsignedShortByte casting and do not work with VolatileUnsignedShortType (or the byte equivalent).

Just to explain a bit what's in the example 9: I wanted to:

  • make easily several sources that take time to be computed (there's a sleep in the block computation)
  • show a mix of 8 bits and 16 bits lazy loaded images
  • check that the sources are displayed correctly when their visibility are changed

In Example10, I was testing the display of a single plane with various thickness and check how they look like.

(I pushed my branch where these demos are working here: https://github.com/NicoKiaru/bigvolumeviewer-core/tree/8bits-dupcachespec but I don't understand really well the design so it's probably messy)

I hope there's an easy fix 🤞

@ekatrukha
Copy link
Contributor

ekatrukha commented May 12, 2025

Hello @NicoKiaru,

examples in more-examples branch do not work, because you've cloned main branch of bvv.
The recent updates by @tpietzsch live in the 8and16bit branch (where new loading of 8-bit volumes works),
you need to use it as a reference.

Can you sync that branch in your fork and try them again?

The 8bits-dupcachespec has too many other changes to track what is going on, sorry, it looks very complicated.

The other thing, I looked at your (or Claude? :) solution for thin volumes and I think it is on the wrong path.
There is no need for the "close to zero ray angle" case.
The "one plane" volumes are not shown, since the current bounding box in BVV is a bit clipped (compared to BDV).
Basically, it is one pixel smaller in each dimension.
So if one (Z) dimension is 1, there would be no intersection.

Maybe you can take a look at this PR,
I hope it explains things a bit.

There is a quick "hack" solution there.
After some discussion with Tobias, a proper/better way to fix it would be to load multires/offsets differently,
but it is WIP.

Cheers,
Eugene

@NicoKiaru
Copy link
Author

Ah, double stupid me then!

Indeed it works! Fantastic!

Yeah regarding Claude that's a good lesson 🤣 . It does do stuff but if you don't understand and dig carefully it can go the wrong way. The result was there but not with the proper method. I also noticed that when I look at the side of thin plates something was off. I'm super eager to test your branch then.

The 8bits-dupcachespec has too many other changes to track what is going on, sorry, it looks very complicated.

Completely agree. My goal was to clean and make a new proper PR, but I'm glad I don't even need to do it anymore!

@NicoKiaru
Copy link
Author

Guess this PR is not needed anymore

@NicoKiaru NicoKiaru closed this May 12, 2025
@ekatrukha
Copy link
Contributor

It is a pretty complicated codebase, since multiple languages/devices + cache/memory handling.
I will test a bit the newest version in bvv-playground fork, but I think this update means
that we can finally remove "to 8-bit" in the future bdv-loaders release.

@imagesc-bot
Copy link

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/fiji-friends-weekly-dev-update-thread/103718/93

@ekatrukha
Copy link
Contributor

Hello @tpietzsch

I've been using it for the last week and did not find any errors,
so I think you can merge that branch to master.

Cheers,
Eugene

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

Successfully merging this pull request may close these issues.

4 participants