[FFmpeg-devel] [PATCH] Replace select with poll

[Martin Storsjö]

On Mon, 24 Jan 2011, Luca Barbato wrote:

> Select has limitations on the fd values it could accept and silently
> breaks when it is reached.

The patch seems to work fine for me in quick tests with RTSP and SAP.

> @@ -1529,55 +1529,52 @@ 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;
> +    //FIXME use malloc
> +    struct pollfd p[2*rt->nb_rtsp_streams+1];

I'm not particularly fond of VLA's, and others have worked hard on getting 
rid of them...

> +            //FIXME this loop should be simplified

Is this comment still valid? During the last review, you simplified it a 
bit, making it not worse than before at least.

>              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) {
>                          ret = url_read(rtsp_st->rtp_handle, buf, buf_size);
>                          if (ret > 0) {
>                              *prtsp_st = rtsp_st;
>                              return ret;
>                          }
>                      }
> +                    j+=2;
>                  }
>              }

This only checks if POLLIN was set on the RTP fd, not on the RTCP one. I 
guess a simple if (p[j].revents & POLLIN || p[j + 1].revents & POLLIN) 
would be sufficient?

> @@ -73,6 +71,7 @@ static int tcp_open(URLContext *h, const char *uri, int flags)
>   redo:
>      ret = connect(fd, cur_ai->ai_addr, cur_ai->ai_addrlen);
>      if (ret < 0) {
> +        struct pollfd p = {fd, POLLOUT, 0};
>          if (ff_neterrno() == FF_NETERROR(EINTR)) {
>              if (url_interrupt_cb())
>                  goto fail1;
> @@ -88,15 +87,8 @@ static int tcp_open(URLContext *h, const char *uri, int flags
>                  ret = AVERROR(EINTR);
>                  goto fail1;
>              }
> -            fd_max = fd;
> -            FD_ZERO(&wfds);
> -            FD_ZERO(&efds);
> -            FD_SET(fd, &wfds);
> -            FD_SET(fd, &efds);
> -            tv.tv_sec = 0;
> -            tv.tv_usec = 100 * 1000;
> -            ret = select(fd_max + 1, NULL, &wfds, &efds, &tv);
> -            if (ret > 0 && (FD_ISSET(fd, &wfds) || FD_ISSET(fd, 
> &efds)))
> +            ret = poll(&p, 1, 100);
> +            if (ret > 0)
>                  break;
>          }
 
Previously, this select checked the fd in both wfds and efds, should se 
set POLLERR in the pollfd above to keep the same behaviour? (I'm not 
off-hand sure if there's a concrete need for it.)

Other than these comments, it looks quite ok.

// Martin