[FFmpeg-devel] [RFC] switch to poll()

[Luca Barbato]

On 11/15/2010 03:18 PM, Martin Storsj? wrote:
> I don't think mallocing a small array for the poll structs is a problem 
> even if we wouldn't use it - it's a much smaller issue in practice than 
> having VLA's in the code, I'd say.

The vla was the quickest way to get the feature w/out adding more lines,
I wouldn't keep it at all.

> In general it looks quite good to me, especially after getting patch #2 to 
> clean up the rtsp stuff. For that patch, perhaps it would be good with a 
> comment saying that care must be taken to keep j in sync with the poll 
> structs.

You are right.

>> diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
>> index 1f55016..77d2873 100644
>> --- a/libavformat/rtsp.c
>> +++ b/libavformat/rtsp.c
>>              for (i = 0; i < rt->nb_rtsp_streams; i++) {
>>                  rtsp_st = rt->rtsp_streams[i];
>>                  if (rtsp_st->rtp_handle) {
>> +                    int j = 1 - (tcp_fd == -1);
>>                      fd = url_get_file_handle(rtsp_st->rtp_handle);
> 
> The initialization of j here, when it will be reinitialized in the for loop below,
> feels strange. I do see what you do with it in patch #2, though, but it isn't
> necessary here yet.

0 is always for the tcp fd if present.

>>                      fd_rtcp = rtp_get_rtcp_file_handle(rtsp_st->rtp_handle);
>> -                    if (FD_ISSET(fd_rtcp, &rfds) || FD_ISSET(fd, &rfds)) {
>> +                    for(j = 1; j<max_p; j++) {
>> +                    if ((p[j].revents & POLLIN) &&
>> +                        (p[j].fd == fd_rtcp || p[j].fd == fd)) {
>>                          ret = url_read(rtsp_st->rtp_handle, buf, buf_size);
>>                          if (ret > 0) {
>>                              *prtsp_st = rtsp_st;
>>                              return ret;
>>                          }
>>                      }
>> +                    }
>>                  }
>>              }
> 
> 
> Do you want me to test run some of the modified codepaths that you haven't 
> tested yourself, e.g. SAP?

Yes please

lu

-- 

Luca Barbato
Gentoo/linux
http://dev.gentoo.org/~lu_zero