Remove some dead code when writing

[kingsawyer]

The file version is set to 2 above for writing files, so it is impossible to take the first branch and write version 1 style. If people are interested in how older versions wrote they can look in history of ImfDwaCompressor.cpp or look at the read code.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: kingsawyer / name: Dave Sawyer (feee683, 2d2e852)

[lgritz]

Just to make sure I understand here... this code is specific to DWA compression, which is only a file version 2 feature, so that's why we can eliminate all the "version 1" clauses?

[peterhillman]

@lgritz it's not the version of OpenEXR library, or the general file format version. It's related to #119, where the internal DWA structure was changed and a version field bumped. The uncompress code supports the old DWAv1 compression format, but the unreachable code to write version 1 seems unnecessary. If anyone needed to write a DWAv1 file they could still change one line in the C++ library and use that.
@meshula I believe the sLegacyChannelRules is still required if a DWAv1 file needs to be read, as it looks like it's also used by the uncompress code

[meshula]

Oh, maybe I didn't grep outside of the OpenEXRCore folder.

[kingsawyer]

@lgritz This is purely localized reasoning. The code is unreachable because the conditional is a constant value.

[lgritz]

@kingsawyer Yes, but the subtext of my question was "are we satisfied that the assignment of that constant is not itself a bug."

[kingsawyer]

@meshula Can you please reconsider your request as it would break v1 support?

[kingsawyer]

@meshula Hello Nick?

resolved

[meshula]

Ah, I should have marked the question as resolved, since it was answered in the following discussion. @lgritz @peterhillman Any further comment?

[lgritz]

Fine with me. My understanding is that this code only comes into play with DWA compression, and therefore MUST be a v2 file.

[peterhillman]

Well, it's v2 of DWA, not the OpenEXR file version, so the modification to the comment is rather misleading.
Some DWAv1 files (created before #119 was merged) might still exist in the wild. The decompression code will still handle those files after this change. The C++ implementation can still be trivially modified to generate DWAv1 compression, but that code is likely to disappear when the C++ library switches to decompressing using OpenEXRCore.

I'm fine with this too, but I would suggest leaving a comment in the code stating that DWAv1 set me->_channelRules to sLegacyChannelRules. That would satisfy any code archeologists wanting to unpick the history. It would also help anybody wanting to modify the code to generate a new DWAv1 file for testing the decompressor.

[kingsawyer]

Updated to say DWA v2, but I don't think current code should talk much or at all about how obsolete code worked (or didn't). Easy enough to go back in the file history to see what it did. The vast majority will not care about implementation details of bygone days.

[peterhillman]

Thanks for contributing the original PR and for this update. That's a clearer comment now. An extra few words in that comment block noting how to generate DWAv1 would be useful because the code still supports decompressing DWAv1 files, which means it's not quite obsolete. It may be confusing that the compressor has no trace of how to generate a file that would trigger the v1 branch in the decompressor. The full history of DWA compression isn't in this file, it's in ImfDwaCompressor.cpp (which I'd forgotten might be removed later), so git blame doesn't really help.

[kingsawyer]

Updated the check-in comment to point to ImfDwaCompressor.cpp for history. That should be a good third path to finding the deleted code in case people don't want to look at the read code or sync the repo to an older hash. I don't think it's confusing that we write the latest format and read any of the old formats. It's how most any file format read/write works that wants to support backwards compatibility.

[meshula]

I think that's good, thanks