[FFmpeg-devel] [PATCH 1/2] os: replace select with poll

Fri Jan 28 03:59:44 CET 2011

Hi Luca,

On 27 January 2011 18:12, Luca Barbato <lu_zero at gentoo.org> wrote:
> Select has limitations on the fd values it could accept and silently
> breaks when it is reached.
> ---
> ?libavformat/os_support.c | ? ?3 --
> ?libavformat/rtpproto.c ? | ? 24 ++++++-------------
> ?libavformat/rtsp.c ? ? ? | ? 55 +++++++++++++++++++++++++---------------------
> ?libavformat/rtsp.h ? ? ? | ? ?5 ++++
> ?libavformat/rtspenc.c ? ?| ? 19 +++++-----------
> ?libavformat/sapdec.c ? ? | ? 14 ++++-------
> ?libavformat/tcp.c ? ? ? ?| ? 48 +++++++++++----------------------------
> ?libavformat/udp.c ? ? ? ?| ? 15 ++++--------
> ?8 files changed, 73 insertions(+), 110 deletions(-)
>
> diff --git a/libavformat/os_support.c b/libavformat/os_support.c
> index 83f0820..70cca92 100644
> --- a/libavformat/os_support.c
> +++ b/libavformat/os_support.c
> @@ -236,7 +236,6 @@ int ff_socket_nonblock(int socket, int enable)
> ?}
> ?#endif /* CONFIG_NETWORK */
>
> -#if CONFIG_FFSERVER
> ?#if !HAVE_POLL_H
> ?int poll(struct pollfd *fds, nfds_t numfds, int timeout)
> ?{
> @@ -305,5 +304,3 @@ int poll(struct pollfd *fds, nfds_t numfds, int timeout)
> ? ? return rc;
> ?}
> ?#endif /* HAVE_POLL_H */
> -#endif /* CONFIG_FFSERVER */
> -
> diff --git a/libavformat/rtpproto.c b/libavformat/rtpproto.c
> index 6ef6784..aa2cc37 100644
> --- a/libavformat/rtpproto.c
> +++ b/libavformat/rtpproto.c
> @@ -34,8 +34,8 @@
> ?#include "network.h"
> ?#include "os_support.h"
> ?#include <fcntl.h>
> -#if HAVE_SYS_SELECT_H
> -#include <sys/select.h>
> +#if HAVE_POLL_H
> +#include <sys/poll.h>
> ?#endif
> ?#include <sys/time.h>
>
> @@ -221,9 +221,9 @@ static int rtp_read(URLContext *h, uint8_t *buf, int size)
> ? ? RTPContext *s = h->priv_data;
> ? ? struct sockaddr_storage from;
> ? ? socklen_t from_len;
> - ? ?int len, fd_max, n;
> - ? ?fd_set rfds;
> - ? ?struct timeval tv;
> + ? ?int len, n;
> + ? ?struct pollfd p[2] = {{s->rtp_fd, POLLIN, 0}, {s->rtcp_fd, POLLIN, 0}};
> +
> ?#if 0
> ? ? for(;;) {
> ? ? ? ? from_len = sizeof(from);
> @@ -242,18 +242,10 @@ static int rtp_read(URLContext *h, uint8_t *buf, int size)
> ? ? ? ? if (url_interrupt_cb())
> ? ? ? ? ? ? return AVERROR(EINTR);
> ? ? ? ? /* build fdset to listen to RTP and RTCP packets */
> - ? ? ? ?FD_ZERO(&rfds);
> - ? ? ? ?fd_max = s->rtp_fd;
> - ? ? ? ?FD_SET(s->rtp_fd, &rfds);
> - ? ? ? ?if (s->rtcp_fd > fd_max)
> - ? ? ? ? ? ?fd_max = s->rtcp_fd;
> - ? ? ? ?FD_SET(s->rtcp_fd, &rfds);
> - ? ? ? ?tv.tv_sec = 0;
> - ? ? ? ?tv.tv_usec = 100 * 1000;
> - ? ? ? ?n = select(fd_max + 1, &rfds, NULL, NULL, &tv);
> + ? ? ? ?n = poll(p, 2, 100);
> ? ? ? ? if (n > 0) {
> ? ? ? ? ? ? /* first try RTCP */
> - ? ? ? ? ? ?if (FD_ISSET(s->rtcp_fd, &rfds)) {
> + ? ? ? ? ? ?if (p[1].revents & POLLIN) {
> ? ? ? ? ? ? ? ? from_len = sizeof(from);
> ? ? ? ? ? ? ? ? len = recvfrom (s->rtcp_fd, buf, size, 0,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? (struct sockaddr *)&from, &from_len);
> @@ -266,7 +258,7 @@ static int rtp_read(URLContext *h, uint8_t *buf, int size)
> ? ? ? ? ? ? ? ? break;
> ? ? ? ? ? ? }
> ? ? ? ? ? ? /* then RTP */
> - ? ? ? ? ? ?if (FD_ISSET(s->rtp_fd, &rfds)) {
> + ? ? ? ? ? ?if (p[0].revents & POLLIN) {
> ? ? ? ? ? ? ? ? from_len = sizeof(from);
> ? ? ? ? ? ? ? ? len = recvfrom (s->rtp_fd, buf, size, 0,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? (struct sockaddr *)&from, &from_len);
> diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
> index 752f429..a56ff99 100644
> --- a/libavformat/rtsp.c
> +++ b/libavformat/rtsp.c
> @@ -26,8 +26,8 @@
> ?#include "avformat.h"
>
> ?#include <sys/time.h>
> -#if HAVE_SYS_SELECT_H
> -#include <sys/select.h>
> +#if HAVE_POLL_H
> +#include <poll.h>
> ?#endif
> ?#include <strings.h>
> ?#include "internal.h"
> @@ -44,11 +44,11 @@
> ?//#define DEBUG
> ?//#define DEBUG_RTP_TCP
>
> -/* Timeout values for socket select, in ms,
> +/* Timeout values for socket poll, in ms,
> ?* and read_packet(), in seconds ?*/
> -#define SELECT_TIMEOUT_MS 100
> +#define POLL_TIMEOUT_MS 100
> ?#define READ_PACKET_TIMEOUT_S 10
> -#define MAX_TIMEOUTS READ_PACKET_TIMEOUT_S * 1000 / SELECT_TIMEOUT_MS
> +#define MAX_TIMEOUTS READ_PACKET_TIMEOUT_S * 1000 / POLL_TIMEOUT_MS
> ?#define SDP_MAX_SIZE 16384
> ?#define RECVBUF_SIZE 10 * RTP_MAX_PACKET_LENGTH
>
> @@ -429,8 +429,14 @@ static void sdp_parse_line(AVFormatContext *s, SDPParseState *s1,
> ? ? }
> ?}
>
> +/**
> + * Parse the sdp description and allocate the rtp streams and the
> + * pollfd array used for udp ones.
> + */
> +
> ?int ff_sdp_parse(AVFormatContext *s, const char *content)
> ?{
> + ? ?RTSPState *rt = s->priv_data;
> ? ? const char *p;
> ? ? int letter;
> ? ? /* Some SDP lines, particularly for Realmedia or ASF RTSP streams,
> @@ -470,6 +476,8 @@ int ff_sdp_parse(AVFormatContext *s, const char *content)
> ? ? ? ? if (*p == '\n')
> ? ? ? ? ? ? p++;
> ? ? }
> + ? ?rt->p = av_malloc(sizeof(struct pollfd)*2*(rt->nb_rtsp_streams+1));
> + ? ?if (!rt->p) return AVERROR(ENOMEM);
> ? ? return 0;
> ?}

This seems a strange place to allocate it, but I can't see a better
place to do it.

> ?#endif /* CONFIG_RTPDEC */
> @@ -531,6 +539,7 @@ void ff_rtsp_close_streams(AVFormatContext *s)
> ? ? ? ? av_close_input_stream (rt->asf_ctx);
> ? ? ? ? rt->asf_ctx = NULL;
> ? ? }
> + ? ?av_free(rt->p);
> ? ? av_free(rt->recvbuf);
> ?}
>
> @@ -1554,55 +1563,51 @@ static int udp_read_packet(AVFormatContext *s, RTSPStream **prtsp_st,
> ?{
> ? ? RTSPState *rt = s->priv_data;
> ? ? RTSPStream *rtsp_st;
> - ? ?fd_set rfds;
> - ? ?int fd, fd_rtcp, fd_max, n, i, ret, tcp_fd, timeout_cnt = 0;
> - ? ?struct timeval tv;
> + ? ?int n, i, ret, tcp_fd, timeout_cnt = 0;
> + ? ?int max_p = 0;
> + ? ?struct pollfd *p = rt->p;
>
> ? ? for (;;) {
> ? ? ? ? if (url_interrupt_cb())
> ? ? ? ? ? ? return AVERROR(EINTR);
> ? ? ? ? if (wait_end && wait_end - av_gettime() < 0)
> ? ? ? ? ? ? return AVERROR(EAGAIN);
> - ? ? ? ?FD_ZERO(&rfds);
> + ? ? ? ?max_p = 0;
> ? ? ? ? if (rt->rtsp_hd) {
> - ? ? ? ? ? ?tcp_fd = fd_max = url_get_file_handle(rt->rtsp_hd);
> - ? ? ? ? ? ?FD_SET(tcp_fd, &rfds);
> + ? ? ? ? ? ?tcp_fd = url_get_file_handle(rt->rtsp_hd);
> + ? ? ? ? ? ?p[max_p].fd = tcp_fd;
> + ? ? ? ? ? ?p[max_p++].events = POLLIN;
> ? ? ? ? } else {
> - ? ? ? ? ? ?fd_max = 0;
> ? ? ? ? ? ? tcp_fd = -1;
> ? ? ? ? }
> ? ? ? ? for (i = 0; i < rt->nb_rtsp_streams; i++) {
> ? ? ? ? ? ? rtsp_st = rt->rtsp_streams[i];
> ? ? ? ? ? ? if (rtsp_st->rtp_handle) {
> - ? ? ? ? ? ? ? ?fd = url_get_file_handle(rtsp_st->rtp_handle);
> - ? ? ? ? ? ? ? ?fd_rtcp = rtp_get_rtcp_file_handle(rtsp_st->rtp_handle);
> - ? ? ? ? ? ? ? ?if (FFMAX(fd, fd_rtcp) > fd_max)
> - ? ? ? ? ? ? ? ? ? ?fd_max = FFMAX(fd, fd_rtcp);
> - ? ? ? ? ? ? ? ?FD_SET(fd, &rfds);
> - ? ? ? ? ? ? ? ?FD_SET(fd_rtcp, &rfds);
> + ? ? ? ? ? ? ? ?p[max_p].fd = url_get_file_handle(rtsp_st->rtp_handle);
> + ? ? ? ? ? ? ? ?p[max_p++].events = POLLIN;
> + ? ? ? ? ? ? ? ?p[max_p].fd = rtp_get_rtcp_file_handle(rtsp_st->rtp_handle);
> + ? ? ? ? ? ? ? ?p[max_p++].events = POLLIN;
> ? ? ? ? ? ? }
> ? ? ? ? }
> - ? ? ? ?tv.tv_sec = 0;
> - ? ? ? ?tv.tv_usec = SELECT_TIMEOUT_MS * 1000;
> - ? ? ? ?n = select(fd_max + 1, &rfds, NULL, NULL, &tv);
> + ? ? ? ?n = poll(p, max_p, POLL_TIMEOUT_MS);
> ? ? ? ? if (n > 0) {
> + ? ? ? ? ? ?int j = 1 - (tcp_fd == -1);
> ? ? ? ? ? ? timeout_cnt = 0;
> ? ? ? ? ? ? for (i = 0; i < rt->nb_rtsp_streams; i++) {
> ? ? ? ? ? ? ? ? rtsp_st = rt->rtsp_streams[i];
> ? ? ? ? ? ? ? ? if (rtsp_st->rtp_handle) {
> - ? ? ? ? ? ? ? ? ? ?fd = url_get_file_handle(rtsp_st->rtp_handle);
> - ? ? ? ? ? ? ? ? ? ?fd_rtcp = rtp_get_rtcp_file_handle(rtsp_st->rtp_handle);
> - ? ? ? ? ? ? ? ? ? ?if (FD_ISSET(fd_rtcp, &rfds) || FD_ISSET(fd, &rfds)) {
> + ? ? ? ? ? ? ? ? ? ?if (p[j].revents & POLLIN || p[j+1].revents & POLLIN) {

This looks fine, but I've always thought this bit of logic -- reaching
outside of the transport protocol to determine fd readiness -- was a
little smelly. That is a discussion for another time, though.

Nice work.

Josh