ome / omero-downloader

An OMERO client for downloading data in bulk from the server.

https://www.openmicroscopy.org/omero/apps/

GNU General Public License v2.0

4 stars 5 forks source link

Export problem with RGB JPEG #17

Closed mtbc closed 4 years ago

mtbc commented 5 years ago

If I use -f tiff to export 8kx8k.jpg then import the 8kx8k.jpg.tiff into demo the result looks horrifying. What's going wrong? Why did https://docs.openmicroscopy.org/internal/testing_scenarios/OMEROdownloader.html#file-exports miss it?

mtbc commented 5 years ago

The referenced "File exports" fails at step 10 for me. The issue's obvious enough that I'd be surprised if I'd got as far as listing the PR with things as I now see them and #11 also passed @dominikl's testing. The interesting question is: what's different now?

mtbc commented 5 years ago

10 is the culprit. Now to explore why.

mtbc commented 5 years ago

exported-images.zip show the difference to writing the TIFFs that setWriteSequentially is making.

mtbc commented 5 years ago

In its console output showinf shows no difference between the bad and good images other than in TileByteCounts.

mtbc commented 5 years ago

@dgault: Judging from TiffWriter.log does it seem to you that I misunderstood the contract I entered by using setWriteSequentially(true)? Maybe I wrote in the wrong order?

mtbc commented 5 years ago

example.zip is a very simple example where use of setWriteSequentially(true) appears to cause pixel data corruption. The generated test-false.tiff looks fine but test-true.tiff does not.

mtbc commented 5 years ago

Created https://trello.com/c/2w5BhotF/371-setwritesequentially-puzzle to hand over to the Bio-Formats folks. Will leave this issue open as a marker to have OMERO.downloader use setWriteSequentially(true) if it turns out that there is a way for it to.

mtbc commented 5 years ago

No, I didn't close this issue, GitHub did so automatically, probably imagining that to do so would be helpful.

joshmoore commented 5 years ago

Well you do have This PR fixes #17 in the description :smile:

mtbc commented 5 years ago

It does fix it but it's a temporary fix that I hope to revert someday! :smiley: As is too common I am fighting unsolicited extra help based on what somebody imagines my workflow to be. (Indeed, my mail app just "helpfully" added to my calendar a meeting I won't be attending.)

melissalinkert commented 4 years ago

The issue with the example code is that it creates a new IFD for each tile in the same plane, which confuses TiffSaver with setWriteSequentially(true). Moving the IFD construction outside of the tile loop fixes it so that both setWriteSequentially settings are correct.

Haven't tested downloader specifically, but a quick look suggests that reverting https://github.com/ome/omero-downloader/pull/18/commits/d6c4f224569b5143352ad2f69e92624a103819d0 and moving these 3 lines:

https://github.com/mtbc/omero-downloader/blob/a8803737cd417d50a29ef094c76f12402441c075/src/main/java/org/openmicroscopy/client/downloader/LocalPixels.java#L169

to here:

https://github.com/mtbc/omero-downloader/blob/a8803737cd417d50a29ef094c76f12402441c075/src/main/java/org/openmicroscopy/client/downloader/LocalPixels.java#L160

would be the equivalent fix.

mtbc commented 4 years ago

Does it make sense that I'd need a new IFD for each new plane?

melissalinkert commented 4 years ago

Definitely makes sense to create a separate IFD per plane (since IFD is TIFF-speak for all of the metadata for one plane). Reusing one IFD across multiple planes is definitely not a good idea, and creating a new one for each tile in a plane makes reassembling the tiles harder.