Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

RgbaInputFile: Multipart support #1194

Merged
merged 1 commit into from
Dec 2, 2021
Merged

RgbaInputFile: Multipart support #1194

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/lib/OpenEXR/ImfMultiPartInputFile.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,8 @@ class MultiPartInputFile : public GenericInputFile
friend class ScanLineInputFile;
friend class DeepScanLineInputFile;
friend class DeepTiledInputFile;

friend class RgbaInputFile;
};


Expand Down
20 changes: 19 additions & 1 deletion src/lib/OpenEXR/ImfRgbaFile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
#include <ImfRgbaFile.h>
#include <ImfOutputFile.h>
#include <ImfInputFile.h>
#include <ImfMultiPartInputFile.h>
#include <ImfChannelList.h>
#include <ImfRgbaYca.h>
#include <ImfStandardAttributes.h>
Expand Down Expand Up @@ -1193,6 +1194,20 @@ RgbaInputFile::RgbaInputFile (const char name[], int numThreads):
}


RgbaInputFile::RgbaInputFile (int partNumber, const char name[], int numThreads):
_inputFile (0),
_fromYca (0),
_channelNamePrefix ("")
{
_multiPartFile = new MultiPartInputFile(name, numThreads);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will cause a memory leak if any of the following lines throw an exception, since the destructor won't be called to delete the _multiPartFile.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. Does the same apply to _inputFile in the other existing constructor as well? Here _inputFile is simply new'd too, that's what I used as a basis basically:

RgbaInputFile::RgbaInputFile (const char name[], int numThreads):
    _inputFile (new InputFile (name, numThreads)),
    _fromYca (0),
    _channelNamePrefix ("")
{
    RgbaChannels rgbaChannels = channels();

    if (rgbaChannels & WRITE_C)
	_fromYca = new FromYca (*_inputFile, rgbaChannels);
}

_inputFile = _multiPartFile->getInputPart<InputFile>(partNumber);
RgbaChannels rgbaChannels = channels();

if (rgbaChannels & (WRITE_Y | WRITE_C))
_fromYca = new FromYca (*_inputFile, rgbaChannels);
}


RgbaInputFile::RgbaInputFile (OPENEXR_IMF_INTERNAL_NAMESPACE::IStream &is, int numThreads):
_inputFile (new InputFile (is, numThreads)),
_fromYca (0),
Expand Down Expand Up @@ -1237,7 +1252,10 @@ RgbaInputFile::RgbaInputFile (OPENEXR_IMF_INTERNAL_NAMESPACE::IStream &is,

RgbaInputFile::~RgbaInputFile ()
{
delete _inputFile;
if (_multiPartFile)
delete _multiPartFile;
else
delete _inputFile;
delete _fromYca;
}

Expand Down
5 changes: 5 additions & 0 deletions src/lib/OpenEXR/ImfRgbaFile.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,10 @@ class RgbaInputFile
RgbaInputFile (const char name[], int numThreads = globalThreadCount());


IMF_EXPORT
RgbaInputFile (int partNumber, const char name[], int numThreads = globalThreadCount());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The disadvantage of this approach is that you need to know how many parts are in the file. You also cannot use this API to scan through the parts of a file without reopening the file each time with a different partNumber

A more complete API might provide a 'parts()' method to return the number of parts, and a 'setPart' method to change the part used for subsequent reads, as setLayerName() does.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this also ties into your previous comment regarding using the MultiPart API for all constructors.
I'm not sure if I'm the best person for this task, maybe this should have been a feature request (with my code shown as a proof of concept) rather than a pull request.
What do you think/propose?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a great addition, and I like your approach. If you'd like to go ahead and fix the DCO and CLA issues, we can merge it as is. I'm happy to make extra tweaks on top of that. Otherwise, I guess we can start again?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say take the code and run. I could go about the DCO/CLA stuff but if it's OK with you just take the commit, cherry pick or copy or use a basis, as you prefer. From my perspective I proposed this because I need this in a project, and it's not convenient to maintain a fork.
Let me know what you think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, that's against the spirit behind the DCO/CLA. The PR is your code, so "take it and run" is really just another way of saying "commit it without a DCO or CLA," right? Somebody else re-typing it doesn't change the original authorship or ownership of the code. It's the DCO/CLA that is the formality that certifies to us that it is indeed ok with the owner of the code to incorporate it into the project under the project's license terms.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DCO/CLA done. Now you can legally take the code and run ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!



//-----------------------------------------------------------
// Constructor -- attaches the new RgbaInputFile object to a
// file that has already been opened by the caller.
Expand Down Expand Up @@ -440,6 +444,7 @@ class RgbaInputFile
InputFile * _inputFile;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's too much burden to switch this to an InputPart, and use the MultiPart API for all constructors.
That also means you don't need the 'friend' in the header

FromYca * _fromYca;
std::string _channelNamePrefix;
MultiPartInputFile* _multiPartFile = nullptr;
};


Expand Down