Skip to content
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

SDT - Handle IllegalArgumentException #2981

Merged
merged 3 commits into from Jan 10, 2018

Conversation

dgault
Copy link
Member

@dgault dgault commented Oct 18, 2017

This is for an issue raised on the mailing list - http://lists.openmicroscopy.org.uk/pipermail/ome-users/2017-October/006718.html
Associated sample file: QA-18098

The problem is caused by reading the block length of the data block and attempting to skip that number of bytes. The expected result from skip bytes should be that it attempts to skip the number of bytes provided and if the end of the file is reached returns the actual number skipped (eg https://docs.oracle.com/javase/7/docs/api/java/io/RandomAccessFile.html#skipBytes(int)). However NIOFileHandle in common throws an exception.

For compressed data the number of available bytes in the file may be less. We have other sample files of this nature (QA-11079) which similarly attempt to skip a number of bytes which do not exist, by chance these do not throw the same error though. I believe the real fix should be in NIOHandle (likely https://github.com/ome/ome-common-java/blob/43ca478d2977455c79656b12f1ad60aa39f1f86e/src/main/java/loci/common/NIOFileHandle.java#L456) however it could be worthwhile to include a fix at the format level for a patch release.

To test:

  • Ensure all builds and tests remain green
  • Using QA-18098 and QA-18827, read the files without the PR (using showinf) and you should see an IllegalArgumentException
  • With PR redo the above test and ensure that the files are able to be read without exception

@dgault dgault added this to the 5.7.3 milestone Nov 17, 2017
@dgault
Copy link
Member Author

dgault commented Dec 6, 2017

A further report of this has been received in QA-18827

The fix originally in place in the PR was not sufficient for these latest sample files. The last commit should enable the correct block headers to be read using the nextBlockOffs, which from the CPP header we have as part of the spec is the offset to the data block header of the next data block.

@mtbc
Copy link
Member

mtbc commented Dec 7, 2017

Would simply always doing the else branch also work? Just curious as to the logic here.

@mtbc
Copy link
Member

mtbc commented Dec 7, 2017

I can't see anything coherent in the QA-18098 image but the functional testing does pass in my opinion.

@dgault
Copy link
Member Author

dgault commented Dec 7, 2017

Yeah, unfortunately the files which are causing this issue appear to be from FIFO mode which produces compressed photon data files, as such the actual display does not appear as an easy identifiable image for testing purposes. Instead it displays as single pixels with very low counts.

I shall test removing the else to see if there are any impact on existing data sets.

@dgault dgault merged commit 10cf825 into ome:develop Jan 10, 2018
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.

None yet

2 participants