Created attachment 144941 [details] [review]
Fixes recent changes incompatible with Windows

Comment on attachment 144941 [details] [review]
Fixes recent changes incompatible with Windows

Seriously? This is awful, it's full of object files and libtool gunk, and I have no idea what the real changes needed are.

Pulling in new libraries is not ok either.

+1 on mike's comment.

Split all of it up (the one that fixes using _priv_gst_dll_handle, and another one for fsync fixing), do not pull in external libraries, do not touch libtool.

Created attachment 144999 [details] [review]
Uses _commit() instead of fsync(), _cwait() to wait for pluginloader child

Note that while _cwait() is supposed to be a correct function to be used, pluginloader still fails (because it tries to use GstPoll with lowlevel IO file descriptors, returned by _pipe(), which does not work on Windows).

Also, it is not straightforward for me to separate _priv_gst_dll_handle fix from fsync fix, because both are required to compile GStreamer (and sending a patch that leaves the trunk in incompilable state is bad!).

Comment on attachment 144999 [details] [review]
Uses _commit() instead of fsync(), _cwait() to wait for pluginloader child

So this patch just makes it compile, not actually work? That's not too helpful either.

The problems here look much more pervasive - there are a whole lot of serious win32 issues throughout this code (cross-CRT issues, etc.).

It really needs the process/pipe management stuff factored out, and then a completely separate win32 implementation written using the native win32 APIs - doing it with the glib APIs is unlikely to work well.

I can look at doing that some time soonish, probably.

1) It works. When external loader fails, it falls back to the old plugin loader and still works.

2) Problem is that PluginLoader uses g_spawn*() to create the subprocess and pipes to communicate with it, and wraps these pipes into GstPoll. The pipes returned by g_spawn_async_with_pipes() are lowlevel CRT pipes, (_pipe() and friends) and do not support asynchronous I/O. With WinAPI pipes (CreateNamedPipe() and friends) it should be possible to do asynchronous I/O and thus it should be possible to wrap them into GstPoll (after GstPoll is extended to work with these pipes), but that also means either implementing a version of g_spawn*() and friends that work with such pipes (which is what glib developers should have done), or not using g_spawn*() at all.

As a sidenote, the same goes for files. With CRT file I/O you can't do asynchronous I/O (AFAIK, i may be wrong), but with WinAPI file I/O you can. In the end the usage of WinAPI I/O (as it was done with sockets in GstPoll) can lead to uniform higher-level I/O API that supports async I/O, events and various stuff. Of course it also means that there will be either separate object files with implementation for *nix and for Win32, or a lot of #ifdefs (kinda like GstPoll at the moment).

Created attachment 145596 [details] [review]
Uses _commit() instead of fsync() on win32

Created attachment 145597 [details] [review]
Fixes win32 private dll handle access issue

Created attachment 145598 [details] [review]
Removes a FIXME about using DuplicateHandle on win32

Created attachment 145599 [details] [review]
Uses _cwait() to wait for process exit on win32

OK, i've managed to break the patch into a few separate patches (also updated it to be against current HEAD).

The cwait one looks like it probably isn't ok.

The others look ugly but acceptable. 

I'm not sure why the code does fsync at all - maybe that can just be removed?

_cwait() should work on process identifiers returned by _spawn*(), see
http://msdn.microsoft.com/en-us/library/zb9ehy71%28VS.80%29.aspx

As for the necessity of fsync() - ask thaytan.

I read the documentation. We don't call it on something returned by _spawn(), we call it on something returned from glib. This is presumably unsafe, due to cross-CRT issues.

Created attachment 146622 [details] [review]
Uses commit() instead of fsync() on win32

Created attachment 146623 [details] [review]
Fix win32 module call path

Created attachment 146624 [details] [review]
Use dup() and dup2() on win32

Created attachment 146625 [details] [review]
Use _cwait() to wait for child process termination on win32

Created attachment 146626 [details] [review]
Support for win32 named pipes

Created attachment 146627 [details] [review]
native win32 pipe I/O and process spawning

Created attachment 146628 [details] [review]
Whitespace fix

Phew. Sorry for the tail of old patches, but i did not master git well enough to cut it off (or maybe i've been too lazy to do that).

Anyway, here's the proposal: use native process spawning (CreateProcess()) and pipe I/O (CreateNamedPipe(), CreateFile(), ReadFile(), WriteFile() and friends).

The advantage is that it has nothing to do with CRT and is not affected by CRT incompatibility. And it is not affected by the fact that msvcrt is crippled and cannot select() on non-socket descriptors.
Basically this patch does roughly the same that [i suspect (since i don't know for sure)] cygwin does - implements more-or-less POSIX-compatible synchronous file I/O using WinAPI. Not completely, mind you. It doesn't implement select() or poll(), because gstpoll is full of win32 #ifdefs already and is perfectly capable of selecting native handles in native way.

It's kinda ugly, because i left process spawning functionality (code ripped from glib) in gstpluginloader.c, while it should have got a separate file, and had to leave gst_poll_fd_read and gst_poll_fd_write in gstpoll.c, while cross-platform I/O should not have anything in common with gstpoll (since i didn't implement poll/select, gstpoll has to dig deep into the I/O to poll the handles, that is why the code is still tied to gstpoll and why I/O is implemented in gstpoll context).

Also, a few pieces of commented code here and there. And a few junk code that does nothing. And some parts are broken/unimplemented.
I've tried to keep it backward-compatible for winsock, that is - it should work with sockets on Windows the same way it worked before. In theory. Didn't test that.

But bottom line is, this code works - at least i was able to run gst-inspect-0.10 without crashing and got 186 plugins (1 blacklist entry not shown), 919 features.

Separated dll handle issue into https://bugzilla.gnome.org/show_bug.cgi?id=601668 , since it is not really linked to pluginloader (other than the fact that it is a blocker too).

Below are three pluginloader patches:

First two are native win32 pipe I/O support for gstpoll and the modification for gstpluginloader to use improved gstpoll and native win32 process spawning. The comments above are still applicable.

Third patch is an alternative to the first pair. It makes gstregistry skip gstpluginloader altogether, and also adds a few bogus #defines to gstpluginloader to make it compilable on Windows. Pluginloader remains broken, of course, but since its code is never really executed on Windows due to gstregistry changes (unless i've missed some calls to pluginloader elsewhere), it doesn't really matters whether it works or not.

Created attachment 147559 [details] [review]
[gstpoll] Native Win32 named pipe I/O

Created attachment 147561 [details] [review]
[gstpluginloader] Native Win32 process spawning and I/O

Created attachment 147563 [details] [review]
[gstpluginloader] Disable external pluginloader on Windows

This patch and patches #147559 and #147561 are mutually exclusive.

Created attachment 148632 [details] [review]
Limited POSIX-compatible I/O and process spawning API, implemented on top of WinAPI

It's big, it's bad...it works.

I've made it like this:
1) Look up POSIX specs
2) Look up WinAPI specs
3) Implement
4) Test on pluginloader (which was the focus of this patch)
5) Fix until pluginloader works
5) Test on unit tests
6) Fix until unit tests affected by it do work [again (because some of them weren't working in the first place)]

Which means that in its present condition this code is correct (mostly) in relation to GStreamer code and its unit tests, but not POSIX as it is. That is why i left it in form of #ifdefs instead of replacing POSIX/glibc syscalls with something like gst_io_read() or whatever. The intention is for programmers to be painfully aware of the fact that there are two different I/O interfaces with roughly the same logic, but with possible differences in behaviour in some cases (in which case it must be fixed).

Some things are not implementable (at least i didn't find a way to do it). For example, it is not possible to check a pipe for closiness without actually reading or writing data (writing or reading 0 bytes doesn't help). Because of that gst_win32_prepare_for_poll does a clever thing - it looks at polled events: if there are no events other than POLLHUP (FD_CLOSE), it reads or writes (depends on handle access) 1 byte. Normally that would screw up the communication process, but since application obviously expects pipe to be closed, it doesn't matter if we write garbage or waste data by reading it. But application should be aware of it to avoid unpleasant surprises.

Another thing - i didn't do a descriptor table. Which means that there are no '0', '1' and '2' descriptors matching stdin/out/err. This is obvious in some of the unit tests where two functions rely on the fact that fd = 1 is always the same fd. To make it work, i had to create a wrapper (because this is how it works) WinFd structure in one function and pass it to the other function via global variable to make sure that the other function uses the same pointer.

Hacky things:

return values (especially the painful 64-bit file seeking). Some functions may return signed values instead of unsigned. Be careful.

fd as gpointer. It might be dangerous because while gpointer is big enough to hold any fd, it is unsigned, while fd is signed. If you see '== (gpointer) -1' - it really was '< 0'.

Some unit tests will continue to fail. I guess they were failing before too. Unless i know exactly what exactly is being tested (tests are known to produce a lot of warnings/errors because they sometimes do bad things on purpose, so just fixing errors is not correct - i have to know which errors are intended and which aren't) i can't rectify that.

Hi tried the patch-148632 and this is what I got:

Program received signal SIGSEGV, Segmentation fault.
0x6b72f919 in gst_win32_fsync (wfd=0xffffffff) at gstwin32io.c:278
278       FlushFileBuffers (wfd->fd);
(gdb) bt

+ Trace 219501

Steps:
clean env and old gstreamer installation
make
make install
gst-inspect-0.10

Can't reproduce.

Apparently, loader->fd_w.fd is left at "(gpointer) -1" (the value it is initialized with).

The only place where fd_w.fd is altered is the call to gst_win32_spawn_async_with_named_pipes(). If fd_w.fd is left at 0xffffffff after that call, it means that do_spawn_with_named_pipes() fails to create a pipe. But it doesn't, because if it did, it would have returned FALSE.

Rather, i think it means that plugin_loader_free() is called for a pluginloader that was initialized (which is done at creation time), but that never had a chance to call gst_plugin_loader_try_helper(), so fd_w.fd remained 0xffffffff.

Which is very confusing, because gst_update_registry() never calls clear_scan_context() and thus plugin_loader_free (which it does, according to your backtrace). And even if it did, clear_scan_context() shouldn't try to destroy pluginloader, unless context->helper is non-NULL, which should only happen when gst_plugin_loader_try_helper() was called.

The fact that it comes to plugin_loader_free() being called for a pluginloader without a helper process is clearly a bug. However as far as i can see, calling fsync() with invalid FD is not an error (sets errno to BADF, but that's all it does), so here's a fixed version of gst_win32_fsync():

gint
gst_win32_fsync (WinFd *wfd)
{
  if (G_UNLIKELY (!wfd || wfd->fd == (gpointer) -1))
  {
    errno = EBADF;
    return -1;
  }

  FlushFileBuffers (wfd->fd);
  return 0;
}

P.S. Don't forget to fix the prototype in gstwin32io.h

P.P.S. It might be a good idea to further improve this function by checking for FlushFileBuffers return value: if it's zero, GetLastError() should be called and errno set appropriately.

hi, it didn't resolve the crash:

Program received signal SIGSEGV, Segmentation fault.
0x6b72f91d in gst_win32_fsync (wfd=0xffffffff) at gstwin32io.c:278
278       if (G_UNLIKELY (!wfd || wfd->fd == (gpointer) -1))
(gdb) bt

+ Trace 219515

but if (G_UNLIKELY (!wfd || wfd == (gpointer) -1 || wfd->fd == (gpointer) -1))
yes

in gst_win32_close you are checking wfd == (gpointer) -1, why not in gst_win32_fsync so.

Well now gst-inspect-0.10 does not crash.

$ gst-inspect-0.10
coreindexers:  memindex: A index that stores entries in memory
coreelements:  multiqueue: MultiQueue
coreelements:  typefind: TypeFind
coreelements:  tee: Tee pipe fitting
coreelements:  filesink: File Sink
coreelements:  queue2: Queue 2
coreelements:  queue: Queue
coreelements:  identity: Identity
coreelements:  filesrc: File Source
coreelements:  fdsink: Filedescriptor Sink
coreelements:  fdsrc: Filedescriptor Source
coreelements:  fakesink: Fake Sink
coreelements:  fakesrc: Fake Source
coreelements:  capsfilter: CapsFilter
staticelements:  bin: Generic bin
staticelements:  pipeline: Pipeline object

Total count: 3 plugins, 16 features

By the way, I am using msys+mingw in a winxp virtualbox client, and host is a linux. gstreamer checkout is on the host and the dir is shared to the client.
Maybe it can explain this particular case that caused this crash.

Sorry, my bad (about checking wfd->fd instead of just wfd).

As for what caused the crash - i am clueless, really. You could try putting some log-points into gstregistry and pluginloader to see when a pluginloader object is created and when it is destroyed and what is being done with it in between. If there is a reason why a pluginloader is constructed and then freed without being used, you'll see it. Otherwise there's a bug in my own gstwin32io.c 

Sadly, i only have Windows host (and creating a Linux VBox that runs a Windows VBox is kinda...weird), so for me it is still unreproducible.

By the way, i hope no one is offended by the fact that this code logs at warning level? :)

Created attachment 151059 [details] [review]
Fix win32 compilation

If we just need something that compiles, to fallback to old behaviour this should work for the moment.

Created attachment 151068 [details] [review]
Fix win32 compilation

I haven't seen LRN first patch :/
Mine is similar but including <io.h> instead of redifining _close, etc...
I have also disabled the plugin loader as he does.

The long-term solution is to wait until certain version of GLib and use GPoll instead of GstPoll.
GstPoll+my_big_patch and GPoll are different in following ways:
1) My poll uses asynchronous pipes (when coupled with my implementation of spawn()), which work without threads (synchronous pipes + threads are also supported). GPoll uses normal synchronous CRT pipes with threads for anynchronousness
2) My implementation of spawn() uses Windows process handles instead of process IDs, thus eliminating the problem with CRT incompatibility (process ID returned by g_spawn*() are supposedly only waitable/closeable with waitpid/close of the same version of MSVCRT). It might be fixable on GLib side if they provide g_close() and g_waitpit() (do they?)
3) My patch doesn't work with GSource at all, because GSource polls sources internally and there is no way to substitute that poll with my own.
4) My patch is my patch, GPoll is supposedly maintained by GLib developers
5) Migration from GstPoll to GPoll is easier than migration from GstPoll to GstPoll+my_big_patch (especially considering the lack of GSource integration)

LRN: Andoni's patch seems marginally cleaner to me - does it make things work for you as well?

Comment on attachment 151068 [details] [review]
Fix win32 compilation

Committed (after making sure to include gst_private.h which includes glib.h before so that G_OS_WIN32 is defined properly - how did it work right before without that, I wonder - did you test the patch?)

http://lrn.no-ip.info:58010/builders/w32-core-bin/builds/220/steps/make/logs/stdio

Is there still something necessary to do here?

Plugin loader still doesn't work on W32 (presumably), and is still disabled. The patch that was committed simply disables it unconditionally on W32.

Honestly, i haven't thought about this problem for months. Also didn't check what glib was doing with g_spawn() and GPoll...

I think the solution here would be to "just" exec() the plugin scanner and use a different IPC mechanism, e.g. a TCP socket, to talk between main process and plugin scanner.

Comment on attachment 148632 [details] [review]
Limited POSIX-compatible I/O and process spawning API, implemented on top of WinAPI

At least needs to be rebased to git master, but also should be split a bit. Such a extremely large patch is simply not reviewable in reasonable time.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gstreamer/issues/11.