Created attachment 101541 [details] [review]
first patch

First attempt. Based on a rewritten GstFDSet by Peter Kjellerstedt, renamed to GstPoll, to avoid conflict with GstFDSet in -base. Changed by me to add a minimal set of methods, one to restart and one to start/stop flushing.

Shouldn't this kind of thing go in glib?

(We'll pretend that gstreamer picks up new stuff in glib in a semi-timely manner.)

It should. Pieces are there for GMainContext but it's all not quite the same... 

Created attachment 102342 [details] [review]
Updated patch to add GstPoll

This is an updated version of the patch to add GstPoll support. Most notable changes:

* Make configure handle HAVE_PSELECT, HAVE_POLL and HAVE_PPOLL.
* Link in the documentation for GstPoll.
* Make the code for pselect() work again (it needs to be implemented together with select() rather than ppoll() since it uses the same fdsets as select().)
* Do not add a file descriptor which is already in the set.
* Some clean-up
* gst-indented

Created attachment 102343 [details] [review]
Convert gstfdsrc/gstfdsink to use GstPoll

This patch converts gstfdsrc and gstfdsink to use GstPoll. The changes in gstfdsink.c are only compile tested since there are no unit tests for gstfdsink...

Created attachment 102415 [details] [review]
Updated patch to add GstPoll

Another update of the patch for GstPoll. This time I added gst_poll_set_controlable(), a functionality which was required to make it possible to convert gstrtspconnection.c to use GstPoll.

Created attachment 102416 [details] [review]
Convert gst-plugins-base/gst/tcp to use GstPoll

This patch converts the code in gst-plugins-base/gst/tcp to use GstPoll. Most of the changes are only compile tested since there are too few unit tests...

Created attachment 102450 [details] [review]
Updated patch to add GstPoll

Updated the patch for GstPoll again; this time with controllable spelled correctly...

Created attachment 102585 [details] [review]
Convert gst-plugins-base/gst-libs/gst/rtsp to use GstPoll

This patch converts gst-plugins-base/gst-libs/gst/rtsp to use GstPoll. It is only partially tested since we use a modified version of gstrtspconnection.c in an RTSP server rather than as a RTSP client.

Created attachment 102593 [details] [review]
Convert gst-plugins-good/gst/udp to use GstPoll

This patch converts gst-plugins-good/gst/udp to use GstPoll. 

There was one weird thing in the old code. I can not really understand the ratinale behind the following code:

#ifdef G_OS_WIN32
    if (((max_sock + 1) != READ_SOCKET (udpsrc)) ||
        ((max_sock + 1) != WRITE_SOCKET (udpsrc))) {
      ret = select (max_sock + 1, &read_fds, NULL, NULL, timeout);
    } else {
      ret = 1;
    }
#else

As far as I can tell that means the ret = 1; else clause should be taken if max_sock + 1 == READ_SOCKET (udpsrc) && max_sock + 1 == WRITE_SOCKET (udpsrc), something which can never happen. So I removed that special treatment for W32...

Created attachment 102811 [details] [review]
Convert gstreamerlibs/gst/net to use GstPoll

This patch converts gstreamerlibs/gst/net to use GstPoll.

The changes in gstnetclientclock.h and gstnettimeprovider.h are a little bit messy due to the fact that GstNetClientClock and GstNetTimeProvider have fixed sizes in the API.

Also note that _gst_reserved in GstNetTimeProvider is an empty array on 64-bit machines now (we have run out of padding). I think this might cause problems with some compilers, but I have no obvious generic method of telling if this will happen that can be used by the preprocessor to avoid including the _gst_reserved array...

That's it. There should now be patches for all uses of select() in the GStreamer core packets, except one use in gst-plugins-good/sys/oss/gstossdmabuffer.c and one in gst-plugins-bad/sys/dvb/camtransport.c. The latter two I will not do since I do not have the means to even compile test them...

Created attachment 102985 [details] [review]
Updated patch to add GstPoll

Removed the user data pointer from GstPollFD since it did not fill any real purpose and only made the struct bigger.

Created attachment 102986 [details] [review]
Convert gstreamerlibs/gst/net to use GstPoll

The removal of the user data pointer from GstPollFD made it possible to reduce the mess in gstnetclientclock.h and gstnettimeprovider.h.

Adding me to the CC list.

By the way, isn't "first patch" obsoleted by "Updated patch to add GstPoll" ?

Yes, the "first patch" is obsolete, but since it wasn't added by me, I was not allowed to mark it as obsolete when I added the updated patches.

Just to include here a possible followup for this patch.

GstPoll works on windows, with the following changes:

* include gst/windows/gstwindows.h in gstpoll.h:
        - wraps close(), socketpair() for windows builds
        - struct pollfd & POLL* defines
        - on Windows, current code will use select() to wait. On windows
select() only works with socket descriptors.
This has the side effect to make the gstpoll patch depend on #515312

*  FD_SETSIZE is defined to 64 in my mingw headers, and windows socket
descriptors are allocated with numbers above FD_SETSIZE. So the check pfd->fd <
FD_SIZE is not valid on windows. */

see bug #515312 for more details and updated patch.

        * configure.ac:
        Add checks for poll, ppoll and pselect.

        * docs/gst/gstreamer-docs.sgml:
        * docs/gst/gstreamer-sections.txt:
        Add docs for GstPoll.

        * gst/Makefile.am:
        * gst/gst.h:
        * gst/gstpoll.c: (find_index), (selectable_fds),
        (pollable_timeout), (choose_mode), (pollfd_to_fd_set),
        (fd_set_to_pollfd), (gst_poll_new), (gst_poll_free),
        (gst_poll_set_mode), (gst_poll_get_mode),
        (gst_poll_add_fd_unlocked), (gst_poll_add_fd),
        (gst_poll_remove_fd), (gst_poll_fd_ctl_write),
        (gst_poll_fd_ctl_read_unlocked), (gst_poll_fd_ctl_read),
        (gst_poll_fd_has_closed), (gst_poll_fd_has_error),
        (gst_poll_fd_can_read_unlocked), (gst_poll_fd_can_read),
        (gst_poll_fd_can_write), (gst_poll_wait),
        (gst_poll_set_controllable), (gst_poll_restart),
        (gst_poll_set_flushing):
        * gst/gstpoll.h:
        Add generic poll abstraction. We ideally don't want to have this in core
        here but in glib intead...
        This code will be used in various network elements and ultimately for
        the nanosecond precision monotonic clock (that's why it's here in core).
        It'll allow us to implement cancelable socket operations for windows too.

        * tests/check/Makefile.am:
        * tests/check/gst/gstpoll.c: (test_poll_wait), (GST_START_TEST),
        (delayed_stop), (delayed_restart), (delayed_flush),
        (delayed_control), (gst_poll_suite):
        Add GstPoll unit test.

        Patch by: Peter Kjellerstedt <pkj at axis dot com>

        * libs/gst/net/gstnetclientclock.c: (gst_net_client_clock_init),
        (gst_net_client_clock_finalize), (gst_net_client_clock_do_select),
        (gst_net_client_clock_thread), (gst_net_client_clock_start),
        (gst_net_client_clock_stop), (gst_net_client_clock_new):
        * libs/gst/net/gstnetclientclock.h:
        * libs/gst/net/gstnettimeprovider.c: (gst_net_time_provider_init),
        (gst_net_time_provider_finalize), (gst_net_time_provider_thread),
        (gst_net_time_provider_start), (gst_net_time_provider_stop),
        (gst_net_time_provider_new):
        * libs/gst/net/gstnettimeprovider.h:
        Massive code removal and cleanups because of GstPoll.
        Fixes #505417.

        Patch by: Peter Kjellerstedt  <pkj at axis com>

        * gst-libs/gst/rtsp/gstrtspconnection.c:
        (gst_rtsp_connection_create), (gst_rtsp_connection_connect),
        (gst_rtsp_connection_write), (gst_rtsp_connection_read_internal),
        (gst_rtsp_connection_receive), (gst_rtsp_connection_close),
        (gst_rtsp_connection_free), (gst_rtsp_connection_poll),
        (gst_rtsp_connection_flush):
        * gst-libs/gst/rtsp/gstrtspconnection.h:
        Use GstPoll for the rtsp connection. See #505417.

        Patch by: Peter Kjellerstedt <pkj at axis dot com>

        * plugins/elements/gstfdsink.c: (gst_fd_sink_render),
        (gst_fd_sink_start), (gst_fd_sink_stop), (gst_fd_sink_unlock),
        (gst_fd_sink_unlock_stop), (gst_fd_sink_update_fd):
        * plugins/elements/gstfdsink.h:
        * plugins/elements/gstfdsrc.c: (gst_fd_src_update_fd),
        (gst_fd_src_start), (gst_fd_src_stop), (gst_fd_src_unlock),
        (gst_fd_src_unlock_stop), (gst_fd_src_create),
        (gst_fd_src_uri_set_uri):
        * plugins/elements/gstfdsrc.h:
        Port to GstPoll. See #505417.

        Patch by: Peter Kjellerstedt  <pkj at axis com>

        * gst/tcp/Makefile.am:
        * gst/tcp/fdsetstress.c:
        * gst/tcp/gstfdset.c:
        * gst/tcp/gstfdset.h:
        Removed fdset and stress test, they are now known as GstPoll in
        core.

        * gst/tcp/gstmultifdsink.c: (gst_multi_fd_sink_class_init),
        (gst_multi_fd_sink_add_full), (gst_multi_fd_sink_remove),
        (gst_multi_fd_sink_clear), (gst_multi_fd_sink_remove_client_link),
        (gst_multi_fd_sink_handle_client_write),
        (gst_multi_fd_sink_queue_buffer),
        (gst_multi_fd_sink_handle_clients), (gst_multi_fd_sink_start),
        (gst_multi_fd_sink_stop):
        * gst/tcp/gstmultifdsink.h:
        * gst/tcp/gsttcp.c: (gst_tcp_socket_read), (gst_tcp_socket_close),
        (gst_tcp_read_buffer), (gst_tcp_gdp_read_buffer),
        (gst_tcp_gdp_read_caps):
        * gst/tcp/gsttcp.h:
        * gst/tcp/gsttcpclientsink.c: (gst_tcp_client_sink_init),
        (gst_tcp_client_sink_setcaps), (gst_tcp_client_sink_render),
        (gst_tcp_client_sink_start), (gst_tcp_client_sink_stop):
        * gst/tcp/gsttcpclientsink.h:
        * gst/tcp/gsttcpclientsrc.c: (gst_tcp_client_src_init),
        (gst_tcp_client_src_create), (gst_tcp_client_src_start),
        (gst_tcp_client_src_stop), (gst_tcp_client_src_unlock):
        * gst/tcp/gsttcpclientsrc.h:
        * gst/tcp/gsttcpserversink.c: (gst_tcp_server_sink_handle_wait),
        (gst_tcp_server_sink_init_send), (gst_tcp_server_sink_close):
        * gst/tcp/gsttcpserversink.h:
        * gst/tcp/gsttcpserversrc.c: (gst_tcp_server_src_init),
        (gst_tcp_server_src_create), (gst_tcp_server_src_start),
        (gst_tcp_server_src_stop), (gst_tcp_server_src_unlock):
        * gst/tcp/gsttcpserversrc.h:
        Port to GstPoll. See #505417.

        Patch by: Peter Kjellerstedt <pkj at axis com>

        * gst/udp/gstudpsrc.c: (gst_udpsrc_init), (gst_udpsrc_create),
        (gst_udpsrc_get_property), (gst_udpsrc_start), (gst_udpsrc_unlock),
        (gst_udpsrc_unlock_stop), (gst_udpsrc_stop):
        * gst/udp/gstudpsrc.h:
        Port to GstPoll. See #505417.