Remove lock for frame delay cacluation #305
Conversation
harryz2000
changed the title
|
I think it would be easier to add a TimeService to the coordinator and call the OnFrame and SetMaxDelayMs on an Async call. The MediaFrameBridgeListener call is already thread safe. |
Yes. That would solve the thread lock issue. But to me, it seems not ideal as the calculated delay may not be used for current frame. For cases such as receive a batch of audio frames, they may still use a same delay, which might not be smooth. Although, in practice, it might not be a issue. I am trying to see if it could be solved by another way. Got some progress, will need spend a bit more time to ensure it works. |
|
This should be a draft full solution for this issue. There is still a test to fix. But it should work. It calculates the delay in synchronously without lock. On the other hand it reduces latency asynchronously if all frames come early. Now the OnFrame is thread safe. Yes. It has been a bit harder to implement this. But imho it suits better for our expectation. |
|
@murillo128 Now all unit tests have passed. I did some manual tests well. It works well to me, so I think it is ready for formal review. Personally, I would prefer this solution. It actually becomes cleaner now: One part is for delay calculation, which is lock-free sync operations. Another part for latency reduction, which uses async calls. The test is using a TestTimeService which does sync calls for Async(), therefore most of the tests passed. The only test failed and has been fixed is the latency reduction may be delayed by one frame, as we are not reduce the delay immediately. However it shouldn't matter. |
|
@murillo128 Please have a look at this commit: 4ec2a6e |
| #include <unordered_map> | ||
| #include <optional> | ||
| #include <chrono> | ||
| #include <queue> | ||
| #include <algorithm> | ||
|
|
||
| class FrameDelayCalculator | ||
| class FrameDelayCalculator : public std::enable_shared_from_this<FrameDelayCalculator> |
There was a problem hiding this comment.
maybe irrelevant, what's the benefit of inheriting enabled_shared_from_this? i dont see shared_from_this or weak_from_this being used in framedelaycalculator.
There was a problem hiding this comment.
We do use the weak_from_this() in the implement file. See line 91 in src/FrameDelayCalculator.cpp
timeService.Async([selfWeak = weak_from_this(), early, now, unifiedTs, refTime, refTimestamp,
No description provided.