aethercast

Merge lp:~phablet-team/aethercast/block-socket into lp:aethercast

  1. block-socket
  2. Merge into trunk
Proposed by Alfonso Sanchez-Beato
Status: Merged
Approved by: Alfonso Sanchez-Beato
Approved revision: 147
Merged at revision: 144
Proposed branch: lp:~phablet-team/aethercast/block-socket
Merge into: lp:aethercast
Diff against target: 253 lines (+7/-84)
11 files modified
conf/aethercast.conf.in (+4/-1)
src/mcs/network/stream.h (+0/-2)
src/mcs/network/udpstream.cpp (+3/-17)
src/mcs/network/udpstream.h (+0/-2)
src/mcs/networkutils.cpp (+0/-13)
src/mcs/networkutils.h (+0/-1)
src/mcs/streaming/rtpsender.cpp (+0/-3)
tests/mcs/integration_tests/test_stream_performance.cpp (+0/-4)
tests/mcs/mir/sourcemediamanager_tests.cpp (+0/-1)
tests/mcs/networkutils_tests.cpp (+0/-9)
tests/mcs/streaming/rtpsender_tests.cpp (+0/-31)
To merge this branch: bzr merge lp:~phablet-team/aethercast/block-socket
Reviewer Review Type Date Requested Status
Simon Fels Approve
Review via email: mp+294142@code.launchpad.net

Commit message

Some times socket output buffer was full and sending returned an EAGAIN
error. Solved by making the socket blocking, as anyway we have a dedicated
thread for sending and you will not be blocked in a datagram socket for
long. LP: #1579773.

Description of the change

Some times socket output buffer was full and sending returned an EAGAIN
error. Solved by making the socket blocking, as anyway we have a dedicated
thread for sending and you will not be blocked in a datagram socket for
long. LP: #1579773.

To post a comment you must log in.
lp:~phablet-team/aethercast/block-socket updated
144. By Alfonso Sanchez-Beato

Some times socket output buffer was full and sending returned an EAGAIN error.
Solved by making the socket blocking, as anyway we have a dedicated thread for
sending and you will not be blocked in a datagram socket for long. LP: #1579773.

Revision history for this message
Simon Fels (morphis) wrote :

LGTM. Minor comments inline.

review: Needs Fixing
lp:~phablet-team/aethercast/block-socket updated
145. By Alfonso Sanchez-Beato

Remove unneeded NetworkUtils::MakeSocketNonBlocking and Stream::WaitUntilReady

Revision history for this message
Alfonso Sanchez-Beato (alfonsosanchezbeato) wrote :

Comments addressed. Less is more ;)

Revision history for this message
Simon Fels (morphis) wrote :

LGTM

review: Approve
lp:~phablet-team/aethercast/block-socket updated
146. By Alfonso Sanchez-Beato

Avoid race condition with Android container (LP: #1579777)

147. By Alfonso Sanchez-Beato

Check whether we have an Android HAL in a better way

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'conf/aethercast.conf.in'
2--- conf/aethercast.conf.in 2016-01-11 16:48:27 +0000
3+++ conf/aethercast.conf.in 2016-05-13 16:22:59 +0000
4@@ -6,7 +6,10 @@
5 respawn
6
7 script
8- if [ -x `which getprop` ] ; then
9+ if [ -f /system/build.prop ] ; then
10+ # Wait for Android properties to be set
11+ while [ "$(getprop init.svc.upstart-watcher)" != "running" ]; do sleep 0.2; done
12+
13 iface=`getprop wifi.direct.interface`
14 if [ "$iface" != "" ] ; then
15 export AETHERCAST_DEDICATED_P2P_INTERFACE=$iface
16
17=== modified file 'src/mcs/network/stream.h'
18--- src/mcs/network/stream.h 2016-04-06 07:52:19 +0000
19+++ src/mcs/network/stream.h 2016-05-13 16:22:59 +0000
20@@ -40,8 +40,6 @@
21
22 virtual bool Connect(const std::string &address, const Port &port) = 0;
23
24- virtual bool WaitUntilReady() = 0;
25-
26 virtual Error Write(const uint8_t *data, unsigned int size,
27 const mcs::TimestampUs &timestamp = 0) = 0;
28
29
30=== modified file 'src/mcs/network/udpstream.cpp'
31--- src/mcs/network/udpstream.cpp 2016-04-22 13:49:46 +0000
32+++ src/mcs/network/udpstream.cpp 2016-05-13 16:22:59 +0000
33@@ -70,11 +70,6 @@
34 return false;
35 }
36
37- if (NetworkUtils::MakeSocketNonBlocking(socket_) < 0) {
38- MCS_ERROR("Failed to make socket non blocking");
39- return false;
40- }
41-
42 struct sockaddr_in addr;
43 memset(addr.sin_zero, 0, sizeof(addr.sin_zero));
44 addr.sin_family = AF_INET;
45@@ -107,23 +102,14 @@
46 return true;
47 }
48
49-bool UdpStream::WaitUntilReady() {
50- fd_set fds;
51- FD_ZERO(&fds);
52- FD_SET(socket_, &fds);
53-
54- const int ret = ::select(socket_ + 1, nullptr, &fds, nullptr, nullptr);
55- if (ret < 0 || !FD_ISSET(socket_, &fds))
56- return false;
57-
58- return true;
59-}
60-
61 Stream::Error UdpStream::Write(const uint8_t *data, unsigned int size,
62 const mcs::TimestampUs &timestamp) {
63
64 boost::ignore_unused_variable_warning(timestamp);
65
66+ // Note this is a blocking socket. However, this is a datagram socket and
67+ // any blocking due to a full sending buffer will be very short. Also, we
68+ // have a dedicated thread to call Write().
69 auto bytes_sent = ::send(socket_, data, size, 0);
70
71 // If we get an error back which relates to a possible congested
72
73=== modified file 'src/mcs/network/udpstream.h'
74--- src/mcs/network/udpstream.h 2016-04-06 07:52:19 +0000
75+++ src/mcs/network/udpstream.h 2016-05-13 16:22:59 +0000
76@@ -34,8 +34,6 @@
77
78 bool Connect(const std::string &address, const Port &port) override;
79
80- bool WaitUntilReady() override;
81-
82 Error Write(const uint8_t *data, unsigned int size,
83 const mcs::TimestampUs &timestamp = 0) override;
84
85
86=== modified file 'src/mcs/networkutils.cpp'
87--- src/mcs/networkutils.cpp 2016-03-30 08:16:06 +0000
88+++ src/mcs/networkutils.cpp 2016-05-13 16:22:59 +0000
89@@ -327,17 +327,4 @@
90 return static_cast<mcs::network::Port>(distribution(generator));
91 }
92
93-int NetworkUtils::MakeSocketNonBlocking(int socket) {
94- if (socket < 0)
95- return -EINVAL;
96-
97- int flags = fcntl(socket, F_GETFL, 0);
98- if (flags < 0)
99- flags = 0;
100- const int res = fcntl(socket, F_SETFL, flags | O_NONBLOCK);
101- if (res < 0)
102- return -errno;
103- return 0;
104-}
105-
106 } // namespace mcs
107
108=== modified file 'src/mcs/networkutils.h'
109--- src/mcs/networkutils.h 2016-03-30 08:13:42 +0000
110+++ src/mcs/networkutils.h 2016-05-13 16:22:59 +0000
111@@ -38,7 +38,6 @@
112 static int BytesAvailableToRead(int fd);
113 static int SendDriverPrivateCommand(const std::string &ifname, const std::string &cmd);
114 static mcs::network::Port PickRandomPort();
115- static int MakeSocketNonBlocking(int socket);
116 };
117 } // namespace mcs
118 #endif
119
120=== modified file 'src/mcs/streaming/rtpsender.cpp'
121--- src/mcs/streaming/rtpsender.cpp 2016-04-22 10:41:47 +0000
122+++ src/mcs/streaming/rtpsender.cpp 2016-05-13 16:22:59 +0000
123@@ -68,9 +68,6 @@
124 if (!queue_->WaitToBeFilled())
125 return true;
126
127- if (!stream_->WaitUntilReady())
128- return true;
129-
130 queue_->Lock();
131
132 while(true) {
133
134=== modified file 'tests/mcs/integration_tests/test_stream_performance.cpp'
135--- tests/mcs/integration_tests/test_stream_performance.cpp 2016-04-08 14:53:06 +0000
136+++ tests/mcs/integration_tests/test_stream_performance.cpp 2016-05-13 16:22:59 +0000
137@@ -55,7 +55,6 @@
138 class MockStream : public mcs::network::Stream {
139 public:
140 MOCK_METHOD2(Connect, bool(const std::string &address, const mcs::network::Port &port));
141- MOCK_METHOD0(WaitUntilReady, bool());
142 MOCK_METHOD3(Write, mcs::network::Stream::Error(const uint8_t*, unsigned int, const mcs::TimestampUs&));
143 MOCK_CONST_METHOD0(LocalPort, mcs::network::Port());
144 MOCK_CONST_METHOD0(MaxUnitSize, std::uint32_t());
145@@ -108,9 +107,6 @@
146 EXPECT_CALL(*output_stream, Connect(_, _))
147 .WillRepeatedly(Return(true));
148
149- EXPECT_CALL(*output_stream, WaitUntilReady())
150- .WillRepeatedly(Return(true));
151-
152 EXPECT_CALL(*output_stream, Write(_, _, _))
153 .WillRepeatedly(Invoke([&](const uint8_t *data, unsigned int size, const mcs::TimestampUs &timestamp) {
154 boost::ignore_unused_variable_warning(data);
155
156=== modified file 'tests/mcs/mir/sourcemediamanager_tests.cpp'
157--- tests/mcs/mir/sourcemediamanager_tests.cpp 2016-05-06 09:24:31 +0000
158+++ tests/mcs/mir/sourcemediamanager_tests.cpp 2016-05-13 16:22:59 +0000
159@@ -27,7 +27,6 @@
160 class MockOutputStream : public mcs::network::Stream {
161 public:
162 MOCK_METHOD2(Connect, bool(const std::string&, const mcs::network::Port&));
163- MOCK_METHOD0(WaitUntilReady, bool());
164 MOCK_METHOD3(Write, mcs::network::Stream::Error(const uint8_t*, unsigned int, const mcs::TimestampUs&));
165 MOCK_CONST_METHOD0(LocalPort, mcs::network::Port());
166 MOCK_CONST_METHOD0(MaxUnitSize, std::uint32_t());
167
168=== modified file 'tests/mcs/networkutils_tests.cpp'
169--- tests/mcs/networkutils_tests.cpp 2016-03-30 08:13:42 +0000
170+++ tests/mcs/networkutils_tests.cpp 2016-05-13 16:22:59 +0000
171@@ -29,12 +29,3 @@
172 EXPECT_GE(mcs::NetworkUtils::kMaxUserPort, port);
173 }
174 }
175-
176-TEST(NetworkUtils_MakeSocketNonBlocking, SocketFlagsCorrectlySet) {
177- const auto sock = ::socket(AF_INET, SOCK_DGRAM, 0);
178-
179- EXPECT_EQ(0, mcs::NetworkUtils::MakeSocketNonBlocking(sock));
180-
181- const auto flags = ::fcntl(sock, F_GETFL, 0);
182- EXPECT_TRUE(flags & O_NONBLOCK);
183-}
184
185=== modified file 'tests/mcs/streaming/rtpsender_tests.cpp'
186--- tests/mcs/streaming/rtpsender_tests.cpp 2016-04-06 07:53:36 +0000
187+++ tests/mcs/streaming/rtpsender_tests.cpp 2016-05-13 16:22:59 +0000
188@@ -36,7 +36,6 @@
189 class MockNetworkStream : public mcs::network::Stream {
190 public:
191 MOCK_METHOD2(Connect, bool(const std::string &address, const mcs::network::Port &port));
192- MOCK_METHOD0(WaitUntilReady, bool());
193 MOCK_METHOD3(Write, mcs::network::Stream::Error(const uint8_t*, unsigned int, const mcs::TimestampUs&));
194 MOCK_CONST_METHOD0(LocalPort, mcs::network::Port());
195 MOCK_CONST_METHOD0(MaxUnitSize, std::uint32_t());
196@@ -118,25 +117,6 @@
197 EXPECT_TRUE(sender->Execute());
198 }
199
200-TEST(RTPSender, ExecuteDoesNotFailWhenStreamIsNotReady) {
201- auto mock_stream = std::make_shared<MockNetworkStream>();
202- auto mock_report = std::make_shared<MockSenderReport>();
203-
204- EXPECT_CALL(*mock_stream, MaxUnitSize())
205- .WillRepeatedly(Return(kStreamMaxUnitSize));
206-
207- EXPECT_CALL(*mock_stream, WaitUntilReady())
208- .Times(1)
209- .WillOnce(Return(false));
210-
211- auto sender = std::make_shared<mcs::streaming::RTPSender>(mock_stream, mock_report);
212-
213- auto packets = mcs::video::Buffer::Create(kMPEGTSPacketSize);
214-
215- EXPECT_TRUE(sender->Queue(packets));
216- EXPECT_TRUE(sender->Execute());
217-}
218-
219 TEST(RTPSender, QueuesUpPackagesAndSendsAll) {
220 auto mock_stream = std::make_shared<MockNetworkStream>();
221 auto mock_report = std::make_shared<MockSenderReport>();
222@@ -149,10 +129,6 @@
223 EXPECT_CALL(*mock_stream, MaxUnitSize())
224 .WillRepeatedly(Return(kStreamMaxUnitSize));
225
226- EXPECT_CALL(*mock_stream, WaitUntilReady())
227- .Times(1)
228- .WillOnce(Return(true));
229-
230 EXPECT_CALL(*mock_stream, Write(_, _, _))
231 .Times(3)
232 .WillRepeatedly(Return(mcs::network::Stream::Error::kNone));
233@@ -173,10 +149,6 @@
234 EXPECT_CALL(*mock_stream, MaxUnitSize())
235 .WillRepeatedly(Return(kStreamMaxUnitSize));
236
237- EXPECT_CALL(*mock_stream, WaitUntilReady())
238- .Times(1)
239- .WillOnce(Return(true));
240-
241 EXPECT_CALL(*mock_stream, Write(_, _, _))
242 .Times(1)
243 .WillOnce(Return(mcs::network::Stream::Error::kRemoteClosedConnection));
244@@ -203,9 +175,6 @@
245 EXPECT_CALL(*mock_stream, MaxUnitSize())
246 .WillRepeatedly(Return(kStreamMaxUnitSize));
247
248- EXPECT_CALL(*mock_stream, WaitUntilReady())
249- .WillOnce(Return(true));
250-
251 std::uint8_t *output_data = nullptr;
252
253 EXPECT_CALL(*mock_stream, Write(_, expected_output_size, _))

Subscribers

People subscribed via source and target branches