After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 698657 - shm: Port shared memory plugin to Windows
shm: Port shared memory plugin to Windows
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
unspecified
Other Windows
: Normal enhancement
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-04-23 13:35 UTC by Joshua M. Doe
Modified: 2018-11-03 13:15 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
required cherry-picked commit from master (753 bytes, patch)
2013-05-02 12:34 UTC, Joshua M. Doe
rejected Details | Review
port of 0.10 shm plugin to Windows (18.78 KB, patch)
2013-05-02 12:35 UTC, Joshua M. Doe
rejected Details | Review
port of 1.0 shm plugin to Windows (21.27 KB, patch)
2013-05-29 20:22 UTC, Joshua M. Doe
none Details | Review

Description Joshua M. Doe 2013-04-23 13:35:39 UTC
I've ported shmsink and shmsrc to Windows. I've used the simplest method possible, just #ifdef'ing code to swap out Unix sockets for TCP sockets, and to use Windows shared memory functions CreateFileMapping/MapViewOfFile instead of shm_open/mmap. Permissions are much trickier on Windows, so I've opted to #ifdef that property out. I'll attach a patch as soon as I get release approval, but before then I'd like to know if my method is acceptable.
Comment 1 Nicolas Dufresne (ndufresne) 2013-04-24 13:25:42 UTC
If the result does not respect shmsrc/sink API (the properties) it would be in my opinion better to create winshmsrc/sink elements. Also, ifdef are generally very hard to maintain, so I would not favor that way, but I'd check with others to see what they think. In the end, ask yourself if the remainting shared code is worth not duplicating.
Comment 2 Tim-Philipp Müller 2013-04-24 13:32:37 UTC
Please show us the patch so we can see how intrusive it is. Much easier to judge with the code at hand.
Comment 3 Joshua M. Doe 2013-04-24 15:14:34 UTC
(In reply to comment #1)
> If the result does not respect shmsrc/sink API (the properties) it would be in
> my opinion better to create winshmsrc/sink elements. Also, ifdef are generally
> very hard to maintain, so I would not favor that way, but I'd check with others
> to see what they think. In the end, ask yourself if the remainting shared code
> is worth not duplicating.

Point taken about the differing API. The ifdefs aren't too bad, mostly it's a matter of different sp_writer_create, sp_open_shm, and sp_client_open functions, but it's certainly not ideal.

(In reply to comment #2)
> Please show us the patch so we can see how intrusive it is. Much easier to
> judge with the code at hand.

Due to organizational processes, it'll be one or two weeks before I can post code. I just wanted to get any feedback I could in the mean time so I could make changes if needed.
Comment 4 Joshua M. Doe 2013-05-02 12:34:43 UTC
Created attachment 243039 [details] [review]
required cherry-picked commit from master
Comment 5 Joshua M. Doe 2013-05-02 12:35:22 UTC
Created attachment 243041 [details] [review]
port of 0.10 shm plugin to Windows
Comment 6 Joshua M. Doe 2013-05-02 12:46:03 UTC
I've attached the patch, which is for 0.10, but I will port to 1.0 once the following issues are resolved:
1) Are the ifdefs acceptable, or should I create a separate Windows plugin?
2) Should I allow TCP ports to be used both on Windows and not? Default both socket-path and socket-port to NULL/0, and use whichever is set?
3) I should probably just leave the default port to be 0 and require it to be set.
4) Just realized I shouldn't be using G_OS_WIN32 in shmpipe.[hc], would _WIN32 cover MinGW/MSVC?
5) Note that I've built this in MSVS, so I'll need to modify the make files to add in the use of the Winsock library.
Comment 7 Fan, Chun-wei 2013-05-28 04:35:11 UTC
Hi Joshua,

I am not the right person to answer the other questions, but hope this will help.

> 4) Just realized I shouldn't be using G_OS_WIN32 in shmpipe.[hc], would _WIN32
> cover MinGW/MSVC?
Yes, _WIN32 will cover MinGW and MSVC.

With blessings.
Comment 8 Joshua M. Doe 2013-05-29 20:22:41 UTC
Created attachment 245588 [details] [review]
port of 1.0 shm plugin to Windows

Here's a version of the patch against 1.0 (master). I've set the default port to 0 to force users to set it, and now use _WIN32 in shmpipe.[ch], and have fixed any incompatibilities with MinGW, so it build fine with make/cerbero.

I have had one problem, where sometimes (most times actually) the stream will stop after all blocks of the shared area are allocated. If I remove the allocator by uncommenting the propose_allocation assignment, everything works fine. I tried to debug this, but couldn't find anything obvious. Perhaps Olivier or someone else familiar with the allocator code could take a look.
Comment 9 Olivier Crête 2013-05-29 20:57:00 UTC
This probably means that you need to make the memory area larger.
Comment 10 Joshua M. Doe 2013-06-03 20:34:23 UTC
I had the shm-size set to many times the buffer size, which I think would have been plenty. Set to 26 times the buffer size it stalls half the time, set to 40 times it stalls maybe 10% of the time, 13 times it stalls every time. If I disable the custom allocator or an upstream element allocates the buffer then everything works fine.
Comment 11 Olivier Crête 2013-06-03 20:41:38 UTC
If you disable the custom allocator, it does a memcpy for every buffer, so I'm not sure that's what you want. That's your pipeline needs over 40 buffers kind of sounds like you're doing something suspect, maybe you have a very long queue ?
Comment 12 Joshua M. Doe 2013-06-04 13:01:46 UTC
I have no queue, here are the pipelines I'm using:

videotestsrc ! video/x-raw,format=RGB,width=320,height=240,framerate=30/1 ! shmsink shm-size=10000000 socket-port=12345

shmsrc is-live=true socket-port=12345 ! video/x-raw,format=RGB,width=320,height=240,framerate=30/1 ! videoconvert ! autovideosink

Even with a very large shm-size, the lag doesn't seem to increase, so while troubling, it's not a show stopper for me.
Comment 13 Sebastian Dröge (slomo) 2014-01-02 13:53:51 UTC
Olivier, Joshua? What's the status of this, is it ready to be merged?
Comment 14 Olivier Crête 2014-01-02 15:04:43 UTC
I'm a bit concerned about adding so much #ifdefed code that won't get much maintenance.
Comment 15 Joshua M. Doe 2014-01-02 16:17:45 UTC
I didn't like the #ifdefs much myself, but since a large majority of the code is shared, it seemed the best approach at the time. Would you suggest creating a separate element or something else?
Comment 16 Sebastian Dröge (slomo) 2014-01-03 09:28:13 UTC
Can the #ifdefs be refactored a bit maybe? I think having this in the shm plugin makes more sense than copying all the other logic and making a completely new plugin. Then we have even more code that needs to be maintained...
Comment 17 Peter Maersk-Moller 2015-01-30 11:49:01 UTC
Any chance that this code can move into Git master nay day soon?
Comment 18 Peter Maersk-Moller 2015-01-30 11:49:40 UTC
nay=any
Comment 19 Olivier Crête 2017-06-06 18:30:26 UTC
Review of attachment 245588 [details] [review]:

::: sys/shm/gstshmsrc.c
@@ +162,3 @@
 gst_shm_src_init (GstShmSrc * self)
 {
+  self->socket_port = DEFAULT_PORT;

#ifdef G_OS_WIN32

::: sys/shm/gstshmsrc.h
@@ +50,2 @@
   gchar *socket_path;
+  guint socket_port;

This one should be ifdef'ed out

::: sys/shm/shmpipe.h
@@ +89,3 @@
 ShmPipe *sp_writer_create (const char *path, size_t size, mode_t perms);
+#else
+ShmPipe *sp_writer_create (unsigned int port, size_t size);

If the API is different, may as well call the function sp_writer_create_win32().

@@ +125,3 @@
 ShmPipe *sp_client_open (const char *path);
+#else
+ShmPipe *sp_client_open (unsigned int port);

If the API is different, may as well call the function sp_client_open_win32().
Comment 20 GStreamer system administrator 2018-11-03 13:15:29 UTC
-- 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/gst-plugins-bad/issues/93.