{"action":"created","issue":{"url":"https://api.github.com/repos/openmicroscopy/bioformats/issues/1749","labels_url":"https://api.github.com/repos/openmicroscopy/bioformats/issues/1749/labels{/name}","comments_url":"https://api.github.com/repos/openmicroscopy/bioformats/issues/1749/comments","events_url":"https://api.github.com/repos/openmicroscopy/bioformats/issues/1749/events","html_url":"https://github.com/openmicroscopy/bioformats/pull/1749","id":69698466,"number":1749,"title":"C++: Reader and writer channel-related fixes","user":{"login":"rleigh-dundee","id":1528953,"avatar_url":"https://avatars.githubusercontent.com/u/1528953?v=3","gravatar_id":"","url":"https://api.github.com/users/rleigh-dundee","html_url":"https://github.com/rleigh-dundee","followers_url":"https://api.github.com/users/rleigh-dundee/followers","following_url":"https://api.github.com/users/rleigh-dundee/following{/other_user}","gists_url":"https://api.github.com/users/rleigh-dundee/gists{/gist_id}","starred_url":"https://api.github.com/users/rleigh-dundee/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/rleigh-dundee/subscriptions","organizations_url":"https://api.github.com/users/rleigh-dundee/orgs","repos_url":"https://api.github.com/users/rleigh-dundee/repos","events_url":"https://api.github.com/users/rleigh-dundee/events{/privacy}","received_events_url":"https://api.github.com/users/rleigh-dundee/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":9,"created_at":"2015-04-20T22:49:22Z","updated_at":"2015-04-21T12:57:09Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/openmicroscopy/bioformats/pulls/1749","html_url":"https://github.com/openmicroscopy/bioformats/pull/1749","diff_url":"https://github.com/openmicroscopy/bioformats/pull/1749.diff","patch_url":"https://github.com/openmicroscopy/bioformats/pull/1749.patch"},"body":"Both the existing readers and writers were making some mistaken assuptions with SizeC/EffectiveSizeC/ImageCount/RGBChannelCount.  This PR corrects these assumptions to use these data correctly, and it also adds some fixes to the API to handle subchannel data and calculations correctly in all circumstances.  I was going to wait on doing this, as discussed, but the impact of getting things wrong is quite severe.  Unlike the Java situation, where using incorrect dimension sizes/indices will at worst give an ArrayOutOfBoundsException, here we will at best immediately segfault or worse, silently corrupt memory and write out junk data.  I thought it better to err on the side of caution rather than give the user incorrect data with potentially catastrophic results.\r\n\r\n- CoreMetadata: Make `sizeC` a vector of integers to accurately represent SamplesPerPixel subchannels on a per-channel basis; remove the now redundant `rgb` field.  SizeC is the sum of all subchannels; EffectiveSizeC is the size of the vector.  `imageCount` is retained as a sanity check but otherwise redundant.\r\n- FormatReader: `isRGB`, `getRGBChannelCount`, `getOptimalTileWidth` and `getOptimalTileHeight` have a `channel` parameter to specify the channel to use, and make use of the per-channel subchannel count.  `getSizeC` and `getEffectiveSizeC` updated to use the new calculations.\r\n- FormatWriter already provided a subset of these methods, which are extended to provide an equivalent set (using MetadataStore rather than CoreMetadata)\r\n- MetadataTools updated to use updated channel count calculations\r\n- All readers can now read RGB data with correct plane indexing and buffer sizing\r\n- MinimalTIFFWriter can now write RGB data\r\n- OMETIFFReader initFile logic updated to use subchannels; this has simplified it a bit\r\n- ImageInfo updated to display subchannels\r\n- Testcases updated\r\n\r\nAdditional minor API fixes:\r\n\r\n- Parameter names and API docs updated to replace the generic `no` by `series`, `plane` etc. to make the API more self-describing; similar for sample counts.\r\n- LookupTable parameters swapped to make the API consistent for methods using plane index and buffer arguments; fixed assertions and unit test for default LUT behaviour\r\n- FormatWriter has added interleaving methods, used by the TIFF writer\r\n\r\n--------\r\n\r\nQuestion: The addition of the `channel` argument works, but an alternative would be to use a `plane` argument to make the method calls align open/saveBytes where the information is most likely to be used (these would internally call getZCTCoords with the provided plane number to get the channel).  Would that be better than channel, or is channel OK?  From the POV of display/rendering, channel is what's needed, but for the user of openBytes/saveBytes, it saves them doing the getZCTCoords themselves to do the lookup before calling methods with channel arguments.\r\n\r\n--------\r\n\r\nTesting: All the dimension-related changes are tested by the unit tests.  The TIFF writing changes may be tested by editing `cpp/test/ome-bioformats/minimaltiffwriter.cpp` and changing:\r\n\r\n```\r\ndiff --git a/cpp/test/ome-bioformats/minimaltiffwriter.cpp b/cpp/test/ome-bioformats/minimaltiffwriter.cpp\r\nindex b042a89..9734bc4 100644\r\n--- a/cpp/test/ome-bioformats/minimaltiffwriter.cpp\r\n+++ b/cpp/test/ome-bioformats/minimaltiffwriter.cpp\r\n@@ -124,8 +124,8 @@ public:\r\n   TearDown()\r\n   {\r\n     // Delete file (if any)\r\n-    if (boost::filesystem::exists(testfile))\r\n-      boost::filesystem::remove(testfile);\r\n+    // if (boost::filesystem::exists(testfile))\r\n+    //   boost::filesystem::remove(testfile);\r\n   }\r\n };\r\n \r\n@@ -148,7 +148,7 @@ TEST_P(TIFFWriterTest, setId)\r\n \r\n   tiffwriter.setMetadataRetrieve(retrieve);\r\n \r\n-  bool interleaved = true;\r\n+  bool interleaved = false;\r\n \r\n   tiffwriter.setInterleaved(interleaved);\r\n ```\r\n\r\nAfter running the tests, look at the files in `cpp/test/ome-bioformats/data/minimaltiffwriter-data-layout-*`.  These should be RGB with one IFD (previously greyscale with 3 IFDs), for example:\r\n\r\n```\r\n% tiffinfo cpp/test/ome-bioformats/data/minimaltiffwriter-data-layout-64x64-planar-tiles-48x16.tiff\r\nTIFF Directory at offset 0x3008 (12296)\r\n  Image Width: 64 Image Length: 64\r\n  Bits/Sample: 8\r\n  Sample Format: unsigned integer\r\n  Compression Scheme: None\r\n  Photometric Interpretation: RGB color\r\n  Samples/Pixel: 3\r\n  Rows/Strip: 1\r\n  Planar Configuration: single image plane\r\n  Software: OME Bio-Formats (C++) 5.1.0\r\n```\r\n\r\nAlso, run `./bf-test ./cpp/libexec/info/info /path/to/ome-tiff` with a 2013-06 OME-TIFF; ImageInfo will show the constituent subchannels in the RGB output.\r\n"},"comment":{"url":"https://api.github.com/repos/openmicroscopy/bioformats/issues/comments/94782047","html_url":"https://github.com/openmicroscopy/bioformats/pull/1749#issuecomment-94782047","issue_url":"https://api.github.com/repos/openmicroscopy/bioformats/issues/1749","id":94782047,"user":{"login":"mtbc","id":2630707,"avatar_url":"https://avatars.githubusercontent.com/u/2630707?v=3","gravatar_id":"","url":"https://api.github.com/users/mtbc","html_url":"https://github.com/mtbc","followers_url":"https://api.github.com/users/mtbc/followers","following_url":"https://api.github.com/users/mtbc/following{/other_user}","gists_url":"https://api.github.com/users/mtbc/gists{/gist_id}","starred_url":"https://api.github.com/users/mtbc/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mtbc/subscriptions","organizations_url":"https://api.github.com/users/mtbc/orgs","repos_url":"https://api.github.com/users/mtbc/repos","events_url":"https://api.github.com/users/mtbc/events{/privacy}","received_events_url":"https://api.github.com/users/mtbc/received_events","type":"User","site_admin":false},"created_at":"2015-04-21T12:57:09Z","updated_at":"2015-04-21T12:57:09Z","body":"Sorry, didn't realize it was a suggested correction! :smiley:"}}