GNOME Bugzilla – Bug 520808
GstPoll's API could need some changes for Windows
Last modified: 2008-03-18 11:20:44 UTC
Please describe the problem: gst_poll_add_fd: On Windows files and sockets don't share the same namespace, so a disambiguation on what kind of fd you're adding to the set could be useful -- if there's file handle 0x1337 and socket handle 0x1337 it could be both and it would be impossible for the implementation to know which one you meant. gst_poll_wait: Not really a request for change, given how naturally this maps to select, poll etc., but is it essential for the caller to know exactly how many fds were signaled and not just one or more? The reason I'm asking is that due to how I see this implemented on Windows it means you WaitForMultipleObjects for the fds, and you just get to know which fd's handle got signaled, and you then have to iterate over the fds and query each of them, accumulating the total. Given that the user of GstPoll would most likely end up doing gst_poll_fd_can_{read,write} to figure out which were signaled, it means the same operation is repeated. Thoughts? Steps to reproduce: Actual results: Expected results: Does this happen every time? Other information:
To clarify what I meant about gst_poll_add_fd: On Windows you need to know what kind of fd you're dealing with in order to know if you should use the winsock API or another API for monitoring the fd. However, given that an implementation cannot know which kind of fd it is without checking validity of the handle in each domain/namespace, it won't be able to do so reliable because the handles in each may overlap. You can have a socket handle that's 0x100 and a file handle that's 0x100, and both will be valid. How will the GstPoll implementation then know if it should monitor the socket 0x100 or the file 0x100?
To clarify what I meant about gst_poll_add_fd: On Windows you need to know what kind of fd you're dealing with in order to know if you should use the winsock API or another API for monitoring the fd. However, given that an implementation cannot know which kind of fd it is without checking validity of the handle in each domain/namespace, it won't be able to do so reliably because the handles in each may overlap. You can have a socket handle that's 0x100 and a file handle that's 0x100, and both will be valid. How will the GstPoll implementation then know if it should monitor the socket 0x100 or the file 0x100?
Oops, sorry about the double post!
An example: C runtime file descriptor: int open (...) System handle: HANDLE CreateFile (...) Winsock descriptor: SOCKET socket (...)
Please disregard the second point (the one about gst_poll_wait), after finishing the GstPoll Windows porting I realized it was no longer valid, as one needs to enumerate socket events in order to reset the event object and store the current state by means of WSAEnumNetworkEvents. (Because this one has side-effects it can't be called multiple times so one has to store the last state.)
I've taken a different approach to make gstpoll work on windows : * disregard file handling in gstpoll as filesrc works well * use gstpoll's select implementation to handle socket descriptors Bug #515312 may be of some use.
Is there any proposed patch for this bug? The release is next monday, so any change needs to go in as soon as possible
Hrmn, well I actually looked at bug #515312 now - I need someone that's actually on windows to evaluate it though.
Well, #515312 is not related to this bug. This is about aiding the GstPoll implementation on platforms like Windows so that it can do the right thing behind the scenes. I'll write up a patch to illustrate the basic idea.
Created attachment 107131 [details] [review] gstpoll_api_changes An illustration of the basic idea. I am however _not_ happy with this, its only purpose is to spawn a discussion around how the API should look like. The method descriptions are based on: http://library.gnome.org/devel/glib/stable/glib-IO-Channels.html
Another possible solution is adding a field to GstPollFD that states the fd kind, and pass this as an argument to gst_poll_fd_init ().
Or add a function gst_poll_fd_set_fd/socket/messages() to explicitly set the socket value and type. These functions also take the right params so that the compiler can typecheck at compile time.
Wim: That sounds much better! I'll write up a patch for that. However, there's another issue of far greater significance that I realized while trying to port rtspsrc to Windows (which I decided to port solely because it would be a better acid-test for GstPoll than udpsrc -- given that libcheck turned out to take way more time to port to Windows than what I feel like spending on it right now in order to run the GstPoll test-suite). A summary of the discussion on IRC and the proposed solution follows: <oleavr> wtay: I'm not sure how to fix GstPoll.. the Windows semantics are weird. when you tell Winsock "when one of these events occur on this socket, signal this event" through WSAEventSelect and you wait() (WaitForMultipleObjects) and it gets signaled, you then have to figure out which of the events occured by calling WSAEnumNetworkEvents.. however, that clears the internal network event records on the socket and resets the event.. so if you try to wait for the same event again immediately after that, it will time out / wait forever.. you have to issue a read()/write() etc. to actually re-enable it <oleavr> now one could then do a dummy read()/write() or similar for each such condition to re-enable it, but if the GstPoll user then does a read()/write() as well and then wait(), it will signal as a consequence of the dummy operation and not the actual operation thereafter <oleavr> in other words, I'm not sure how I can properly emulate the select()/poll()/etc. semantics on Windows (...) <wtay> oleavr, we need something that can wait on whatever it is that windows uses for sockets, multiples of them with a timeout and then see what happened <oleavr> wtay: that part is easy, the tricky part is waiting twice in a row, because a wait changes the internal socket state <MikeS> oleavr: could GstPoll keep additional state internally (the info it gets from WSAEnumNetworkEvents?), and avoid waiting on things that it knows are available right now, or something like that? <oleavr> MikeS: well, it already stores the result of each WSAEnumNetworkEvents after a successful wait, the problem is that such a call has the side-effect of clearing the internal socket event, and taking the role as GstPoll you won't know if the user did an operation that could change that state between two successive calls to gst_poll_wait <MikeS> ah, I see. <MikeS> So basically, you don't know when to reset whatever it stores from WSAEnumNetworkEvents? <MikeS> Because that doesn't go through the GstPoll API at all <oleavr> MikeS: yep, if only WSAEnumNetworkEvents didn't clear the internal network event records of the socket until the next so-called 're-enabling event' like recv/send/etc. <oleavr> a hack would be to issue appropriate re-enabling calls according to which were set, before leaving gst_poll_wait, but then the next gst_poll_wait will give you the result of the dummy operation's completion <MikeS> oleavr: so the only other way around this would be for those other calls (recv, etc) to go through GstPoll, so it could reset things, etc. That would suck too. (...) <wtay> oleavr, can't we just say that all sockets with activity needs to be handled (on windows), which is usually a good thing to do anyways <oleavr> wtay: that's a possibility.. the reason I ran into this is that gstrtsp does 'connect' as: [socket () -> make it nonblocking -> connect() -> gst_poll_wait(WRITE) -> yay, connected!], followed by 'send' being: [gst_poll_wait(WRITE) -> send ()], where the two consecutive waits screws things up :) <MikeS> wtay: or add a function to say "I have not acted upon this thing" for a given FD - so for every socket with activity, you either have to do something, or call that function
Created attachment 107414 [details] [review] gstpoll_fd_ignored_and_windows_fixes A first attempt at the suggested API change suggested by Mike. Plus: - Call WSAEventSelect() in gst_poll_wait() instead of gst_poll_fd_ctl_{read,write}() so that these can be safely called while waiting. - A few minor cleanups. - Fixed a FIXME: Make sure to clear the control socket even when an error occurs.
Created attachment 107415 [details] [review] gstrtsp_new_gst_poll_semantics Update to libgstrtsp in -base required by the API change.
Created attachment 107416 [details] [review] gstrtsp_windows_fixes Generic Windows fixes to libgstrtsp in -base that makes libgstrtsp work on Windows when coupled with gstrtsp_new_gst_poll_semantics and the GstPoll API update.
Created attachment 107417 [details] [review] gstsdp_g_print_string_null_fixes Fixed some g_print issues in libgstsdp (in -base) with %s arguments that might be NULL. Not sure if this should be fixed in the application or if GLib's replacement implementation (_g_gnulib_vasprintf) should be improved to handle this. (This replacement implementation is used on Windows because Microsoft's implementation isn't adequate.)
Created attachment 107418 [details] [review] gstrtspsrc_winsock_init_fix A small fix to the gstrtsp plugin in -good to make it work on Windows -- WSAStartup() needs to be called before any other Winsock API calls are made (and on shutdown obviously WSACleanup() once for every previously made call to WSAStartup()).
Clearly the general Windows fixes not related to GstPoll should've been submitted in separate bugs, but I'm really tired and need to get some sleep, so please bear with me. :)
Patch by: Ole André Vadla Ravnås <ole.andre.ravnas@tandberg.com> * gst/rtsp/gstrtspsrc.c: (gst_rtspsrc_init), (gst_rtspsrc_finalize): Call WSAStartup() and WSACleanup before using the Winsock API. See #520808.
Created attachment 107464 [details] [review] gstpoll_fd_ignored_and_windows_fixes Updated version that also does proper error-handling by translating Winsock's last error and updating errno.
The last patch looks ok to me.
Created attachment 107466 [details] [review] gstpoll_fd_ignored_and_windows_fixes Updated version that adds the missing entry in: docs/gst/gstreamer-sections.txt
I would propose to include patch Comment #23 and the patches in #15 and #16 so that we don't have to wait for another iteration of -core/-base to get rtspsrc working on windows.
Created attachment 107474 [details] [review] gstpoll_fd_ignored_and_windows_fixes Also toggle FD_ACCEPT as part of gst_poll_fd_ctl_read(). tcpclientsrc and tcpserversink are almost there now.
I'm ok with #15, #16 and #25 too. One style comment: the ERRNO_IS_* macros in #16 should be wrapped in ()
Created attachment 107493 [details] [review] gstrtsp_windows_fixes Updated version of #16 that wraps ERRNO_IS_* macros in ().
Created attachment 107500 [details] [review] gstrtsp_windows_fixes The third and last update to #16 that initializes the ioctlsocket(FIONBIO) argument to 1 instead of passing it an uninitialized value as the value to specify blocking vs non-blocking mode. (0 means blocking, non-zero means non-blocking.)
Created attachment 107502 [details] [review] gstrtsp_windows_fixes Fourth and last (promise!) version of the patch in #16 that actually wraps ERRNO_IS_* macros in () -- I wonder where my brain was when I initially interpreted Jan's style comment. :)
Created attachment 107514 [details] [review] nicer fix for null strings using GST_STR_NULL
Oooh, now that's elegant! Thanks, I'll remember that one :)
These look good to go in now to me - I'd like to get them in and cut a new pre-release.
Patch by: Ole André Vadla Ravnås <ole dot andre dot ravnas at tandberg dot com> * docs/gst/gstreamer-sections.txt: * gst/gstpoll.c: (gst_poll_winsock_error_to_errno), (gst_poll_update_winsock_event_mask), (gst_poll_prepare_winsock_active_sets), (gst_poll_collect_winsock_events), (gst_poll_new), (gst_poll_free), (gst_poll_add_fd_unlocked), (gst_poll_fd_ctl_write), (gst_poll_fd_ctl_read_unlocked), (gst_poll_fd_ignored), (gst_poll_fd_has_error), (gst_poll_fd_can_read_unlocked), (gst_poll_check_ctrl_commands), (gst_poll_wait): * gst/gstpoll.h: * win32/common/libgstreamer.def: Add new function gst_poll_fd_ignored() for improved Windows compatibility. Various minor fixes and cleanups. See #520808.
Patch by: Ole André Vadla Ravnås <ole.andre.ravnas@tandberg.com> * gst-libs/gst/rtsp/gstrtspconnection.c: (gst_rtsp_connection_connect), (gst_rtsp_connection_write), (read_line), (gst_rtsp_connection_read_internal): Generic Windows fixes that makes libgstrtsp work on Windows when coupled with the new GstPoll API. See #520808.
I've commited my 'all string' variant of the GST_STR_NULL patch because it's so trivial. * gst-libs/gst/sdp/gstsdpmessage.c: (gst_sdp_message_dump): Use GST_STR_NULL when trying to print strings that could be NULL because this might crash on some platforms. See #520808.