Fix no composite frame on playback (from Nir) by maloel · Pull Request #9210 · realsenseai/librealsense

maloel · 2021-06-12T14:46:31Z

add actual matchers to playback device
live-test issues with playback
added !mayfail to frame-number syncing test for stability
added waiting_on<> in utilities/time/waiting-on.h, with unit-test
rework dispatcher::flush()

Tracked on [DSO-16888]

ev-mp

see inline

ev-mp · 2021-06-13T08:33:22Z

+template< class T >
+class waiting_on


The class seem to mimic std::future, why is it needed?

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.

ev-mp · 2021-06-13T08:35:26Z

+            if( auto wait_state = still_alive() )
+                wait_state->signal( t );


The operation needs to be atomic to avoid race cond.

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.

aangerma · 2021-06-13T10:04:41Z

+    //             break;
+    //     }
+    template < class U, class L >
+    void wait_until( U const& timeout, L const& pred )


I think its better if wait_until will return bool - true = pred , false =timeout

I prefer with no return value to make the code more readable

aangerma · 2021-06-13T10:29:05Z

+
+        // Wake up the local function (which is using wait_until(), presumable) with a new
+        // T value
+        void signal( T const& t ) const


Consider to change it to safe_signal() , I think its clear what is the added value of this.

Nothing makes it safe... it's just a convenience

aangerma · 2021-06-13T10:37:16Z

+        {
+        }
+
+        operator T &() { return _value; }


we want to enable to change the value without signal?

Sure, the set-and-signal function is just a convenience function

aangerma · 2021-06-13T10:42:33Z

+    // Convert to the in-thread representation
+    in_thread_ in_thread() const { return in_thread_( *this ); }
+
+    operator T const &() const { return _ptr->_value; }


can't use operator T &() of wait_state_t?

Changed to *_ptr instead of _ptr->_value

aangerma · 2021-06-13T10:42:47Z

+    // struct value_t { double x; int k; };
+    // waiting_on< value_t > output({ 1., -1 });
+    // output->x = 2.;
+    T * operator->() { return &_ptr->_value; }


No easy syntax that can be used here

aangerma · 2021-06-13T10:43:20Z

+    // waiting_on< value_t > output({ 1., -1 });
+    // output->x = 2.;
+    T * operator->() { return &_ptr->_value; }
+    T const * operator->() const { return &_ptr->_value; }


aangerma · 2021-06-13T10:44:56Z

+        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 );


I think you need to lock the mutex on signal() too

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)

aangerma · 2021-06-13T12:06:47Z

-            //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;


Add comment why do we need local copy of playback_sensors

// 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

aangerma · 2021-06-13T13:58:05Z

+
+    auto i1 = output->i.load();
+    CHECK( i1 >= 10 ); // the thread is still running!
+    CHECK( i1 < 16 );


how can you be sure i1 < 16 if the thread is still running?

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...

aangerma · 2021-06-13T14:01:27Z

+    //std::cout << "i2= " << i2 << std::endl;
+
+    // Wait until it's done, ~30x50ms = 1.5 seconds total
+    REQUIRE( output->i < 30 );


Same here the thread keep running and i can be equal to 30, no?

Same kind of expectation: the other thread has slept between iterations, but this thread has not. For this to fail, something strange must happen.

ev-mp

Approved

ev-mp · 2021-06-13T14:43:05Z

Note that it is still not targeted for dev

maloel added 8 commits June 12, 2021 17:22

provide usable playback device matchers that actually sync

1be0b8c

enable frame_to_string() on a null frame

e225f6d

add !mayfail to 'Syncer try wait for frames' because of known issues

cf39d69

fix no composite frames on playback (from Nir)

b4be454

dispatcher debug & flush() rework (from Nir)

de9d7ec

added waiting_on<> in utilities/time/waiting-on.h

2361e8b

dispatcher::flush() now uses waiting_on<>

a0438e1

fixes in post-processing-from-bag.cpp

5b60cd8

maloel requested review from Nir-Az, aangerma and ev-mp June 12, 2021 14:46

CR review (mine)

2e7a30e

ev-mp reviewed Jun 13, 2021

View reviewed changes

aangerma reviewed Jun 13, 2021

View reviewed changes

ev-mp approved these changes Jun 13, 2021

View reviewed changes

CR changes; added another unit-test for waiting_on<>

ac03d9c

maloel merged commit 522605c into realsenseai:concsync Jun 13, 2021

maloel mentioned this pull request Jun 13, 2021

Concurrency & Syncer rework #9219

Merged

maloel deleted the nirs2 branch December 22, 2021 07:36

		if( auto wait_state = still_alive() )
		wait_state->signal( t );

Uh oh!

Conversation

maloel commented Jun 12, 2021

Uh oh!

ev-mp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ev-mp left a comment

Choose a reason for hiding this comment

Uh oh!

ev-mp commented Jun 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants