Conversation
|
Please provide performance results |
|
Here are the results: Old code: New code: The code for the tests is here, I just iterated 1000 times to generate a DNG. release...baso53:tinydng:improve-performance-tests Note that this is compiled with clang++ and -O3. For some reason, without optimizations, the old code is faster, but IMO this is not important. |
There was a problem hiding this comment.
Pull Request Overview
This PR improves DNG writing performance by replacing the internal std::ostringstream with a custom vectorbuf class that uses std::vector<char> for more efficient data storage. It also adds a new method WriteDNGToStream for directly writing DNG files from raw image data.
- Replace
std::ostringstreamwith customvectorbufimplementation for better performance - Update function signatures from
std::ostringstream*tostd::ostream*for consistency - Add new
WriteDNGToStreammethod for direct DNG file writing
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| buffer.push_back(char(c)); | ||
| // reset the put pointers to cover the new end | ||
| setp(buffer.data(), buffer.data() + buffer.size()); | ||
| pbump(int(buffer.size())); // position at end |
There was a problem hiding this comment.
Casting buffer.size() to int can cause overflow for large buffers. Use static_cast<std::streamsize> or check for overflow before casting.
| pbump(int(buffer.size())); // position at end | |
| pbump(static_cast<std::streamsize>(buffer.size())); // position at end |
| buffer.insert(buffer.end(), s, s + n); | ||
| // reset the put pointers | ||
| setp(buffer.data(), buffer.data() + buffer.size()); | ||
| pbump(int(oldsz + n)); |
There was a problem hiding this comment.
Casting oldsz + n to int can cause overflow for large buffer operations. Use static_cast<std::streamsize> or validate the value fits in an int.
|
|
||
| char* base = buffer.data(); | ||
| setp(base, base + buffer.size()); | ||
| pbump(int(newpos)); |
There was a problem hiding this comment.
Casting newpos to int can cause overflow for large seek positions. Use static_cast<std::streamsize> or validate the value fits in an int.
| /// Return error string to `err` when Write() returns false. | ||
| /// Returns true upon success. | ||
| /// | ||
| /// @param[in] data : same as in SetImageData |
There was a problem hiding this comment.
Remove trailing whitespace after 'SetImageData'.
| /// @param[in] data : same as in SetImageData | |
| /// @param[in] data : same as in SetImageData |
| if (bits_per_samples_.empty()) | ||
| { err_ += "BitsPerSample is not set\n"; return false; } |
There was a problem hiding this comment.
[nitpick] The opening brace should be on the same line as the if statement for consistency with project style.
| if (bits_per_samples_.empty()) | |
| { err_ += "BitsPerSample is not set\n"; return false; } | |
| if (bits_per_samples_.empty()) { err_ += "BitsPerSample is not set\n"; return false; } |
|
@baso53 Thank you! Numbers are nice. Please answer some comments(and do updates) from Copilot review, then PR will be merged. |
Use vectorbuf for storing internal data. This gives much better performance than ostringstream.
Also, added a method for writing a DNG directly from DNGImage.