Move off deprecated OpenMP API, fix FFmpeg code paths #530
Conversation
- Replacement openmp_set_max_active_levels() takes an int argument - In OpenMP 5.0 it can be set to openmp_get_supported_active_levels() - Below 5.0, we just set it to our processor count - Move configuration of OpenMP from ad-hoc locations to constructors for FFmpegWriter/Reader and Timeline
| // Write the frames once it reaches the correct cache size | ||
| if ((int)spooled_video_frames.size() == cache_size || (int)spooled_audio_frames.size() == cache_size) { | ||
| // Is writer currently writing? | ||
| if (!is_writing) | ||
| // Write frames to video file | ||
| write_queued_frames(); | ||
|
|
||
| else { | ||
| // Write frames to video file | ||
| write_queued_frames(); | ||
| } | ||
| // Write frames to video file | ||
| write_queued_frames(); | ||
| } |
There was a problem hiding this comment.
This was the bit Codecov flagged as invalid branching. I should certainly say so! Turns out this block has a bit of history.
Once upon a time it looked basically exactly as it will after my change. Then, 8 years ago in 0f3758b, it became this:
if (spooled_video_frames.size() == cache_size || spooled_audio_frames.size() == cache_size)
{
// Is writer currently writing?
if (!is_writing)
// Write frames to video file
write_queued_frames();
else
{
// YES, WRITING... so wait until it finishes, before writing again
while (is_writing)
usleep(250 * 1000); // sleep for 250 milliseconds
// Write frames to video file
write_queued_frames();
}
}It trundled along like that with only minor changes until 851ad86, when (despite the commit message being "Changing some sleep() calls to usleep()", the sleep was removed entirely, making the two branches accidentally equivalent.
Maybe the right change is to put the sleep call back, I don't know. But in its current state the code is nonsensical, both branches are identical so there's no reason to check is_writing there.
There was a problem hiding this comment.
(I might also point out that even if we want to put the sleep-before-write back in, the cleanest form would be...)
if ((int)spooled_video_frames.size() == cache_size || (int)spooled_audio_frames.size() == cache_size) {
// Wait until an in-progress write finishes
while (is_writing) {
std::this_thread::sleep_for(std::chrono::milliseconds(250));
}
write_queued_frames();
}There's no reason for the preceding if(), when is_writing is false execution will fall straight through the while() without a single iteration.
ferdnyc
added
the
resources
Codecov Report
@@ Coverage Diff @@
## develop #530 +/- ##
===========================================
+ Coverage 48.81% 48.85% +0.04%
===========================================
Files 129 129
Lines 10042 10033 -9
===========================================
Hits 4902 4902
+ Misses 5140 5131 -9
Continue to review full report at Codecov.
|
| /// or throws an exception. | ||
| /// @param path The filesystem location to load | ||
| /// @param inspect_reader if true (the default), automatically open the media file and loads frame 1. | ||
| FFmpegReader(const std::string& path, bool inspect_reader=true); |
There was a problem hiding this comment.
Previously the two constructors for FFmpegReader differed only in that one required an inspect_reader argument, which turned off the Open(); Close(); cycle that was always performed by the one-arg version. (IOW, there was no reason to use the 2-arg version unless inspect_reader was false.) By making it one constructor with an optional inspect_reader argument that defaults to true, I was able to eliminate an entire constructor's worth of duplicate code.
While I was touching these constructors, I changed their string args to take a const reference, so that we can try it out as a change. Especially for arguments that are simply copied to member variables in the initializer list, there should be no down side, only a slightly more efficient method that avoids one unnecessary redundant copy of a class that's just going to be copied again by the initializer list. I anticipate submitting a much larger PR that updates a significant porition of our class methods in the same manner, so consider this a trial run.
There was a problem hiding this comment.
The documentation for a bunch of these constructors also kept ending on, "...throws one of the following exceptions." with no list of exceptions after it. There's nothing worse than documentation that teases you and doesn't follow through, so I changed it to not over-promise.
| // Set the number of threads in OpenMP | ||
| omp_set_num_threads(OPEN_MP_NUM_PROCESSORS); | ||
| // Allow nested OpenMP sections | ||
| omp_set_nested(true); |
There was a problem hiding this comment.
Last but not least, I noticed there were a few places scattered around the code that kept making these calls. They were always in very ad-hoc locations, generally right before a nested #pragma section was actually used. But that meant (a) they weren't necessarily in effect before that point, and (b) they'd keep getting called every time that block of code was run.
I have no idea how expensive omp_set_num_threads() and omp_set_nested() are. I assume not very (probably not at all, really), but they can't be FREE and there's no reason to call them more than once. So, I moved this initialization into the class constructors where it can get done once and never needs to be fiddled wih again.
There's also the fact that omp_set_nested() was deprecated in OpenMP 4.5 due to its lack of granularity. OpenMP has always (since 1.0) supported omp_set_max_active_levels(), which allows control over the amount of nesting possible in the code, rather than just offering a boolean switch. So the code I moved to the constructors uses that, instead.
|
Good catch! I think this implementation is fine for now. I'll do some local testing as well. |
This was primarily inspired by Codecov flagging some code as unreachable, turned out it has a bit of history. But this corrects that, moves us off of a deprecated OpenMP API call, and makes some tweaks to FFmpegWriter/Reader along with Timeline, while I was touching them.
I'll explain some of the changes in inline comments.