Add support for reading DPX files from memory

[r-arjocu]

Description

Add support for reading DPX files from memory. I think it's obvious from the code that I've transformed the LibDpx InStream class into an interface, moved its implementation to InStreamFile and added an InStreamMem class that reads from memory; also updated the DPX OIIO code to allow for an IOProxy when reading.

Tests

I have separately tested the LibDpx InStreamMem class in a small project, but it is not straightforward to port and I have little experience with OIIO's testing environment.
But I have seen there are tests in place that test reading from memory if an OIIO ImageInput supports "ioproxy" attribute, so reading DPX from memory is tested automatically.

Checklist:

• I have read the contribution guidelines.
• If this is more extensive than a small change to existing code, I
  have previously submitted a Contributor License Agreement
  (individual, and if there is any way my
  employers might think my programming belongs to them, then also
  corporate).
• I have updated the documentation, if applicable.
• I have ensured that the change is tested somewhere in the testsuite
  (adding new test cases if necessary).
• My code follows the prevailing code style of this project.

[r-arjocu]

I am unsure about the clang-format, since the LibDpx file modifications were done in a separate project, in order to separately test reading from actual DPX files; I've tried to keep the original LibDpx style. Visual Studio applied the OIIO formatting for "dpxinput.cpp", but I don't think it did for the LibDpx files.

[r-arjocu]

Please "forward port" the commit on the 'master' branch!

[lgritz]

Please "forward port" the commit on the 'master' branch!

Oh yes, I will always port fixes to master. And also to 2.2 which is in beta now and will soon be the supported release branch.

[Biswa96]

If the memcpy_s logic is correct, it can be used with #ifdef _WIN32 like conditions.

[r-arjocu]

If the memcpy_s logic is correct, it can be used with #ifdef _WIN32 like conditions.

I guard against overflowing copy through auto cpySize = (remSize < size) ? remSize : size;. On VS and Windows memcpy_s doesn't give a warning (and is an extra check sometimes). But I have seen gcc and other compilers don't like it, so I think it's safe to use memcpy without complicating the code with #ifdef-s

[lgritz]

I really like where this is going, and I appreciate the addition of IOProxy support to DPX.

OK, so if I understand, you now allow a config hint to supply an IOProxy -- but only the memreader kind -- and then you have a subclass of libdpx's InStream that basically "wraps" the IOProxy. When the hint is not configured, you just use another subclass of InStream that basically did what it always did. (Really, you renamed InStream to InStreamFile, made a new InStream as a pure virtual parent class of that and the new InStreamMem.)

But does anything you've done in InStreamMem truly depend on the fact that it's an IOMemReader?

Maybe a more general design would be to ONLY have what is now your InStreamMem (though you can keep the old name, InStream), and let it use any IOProxy type, not just a memreader.

For the case where no IOProxy is supplied by the user, you just create one (of the IOFile variety) internally to the DPXReader.

The advantages of this are:

• Just one code path, not a separate one for files and mem reading.
• Less total code (you can eliminate the file I/O from the old InStream, all the real work is done by the IOProxy.
• Can work with other, or future, implementations of IOProxy that are not memreader.

An example of this is what we currently do in the PNG reader, which you can see here: https://github.com/OpenImageIO/oiio/blob/master/src/png.imageio/pnginput.cpp#L144

[lgritz]

I believe you also want to add

virtual bool set_ioproxy(Filesystem::IOProxy* ioproxy) override

to this class

[lgritz]

Actually, set_ioproxy may only be a thing for 2.2, not 2.1.

If you're liking ioproxies, you should really try to get to 2.2 as soon as you can. There were several interesting enhancements.

[r-arjocu]

I've seen some in the latest docs that it's easier to create ImageInput-s which read from memory. But I'm somewhat dependent on Vcpkg and especially on time. Maybe we'll switch to 2.2 when a newer version of OpenEXR is available on Vcpgkg -- I don't think I'll get the chance to learn how to modify and modify its "ports".

[r-arjocu]

I really like where this is going, and I appreciate the addition of IOProxy support to DPX.

OK, so if I understand, you now allow a config hint to supply an IOProxy -- but only the memreader kind -- and then you have a subclass of libdpx's InStream that basically "wraps" the IOProxy. When the hint is not configured, you just use another subclass of InStream that basically did what it always did. (Really, you renamed InStream to InStreamFile, made a new InStream as a pure virtual parent class of that and the new InStreamMem.)

But does anything you've done in InStreamMem truly depend on the fact that it's an IOMemReader?

Maybe a more general design would be to ONLY have what is now your InStreamMem (though you can keep the old name, InStream), and let it use any IOProxy type, not just a memreader.

For the case where no IOProxy is supplied by the user, you just create one (of the IOFile variety) internally to the DPXReader.

The advantages of this are:

* Just one code path, not a separate one for files and mem reading.

* Less total code (you can eliminate the file I/O from the old InStream, all the real work is done by the IOProxy.

* Can work with other, or future, implementations of IOProxy that are not memreader.

An example of this is what we currently do in the PNG reader, which you can see here: https://github.com/OpenImageIO/oiio/blob/master/src/png.imageio/pnginput.cpp#L144

I understand the benefits, but I'm unsure about the technical implementation you advise. The code in PNG is fairly straightforward regarding m_io. But from the point of view of LibDpx: it needs a "stream reader" for its DPXReader class, set through SetInStream(). Are you telling me to convert back the InStream from a pure virtual to a "normal" class, and to put an IOProxy* member field, and that all calls to the InStream methods should be forwarded to and resolved by the IOProxy* field ?

[lgritz]

I'm saying that I think that your implementation of InStreamMem will work with any IOProxy, not just the memreader kind.

So you could call it InStreamIOProxy for clarity. And therefore that's the only kind of InStream you need -- you don't need InStreamFile at all, if you want to read from a file, you just create an IOFile kind of proxy and hand it to InStreamIOProxy. And if you do all I/O through that, you can get rid of InStreamFile, and get rid of the now-useless InStream parent class, and rename the InStreamMem/InStreamIOProxy to be simply the one and only InStream.

If this doesn't make a ton of sense, don't worry about it. I can start with what you have, mock it up, and see if it works.

[r-arjocu]

I understand what you're saying. I'll give it a try now. Thank you!

[lgritz]

I understand what you're saying. I'll give it a try now. Thank you!

Cool, thanks. I think that if it works, you'll be pleased with the simplicity of the result, and it will make the DPX reader a lot more flexible by being able to handle any kind of proxy nor or in the future.

[r-arjocu]

Kind of off topic, but isn't there a bug in the IOProxy class on line 359, RB-2.1?

 bool seek (int64_t offset, int origin) {
        return seek ((origin == SEEK_SET ? offset : 0) +
                     (origin == SEEK_CUR ? offset+tell() : 0) +
                     (origin == SEEK_END ? size() : 0));
    }

Shouldn't it be (origin == SEEK_END ? offset+int64_t(size()) : 0?

[lgritz]

Um... I think you are right!

Can you add this fix as well? Thanks.

[r-arjocu]

Um... I think you are right!

Can you add this fix as well? Thanks.

I can add it. Should it be a separate pull request? --> I'll add it in the current pull req.

One question regarding tell() function from IOProxy. If the proxy is closed, does it return 0 or -1? --> I deduce from code that it's 0 (zero); I had it -1 in InStreamMem (for testing purposes mainly), but I'll set it to 0 now for uniformity.

[lgritz]

Same PR or separate, up to you. I think arguments could be made either way (separate topic than DPX? or both are enhancing our use of IOProxy, so make sense to do together?).

C++ ftell returns -1 if there's an error, so IOProxy::tell should behave the same way.

[r-arjocu]

Pushed the advised implementation. I hope it's ok. Since I'll be away for 4 or 5 days, I wanted to finish this.
I'm not particularly proud of the m_io field from LibDpx InStream, but couldn't find a better and quicker solution -- besides, I've wasted more than an hour on a typo.
Done testing with imageinout_test and on our files.

[r-arjocu]

A second off topic, I want to mention an inconvenience on Windows at least (maybe it will help somebody else): CMake sets OpenEXP library paths and library names for debug configuration to point to the release ones. I don't know if this has to do with a non-optimal OpenEXR CMake files (I'm new to CMake) or with the Vcpkg ports -- either way, CMake only has the paths and names to the release libraries, and I had to manually do corrections in the project properties of Visual Studio (more than half an hour).
Else, some nasty errors/crashes manifested when running imageinout_test on EXR. Also, saving the project files helps on VS, because it seems it defaults back to the CMake settings when building the whole solution.

[lgritz]

There was some slightly broken CI (mostly because you used a C++14 idiom that broke C++11), but I fixed and pushed an update for you. If that passes, I will merge.

[lgritz]

LGTM, thanks for the fixes!

[lgritz]

I will port forward to 2.2 and master

[lgritz]

Um... we have a bit of a problem, which I did not discover until after I'd merged this and tried to forward-port it to 2.2 and master (your patch was against RB-2.1). You see, in 2.2+, there's another unit test called imageinout_test, and among other things, it tries reading and writing every format in a variety of ways including with IOProxy, both directly with ImageInput, but also via an ImageBuf (which was extended in 2.2 to support IOProxy, along with ImageCache). And, unfortunately, this test is now crashing in the DPX IOProxy-related code.

I'm not sure how bad it is. It could be a minor bug in DPX's handling of IOProxy, undiscovered in your original port because imageinout_test isn't part of 2.1, but the flaw is still there. Or, it could be that the DPX code is fine per se, but it's exposing a deeper logical problem in how ImageBuf and/or ImageCache deal with IOProxy.

I'll look into it and try to fix over the weekend. If it's the case that it's a simple fix, I'll try to fix and apply the patch to 2.1 as well as merging the fixed version into master and 2.2, and we're all set.

But if the changes necessary to fully fix it are compatibility-breaking, or just too extensive and risky, this will have to be a "master" feature only and I'll need to revert the patch to remove it from 2.1 (which is supposed to be exceptionally stable -- I can't introduce any instability or incompatibility at this point in its life).

I'll let you know what I find. But I just wanted to warn you as early as possible that there's at least some possibility that this will not be able to be a 2.1 feature.

[r-arjocu]

There was some slightly broken CI (mostly because you used a C++14 idiom that broke C++11), but I fixed and pushed an update for you. If that passes, I will merge.

I wanted to fix it quickly now, because it was too late last night. So thanks for the fix!

[r-arjocu]

Um... we have a bit of a problem, which I did not discover until after I'd merged this and tried to forward-port it to 2.2 and master (your patch was against RB-2.1). You see, in 2.2+, there's another unit test called imageinout_test, and among other things, it tries reading and writing every format in a variety of ways including with IOProxy, both directly with ImageInput, but also via an ImageBuf (which was extended in 2.2 to support IOProxy, along with ImageCache). And, unfortunately, this test is now crashing in the DPX IOProxy-related code.

I'm not sure how bad it is. It could be a minor bug in DPX's handling of IOProxy, undiscovered in your original port because imageinout_test isn't part of 2.1, but the flaw is still there. Or, it could be that the DPX code is fine per se, but it's exposing a deeper logical problem in how ImageBuf and/or ImageCache deal with IOProxy.

I'll look into it and try to fix over the weekend. If it's the case that it's a simple fix, I'll try to fix and apply the patch to 2.1 as well as merging the fixed version into master and 2.2, and we're all set.

But if the changes necessary to fully fix it are compatibility-breaking, or just too extensive and risky, this will have to be a "master" feature only and I'll need to revert the patch to remove it from 2.1 (which is supposed to be exceptionally stable -- I can't introduce any instability or incompatibility at this point in its life).

I'll let you know what I find. But I just wanted to warn you as early as possible that there's at least some possibility that this will not be able to be a 2.1 feature.

If it's not going to make it in 2.1, it's not a big problem. I've locally added my original solution to 2.1.17.0 and built that, and that code is running ok, tested by our QA. It will be shortly in production usage, and we'll get some feedback from there. If we'll need other fixes and enhancements, I think we'll go with OIIO 2.2 -- with the help of Vcpkg hopefully.

I'll have an eye over what you discover and the fixes when I'll come back. Thank you!

[lgritz]

I think #2665 solves the problem for 2.1 on the DPX side, then there's an additional small change for the ImageCache side I will submit separately.

[r-arjocu]

That's great news. Thank you!