search

ome / bioformats

Bio-Formats is a Java library for reading and writing data in life sciences image file formats. It is developed by the Open Microscopy Environment. Bio-Formats is released under the GNU General Public License (GPL); commercial licenses are available from Glencoe Software.
https://www.openmicroscopy.org/bio-formats
GNU General Public License v2.0
378 stars 241 forks source link

JPEG: reset `useLegacy` flag to its original state upon close #4148

Closed melissalinkert closed 7 months ago

melissalinkert commented 8 months ago

This problem was exposed by #4061, but likely not directly caused by it.

I would expect tests to pass again with this included.

melissalinkert commented 8 months ago

A few initial thoughts:

  • if I understand the root of the issue, it would apply to any DelegateReader (in practice JPEGReader being the only one) that would be re-used to initialize different files requiring either the legacy reader or the native reader, is that correct?

I think the underlying problem is that useLegacy is actually changed in JPEGReader.setId, which DelegateReader and TiffDelegateReader (our only other DelegateReader implementation right now) don't do.

  • in terms of automated tests, whether the legacy or native reader is used is currently not captured in the configuration files. Is this something we would also like to serialize and test? Or would we assume as in the current case that an incorrect selection would either lead to an exception in setId or failures in the downstream tests?

I'd expect tests to fail if the wrong delegate was used.

  • should the fix be implemented more generically in DelegateReader? Naively, I was thinking that DelegateReader.close could simply reset useLegacy

That's a fair point. I was thinking this was more JPEG-specific since that's the only reader that alters useLegacy internally, but I can do the same fix in DelegateReader, and we can decide which one to keep?

melissalinkert commented 8 months ago

On second thought, I don't think a change in DelegateReader will actually work here. See https://github.com/ome/bioformats/compare/develop...melissalinkert:delegate-reader-reset?expand=1 for comparison.

These two lines in JPEGReader:

https://github.com/melissalinkert/bioformats/blob/delegate-reader-reset/components/formats-bsd/src/loci/formats/in/JPEGReader.java#L147 https://github.com/melissalinkert/bioformats/blob/delegate-reader-reset/components/formats-bsd/src/loci/formats/in/JPEGReader.java#L156

mean that DelegateReader resets to the updated value of useLegacy - not the default when a new reader is created or whatever was explicitly set by a call to setLegacy.

sbesson commented 8 months ago

Thanks for the feedback @melissalinkert. Today was busy but I'll take a closer look at this change on Monday. In the meantime, I have another naive question: would it be correct/equivalent to set useLegacy = false before https://github.com/ome/bioformats/blob/194a643d02ed766bf57705a3fa3f9e8d1c880cbc/components/formats-bsd/src/loci/formats/in/JPEGReader.java#L87 i.e. enforce that the first setId call is always made using the DefaultJPEGReader independently of the state of isLegacy()?