Fix no composite frame on playback (from Nir) #9210
Conversation
| template< class T > | ||
| class waiting_on |
There was a problem hiding this comment.
The class seem to mimic std::future, why is it needed?
There was a problem hiding this comment.
Because we're not using std::future... and the whole mechanism of the dispatcher is written without it.
Perhaps we can refactor this class to use it later, but for now... no.
| if( auto wait_state = still_alive() ) | ||
| wait_state->signal( t ); |
There was a problem hiding this comment.
The operation needs to be atomic to avoid race cond.
There was a problem hiding this comment.
The whole class is meant to be used in a master-slave kind of relationship: one thread running one function (and not multiple threads running one function). You're waiting on something to happen with a timeout. That's all.
The use-case of multiple threads all trying to signal the same waiting object is still possible -- this class does not decide for the user what the synchronization mechanism is. The user writes the lambda, which can worry about synchronization if it needs it. I don't think it's the job of the class.
I'll add the above to the class comment.
| // break; | ||
| // } | ||
| template < class U, class L > | ||
| void wait_until( U const& timeout, L const& pred ) |
There was a problem hiding this comment.
I think its better if wait_until will return bool - true = pred , false =timeout
There was a problem hiding this comment.
I prefer with no return value to make the code more readable
|
|
||
| // Wake up the local function (which is using wait_until(), presumable) with a new | ||
| // T value | ||
| void signal( T const& t ) const |
There was a problem hiding this comment.
Consider to change it to safe_signal() , I think its clear what is the added value of this.
There was a problem hiding this comment.
Nothing makes it safe... it's just a convenience
| { | ||
| } | ||
|
|
||
| operator T &() { return _value; } |
There was a problem hiding this comment.
we want to enable to change the value without signal?
There was a problem hiding this comment.
Sure, the set-and-signal function is just a convenience function
common/utilities/time/waiting-on.h
Outdated
| // Convert to the in-thread representation | ||
| in_thread_ in_thread() const { return in_thread_( *this ); } | ||
|
|
||
| operator T const &() const { return _ptr->_value; } |
There was a problem hiding this comment.
can't use operator T &() of wait_state_t?
There was a problem hiding this comment.
Changed to *_ptr instead of _ptr->_value
| // struct value_t { double x; int k; }; | ||
| // waiting_on< value_t > output({ 1., -1 }); | ||
| // output->x = 2.; | ||
| T * operator->() { return &_ptr->_value; } |
There was a problem hiding this comment.
No easy syntax that can be used here
| // waiting_on< value_t > output({ 1., -1 }); | ||
| // output->x = 2.; | ||
| T * operator->() { return &_ptr->_value; } | ||
| T const * operator->() const { return &_ptr->_value; } |
| std::mutex m; | ||
| // Following will issue (from CppCheck): | ||
| // warning: The lock is ineffective because the mutex is locked at the same scope as the mutex itself. [localMutex] | ||
| std::unique_lock< std::mutex > locker( m ); |
There was a problem hiding this comment.
I think you need to lock the mutex on signal() too
There was a problem hiding this comment.
It's up to the user to write the lambda with concurrency in mind, if that's what's needed (for example, in dispatcher.cpp it's not needed)
| //Go over the sensors and stop them | ||
| size_t active_sensors_count = m_active_sensors.size(); | ||
| for (size_t i = 0; i<active_sensors_count; i++) | ||
| std::vector<std::shared_ptr<playback_sensor>> playback_sensors_copy; |
There was a problem hiding this comment.
Add comment why do we need local copy of playback_sensors
There was a problem hiding this comment.
// Stopping the sensor will call another function which will remove the sensor from the
// list of active sensors, which will cause issues -- so we copy it first
|
|
||
| auto i1 = output->i.load(); | ||
| CHECK( i1 >= 10 ); // the thread is still running! | ||
| CHECK( i1 < 16 ); |
There was a problem hiding this comment.
how can you be sure i1 < 16 if the thread is still running?
There was a problem hiding this comment.
If I expect it to notify at 10, 16 would be 6*50=300ms later.
It's possible for it to fail, but very unlikely: it would mean the thread continued but this thread was paused for that amount of time...
| //std::cout << "i2= " << i2 << std::endl; | ||
|
|
||
| // Wait until it's done, ~30x50ms = 1.5 seconds total | ||
| REQUIRE( output->i < 30 ); |
There was a problem hiding this comment.
Same here the thread keep running and i can be equal to 30, no?
There was a problem hiding this comment.
Same kind of expectation: the other thread has slept between iterations, but this thread has not. For this to fail, something strange must happen.
|
Note that it is still not targeted for |
Tracked on [DSO-16888]