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.

Please show us the patch so we can see how intrusive it is. Much easier to judge with the code at hand.

(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.

Created attachment 243039 [details] [review]
required cherry-picked commit from master

Created attachment 243041 [details] [review]
port of 0.10 shm plugin to Windows

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.

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.

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.

This probably means that you need to make the memory area larger.

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.

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 ?

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.

Olivier, Joshua? What's the status of this, is it ready to be merged?

I'm a bit concerned about adding so much #ifdefed code that won't get much maintenance.

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?

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...

Any chance that this code can move into Git master nay day soon?

nay=any

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().

-- 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.