versatica / mediasoup

Cutting Edge WebRTC Video Conferencing
https://mediasoup.org
ISC License
6.27k stars 1.13k forks source link

Worker tests: use unique pointers when possible #1421

Closed ibc closed 4 months ago

ibc commented 4 months ago

(WIP)

Details

ibc commented 4 months ago

Questions

jmillan commented 4 months ago

Why does TestRtpStreamSend.cpp use shared_ptr instead of unique_ptr?

Because a shared_ptr is what the API expects. Check the method being called.

Why does NOT TestRtpStreamSend.cpp use xxx_ptr in cases like this one?

That pointer is owned by the caller of CreateRtpPacket(), which is handled by a unique_ptr

ibc commented 4 months ago

Why does TestRtpStreamSend.cpp use shared_ptr instead of unique_ptr? Because a shared_ptr is what the API expects. Check the method being called.

I don't understand. I literally mean that I replaced some shared_ptr with unique_ptr in that files and it compiles, tests pass and no ASAN warnings (in that test file).

ibc commented 4 months ago

@jmillan, ongoing changes in TestXr.cpp in this PR are producing a SIGSEV when running worker tests (no need to run them with ASAN stuff):

-------------------------------------------------------------------------------
Scenario: RTCP XrDelaySinceLastRt parsing
  create RRT
-------------------------------------------------------------------------------
../../../test/src/RTC/RTCP/TestXr.cpp:131
...............................................................................

../../../test/src/RTC/RTCP/TestXr.cpp:131: FAILED:
  {Unknown expression after the reported line}
due to a fatal error condition:
  SIGSEGV - Segmentation violation signal

===============================================================================
test cases:     39 |     38 passed | 1 failed
assertions: 592664 | 592663 passed | 1 failed

My guess is that we are creating a XR packet and we are passing reports to it created with unique_ptr, however the XR packet will delete those pointers by itself when it's deleted. Does it make sense?

ibc commented 4 months ago

My guess is that we are creating a XR packet and we are passing reports to it created with unique_ptr, however the XR packet will delete those pointers by itself when it's deleted. Does it make sense?

Confirmed, and fixed here https://github.com/versatica/mediasoup/pull/1421/commits/281054c40476ab4fa0f91f68a566f7766e7eb752

jmillan commented 4 months ago

@ibc, this PR is ready to merge, plus there are 0 issues when running make test-asan-address

ibc commented 4 months ago

My aim in this PR is to use unique/share_ptr in all test files, not just in TestXr.cpp.

ibc commented 4 months ago

@jmillan thanks a lot for your commits. Just one thing, we don't need to include <memory>, it's already done in common.hpp.

ibc commented 4 months ago

This is done. I've enabled test-asan-address in CI (only for Linux) and will merge if CI passes.