-
Notifications
You must be signed in to change notification settings - Fork 241
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
Fix plane cropping problem in BF-ITK #104
Conversation
We want to call openBytes(no, x, y, w, h) rather than always calling openBytes(no) and then cropping after the fact. Otherwise, it is impossible to read >2GB planes in tiles from ITK.
{ | ||
for( int i=0; i<rgbChannelCount; i++ ) | ||
{ | ||
for( int b=0; b<bpp; b++ ) | ||
{ | ||
int index = xCount * (yCount * (rgbChannelCount * b + i) + y) + x; | ||
int index = xLen * (yLen * (rgbChannelCount * b + i) + y) + x; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this calculation is quite right for RGB images.
If we have a 2x2 (xLen = 2, yLen = 2) image with 3 RGB channels and 2 bytes per pixel, then as it is index
will be 12 when we try to retrieve the b=1 byte from (0, 0) at channel 0. Regardless of whether the channels are interleaved, the index in this case should be 1.
If we have the same image and wish to retrieve the b=0 byte from (1, 1) at channel 1, then as it is index
will be 7. If the channels are interleaved, then the index should be 20; if the channels are not interleaved, then the index should be 14.
From a "bugfix" standpoint, this PR addresses the reported problem, and I would say is purely better than the old code. That said, it is true that this code is not 100% correct yet; e.g., if you look at line 312 you'll notice that the interleaved status is not taken into account at all. I will be out of the office for the next two weeks; in the meantime, either @hinerm or @melissalinkert: please feel free to fix it (you both have commit rights to my copy of bioformats.git). |
I think the index calculation is fixed now. @hinerm, when you have a few minutes could you please take a look and see if that looks correct to you as well? If it does, then we'll go ahead and merge. |
@melissalinkert : these changes do look correct to me.. I'm happy with them being merged. |
Fix plane cropping problem in BF-ITK
@melissalinkert & @hinerm: Thanks! |
Additional improvements to pyramid .czi handling
We want to call openBytes(no, x, y, w, h) rather than always calling
openBytes(no) and then cropping after the fact. Otherwise, it is
impossible to read >2GB planes in tiles from ITK.
Thanks to Matthias Noll for reporting this issue.