Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PLI: handle picture loss rtcp packet #308

Merged
merged 2 commits into from
Mar 30, 2020
Merged

Conversation

zhuker
Copy link
Contributor

@zhuker zhuker commented Mar 27, 2020

Issue #, if available:
handle received rtcp PLI packet

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov-io
Copy link

Codecov Report

Merging #308 into master will decrease coverage by 32.73%.
The diff coverage is 86.36%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #308       +/-   ##
===========================================
- Coverage   86.84%   54.11%   -32.74%     
===========================================
  Files          33       33               
  Lines        7980     8002       +22     
===========================================
- Hits         6930     4330     -2600     
- Misses       1050     3672     +2622     
Impacted Files Coverage Δ
src/source/PeerConnection/Rtcp.c 51.72% <86.36%> (+21.16%) ⬆️
src/source/Ice/TurnConnection.c 1.09% <0.00%> (-90.13%) ⬇️
src/source/Signaling/ChannelInfo.c 0.00% <0.00%> (-88.49%) ⬇️
src/source/Signaling/Signaling.c 10.54% <0.00%> (-83.00%) ⬇️
src/source/Signaling/LwsApiCalls.c 1.55% <0.00%> (-82.99%) ⬇️
src/source/Rtp/Codecs/RtpVP8Payloader.c 0.00% <0.00%> (-80.00%) ⬇️
src/source/Signaling/StateMachine.c 0.00% <0.00%> (-75.58%) ⬇️
src/source/PeerConnection/Rtp.c 28.66% <0.00%> (-44.00%) ⬇️
src/source/Ice/SocketConnection.c 42.93% <0.00%> (-32.77%) ⬇️
src/source/Signaling/Client.c 70.37% <0.00%> (-29.63%) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a60bf9...b1d3a9b. Read the comment docs.

@zhuker
Copy link
Contributor Author

zhuker commented Mar 27, 2020

there's something wrong with codecov :-\

@Sean-Der
Copy link
Contributor

@zhuker this is downright amazing work, thank you so much! Just a few minor fixes and we can get merged today for sure :)

re: codecoverage, we don't run all tests for outside contributors. Only members of awslabs run tests that hit our backend, that is just to make sure someone doesn't open a PR that dumps credentials etc..

Only OSX Clang we have a build failure here

/Users/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/tst/RtcpFunctionalityTest.cpp:185:31: error: missing field 'receptionReportCount' initializer [-Werror,-Wmissing-field-initializers]
    RtcpPacket rtcpPacket = {0};
                              ^
/Users/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/tst/RtcpFunctionalityTest.cpp:185:30: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces]
    RtcpPacket rtcpPacket = {0};
                             ^
                             {}
/Users/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/tst/RtcpFunctionalityTest.cpp:185:31: error: missing field 'payload' initializer [-Werror,-Wmissing-field-initializers]
    RtcpPacket rtcpPacket = {0};
                              ^
/Users/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/tst/RtcpFunctionalityTest.cpp:189:30: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces]
    KvsPeerConnection kpc = {0};
                             ^
                             {}
/Users/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/tst/RtcpFunctionalityTest.cpp:189:31: error: missing field 'pIceAgent' initializer [-Werror,-Wmissing-field-initializers]
    KvsPeerConnection kpc = {0};

On ASAN we have a leak here

==21043==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 24 byte(s) in 1 object(s) allocated from:
    #0 0x5830ca in calloc /tmp/final/llvm.src/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:155:3
    #1 0x7fb5968bef7c in defaultMemCalloc /home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/open-source/amazon-kinesis-video-streams-pic/src/utils/src/Allocators.c:26:12
    #2 0x7fb5968bf8a6 in doubleListCreate /home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/open-source/amazon-kinesis-video-streams-pic/src/utils/src/DoubleLinkedList.c:14:27
    #3 0x652995 in com::amazonaws::kinesis::video::webrtcclient::RtcpFunctionalityTest_onpli_Test::TestBody() /home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/tst/RtcpFunctionalityTest.cpp:190:5
    #4 0x78b6bd in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x78b6bd)
    #5 0x7759da in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x7759da)
    #6 0x75cb0b in testing::Test::Run() (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x75cb0b)
    #7 0x75d838 in testing::TestInfo::Run() (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x75d838)
    #8 0x75defe in testing::TestCase::Run() (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x75defe)
    #9 0x765680 in testing::internal::UnitTestImpl::RunAllTests() (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x765680)
    #10 0x78ec8d in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x78ec8d)
    #11 0x777e0a in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x777e0a)
    #12 0x765352 in testing::UnitTest::Run() (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x765352)
    #13 0x793590 in RUN_ALL_TESTS() (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x793590)
    #14 0x79356b in main (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x79356b)
    #15 0x7fb594a0c82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
Indirect leak of 24 byte(s) in 1 object(s) allocated from:
    #0 0x5830ca in calloc /tmp/final/llvm.src/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:155:3
    #1 0x7fb5968bef7c in defaultMemCalloc /home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/open-source/amazon-kinesis-video-streams-pic/src/utils/src/Allocators.c:26:12
    #2 0x7fb5968c0281 in doubleListAllocNode /home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/open-source/amazon-kinesis-video-streams-pic/src/utils/src/DoubleLinkedList.c:424:47
    #3 0x7fb5968c009a in doubleListInsertItemHead /home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/open-source/amazon-kinesis-video-streams-pic/src/utils/src/DoubleLinkedList.c:103:5
    #4 0x652b6e in com::amazonaws::kinesis::video::webrtcclient::RtcpFunctionalityTest_onpli_Test::TestBody() /home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/tst/RtcpFunctionalityTest.cpp:197:5
    #5 0x78b6bd in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x78b6bd)
    #6 0x7759da in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x7759da)
    #7 0x75cb0b in testing::Test::Run() (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x75cb0b)
    #8 0x75d838 in testing::TestInfo::Run() (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x75d838)
    #9 0x75defe in testing::TestCase::Run() (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x75defe)
    #10 0x765680 in testing::internal::UnitTestImpl::RunAllTests() (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x765680)
    #11 0x78ec8d in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x78ec8d)
    #12 0x777e0a in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x777e0a)
    #13 0x765352 in testing::UnitTest::Run() (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x765352)
    #14 0x793590 in RUN_ALL_TESTS() (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x793590)
    #15 0x79356b in main (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x79356b)
    #16 0x7fb594a0c82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
SUMMARY: AddressSanitizer: 48 byte(s) leaked in 2 allocation(s).

Copy link
Contributor

@MushMal MushMal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is solid work! Comment about alignment

src/source/PeerConnection/Rtcp.c Outdated Show resolved Hide resolved
@zhuker
Copy link
Contributor Author

zhuker commented Mar 28, 2020 via email

@MushMal
Copy link
Contributor

MushMal commented Mar 28, 2020

At kids birthday today, will fix on Sunday or Monday. Do you have a simple example of passing c++ lambda to C code as a callback which does not look uglier than what I wrote?

On Fri, Mar 27, 2020, 11:59 Sean DuBois @.> wrote: @zhuker https://github.com/zhuker this is downright amazing work, thank you so much! Just a few minor fixes and we can get merged today for sure :) re: codecoverage, we don't run all tests for outside contributors. Only members of awslabs run tests that hit our backend, that is just to make sure someone doesn't open a PR that dumps credentials etc.. Only OSX Clang we have a build failure here https://travis-ci.com/github/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/jobs/305750434 /Users/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/tst/RtcpFunctionalityTest.cpp:185:31: error: missing field 'receptionReportCount' initializer [-Werror,-Wmissing-field-initializers] RtcpPacket rtcpPacket = {0}; ^ /Users/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/tst/RtcpFunctionalityTest.cpp:185:30: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces] RtcpPacket rtcpPacket = {0}; ^ {} /Users/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/tst/RtcpFunctionalityTest.cpp:185:31: error: missing field 'payload' initializer [-Werror,-Wmissing-field-initializers] RtcpPacket rtcpPacket = {0}; ^ /Users/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/tst/RtcpFunctionalityTest.cpp:189:30: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces] KvsPeerConnection kpc = {0}; ^ {} /Users/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/tst/RtcpFunctionalityTest.cpp:189:31: error: missing field 'pIceAgent' initializer [-Werror,-Wmissing-field-initializers] KvsPeerConnection kpc = {0}; On ASAN we have a leak here https://travis-ci.com/github/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/jobs/305750436 ==21043==ERROR: LeakSanitizer: detected memory leaks Direct leak of 24 byte(s) in 1 object(s) allocated from: #0 0x5830ca in calloc /tmp/final/llvm.src/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:155:3 #1 0x7fb5968bef7c in defaultMemCalloc /home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/open-source/amazon-kinesis-video-streams-pic/src/utils/src/Allocators.c:26:12 #2 0x7fb5968bf8a6 in doubleListCreate /home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/open-source/amazon-kinesis-video-streams-pic/src/utils/src/DoubleLinkedList.c:14:27 #3 0x652995 in com::amazonaws::kinesis::video::webrtcclient::RtcpFunctionalityTest_onpli_Test::TestBody() /home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/tst/RtcpFunctionalityTest.cpp:190:5 #4 0x78b6bd in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test, void (testing::Test::)(), char const) (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x78b6bd) #5 0x7759da in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::)(), char const) (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x7759da) #6 0x75cb0b in testing::Test::Run() (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x75cb0b) #7 0x75d838 in testing::TestInfo::Run() (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x75d838) #8 0x75defe in testing::TestCase::Run() (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x75defe) #9 0x765680 in testing::internal::UnitTestImpl::RunAllTests() (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x765680) #10 0x78ec8d in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::)(), char const) (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x78ec8d) #11 0x777e0a in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::)(), char const) (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x777e0a) #12 0x765352 in testing::UnitTest::Run() (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x765352) #13 0x793590 in RUN_ALL_TESTS() (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x793590) #14 0x79356b in main (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x79356b) #15 0x7fb594a0c82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f) Indirect leak of 24 byte(s) in 1 object(s) allocated from: #0 0x5830ca in calloc /tmp/final/llvm.src/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:155:3 #1 0x7fb5968bef7c in defaultMemCalloc /home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/open-source/amazon-kinesis-video-streams-pic/src/utils/src/Allocators.c:26:12 #2 0x7fb5968c0281 in doubleListAllocNode /home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/open-source/amazon-kinesis-video-streams-pic/src/utils/src/DoubleLinkedList.c:424:47 #3 0x7fb5968c009a in doubleListInsertItemHead /home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/open-source/amazon-kinesis-video-streams-pic/src/utils/src/DoubleLinkedList.c:103:5 #4 0x652b6e in com::amazonaws::kinesis::video::webrtcclient::RtcpFunctionalityTest_onpli_Test::TestBody() /home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/tst/RtcpFunctionalityTest.cpp:197:5 #5 0x78b6bd in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::)(), char const) (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x78b6bd) #6 0x7759da in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::)(), char const) (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x7759da) #7 0x75cb0b in testing::Test::Run() (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x75cb0b) #8 0x75d838 in testing::TestInfo::Run() (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x75d838) #9 0x75defe in testing::TestCase::Run() (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x75defe) #10 0x765680 in testing::internal::UnitTestImpl::RunAllTests() (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x765680) #11 0x78ec8d in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::)(), char const) (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x78ec8d) #12 0x777e0a in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::)(), char const) (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x777e0a) #13 0x765352 in testing::UnitTest::Run() (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x765352) #14 0x793590 in RUN_ALL_TESTS() (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x793590) #15 0x79356b in main (/home/travis/build/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/build/tst/webrtc_client_test+0x79356b) #16 0x7fb594a0c82f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f) SUMMARY: AddressSanitizer: 48 byte(s) leaked in 2 allocation(s). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#308 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABBKKQUBSKKAXDJXG35IVFDRJTZQ3ANCNFSM4LVD3WLQ .

Something like this: https://github.com/awslabs/amazon-kinesis-video-streams-webrtc-sdk-c/blob/master/tst/WebRTCClientTestFixture.cpp#L154

@Sean-Der Sean-Der merged commit c024174 into awslabs:master Mar 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants