-
Notifications
You must be signed in to change notification settings - Fork 617
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.
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
RgbaInputFile: Multipart support #1194
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -320,6 +320,10 @@ class RgbaInputFile | |
RgbaInputFile (const char name[], int numThreads = globalThreadCount()); | ||
|
||
|
||
IMF_EXPORT | ||
RgbaInputFile (int partNumber, const char name[], int numThreads = globalThreadCount()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. DCO/CLA done. Now you can legally take the code and run ;) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
@@ -440,6 +444,7 @@ class RgbaInputFile | |
InputFile * _inputFile; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
FromYca * _fromYca; | ||
std::string _channelNamePrefix; | ||
MultiPartInputFile* _multiPartFile = nullptr; | ||
}; | ||
|
||
|
||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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: