Bug fix for segfault in fixStaticSegmentRules #27
Conversation
besmaller
changed the title
src/animation/staticSegments.cpp
Outdated
| @@ -134,14 +134,18 @@ RuleChanges getPossibleRuleChanges(const ContinuousTimeline<ShapeRule>& shapeRul | |||
|
|
|||
| ContinuousTimeline<ShapeRule> fixStaticSegmentRules(const ContinuousTimeline<ShapeRule>& shapeRules, AnimationFunction animate) { | |||
| // The complexity of this function is exponential with the number of replacements. So let's cap that value. | |||
| const int maxReplacementCount = 3; | |||
There was a problem hiding this comment.
maxReplacementCount can stay const!
src/animation/staticSegments.cpp
Outdated
| for (int replacementCount = 1; bestScenario.getStaticSegmentCount() > 0 && replacementCount <= maxReplacementCount; ++replacementCount) { | ||
| for ( | ||
| int replacementCount = 1; | ||
| bestScenario.getStaticSegmentCount() > 0 && replacementCount <= std::min((int)possibleRuleChanges.size(), maxReplacementCount); |
There was a problem hiding this comment.
Could you change the C-style cast into a C++-style static_cast<int>()?
…es was less than 3 (maxReplacementCount)
besmaller
force-pushed
the
bugfix/#25-segfault
branch
from
7f383a8
to
6da2ca1
Compare
|
Still learning how to use git and proper github methodology, but I think I've implemented the changes you mentioned with the appropriate squash and force-push. I ran a local test on 32 and 64-bit builds as well (I need a 64-bit build to deal with a malloc issue in some of the wav files) |
|
Merged! (Travis CI automatically tests PRs in multiple environments, so I was pretty confident about this.) BTW: Rhubarb always processes the input file in small chunks, so even a 32-bit build ought to be able to process arbitrarily long WAVE files. I once tested it with a 30-minute file without any problems and with rather low memory consumption. If you are having any trouble, it would be great if you could create an issue. |
|
Thanks, Daniel. This issue resolution has been my first exposure to pull requests with github, and was quite educational. On the memory issue, I will recreate and open a new issue. I was tracking memory use by the app when it failed on a 10 minute podcast, and it looked like a the time of failure it needed about 1.4GB of memory. |
fixStaticSegmentRules could cause a segfault when trying to access out of the range of the currentRuleChanges iterator, if the size of possibleRuleChanges was less than 3 (maxReplacementCount).
The case exhibiting the problem had music in the input wav file, which has characteristics different from speech, which resulted in an unexpectedly low number of possibleRuleChanges in a static segment.