Created attachment 272942 [details] [review]
udpsrc: add force-buffer-size property

add patch file.

Comment on attachment 272942 [details] [review]
udpsrc: add force-buffer-size property

modify the patch file because of the author.

Created attachment 272944 [details] [review]
Add 'force-buffer-size' for udpsrc element

I think it would cleaner to just have the "force-buffer-size" as a boolean, and re-use "buffer-size" in both cases.

Created attachment 289819 [details] [review]
Modify 1st patchset for applying Julien Isorce's comment

I think user can select one between the 'buffer-size' and 'force-buffer-size' property in https://bug727067.bugzilla-attachments.gnome.org/attachment.cgi?id=272944 patchset.

In your opinion, user should use two properties together.
Actually I'm not sure which one is more better solution.

I've just add the patch set for following your opinion.
Please choose one patch set and apply it for adding the 'force-buffer-size' property.

Review of attachment 289819 [details] [review]:

::: gst/udp/gstudpsrc.c
@@ +1007,1 @@
   }

You should also use udpsrc->force_buffer_size in the first block "#if GLIB_CHECK_VERSION (2, 35, 7)", or any reason you did not ?

@@ +1007,2 @@
   }
 #elif defined (SO_RCVBUF)

Is SO_RCVBUFFORCE always available if SO_RCVBUF is ?

This should probably indeed be in an #ifdef SO_RCVBUFFORCE block.

Also what's the argument against *always* forcing the buffer size? That's what the user expected probably.

Yes, was going to ask the same thing, why have a property for this, instead of just trying to force it if the normal way didn't succeed? After all, if a buffer size got set, it's already a "I really want this".

According to http://linux.die.net/man/7/socket it seems it requires some privileges to use SO_RCVBUFFORCE: "... a privileged (CAP_NET_ADMIN) process can perform ..."

Right, but you can try setting it, and if the resulting size is too small you can try forcing it, and if that didn't work you post a warning message on the bus and move on. If we don't have the special caps, we can error out or try with whatever size we got.

Anybody going to update the patch accordingly? I think it would be good to do that :)

I.e., no new property. First try the current way, if that fails try the FORCE one, if that fails post a warning message.

Created attachment 309951 [details] [review]
Modify patchset for applying Sebastian Dröge (slomo)'s comment

I'm following Sebastian Dröge (slomo)'s advice and attach new patchset.
1. Delete new property. 
2. First try the current way, if that fails try the FORCE one, if that fails post a warning message.

Review of attachment 309951 [details] [review]:

::: gst/udp/gstudpsrc.c
@@ +1019,3 @@
+              src->buffer_size, &opt_err)) {
+        GST_ELEMENT_WARNING (src, RESOURCE, SETTINGS, (NULL),
+            ("Could not create forcibly a buffer of requested %d bytes: %s",

"Could not force receive buffer size to %d bytes: %s" seems more descriptive

@@ +1022,3 @@
+                src->buffer_size, opt_err->message));
+        g_error_free (opt_err);
+        opt_err = NULL;

This is equivalent to g_clear_error(&opt_err). Just use that :)

@@ +1070,3 @@
+     * Try to set the buffer_size by using SO_RCVBUFFORCE */
+    if (rcvsize < src->buffer_size) {
+      rcvsize = src->buffer_size;

Instead of having this code here copied twice, please move it into a function :)

Review of attachment 309951 [details] [review]:

Could you give me some advice?

::: gst/udp/gstudpsrc.c
@@ +1019,3 @@
+              src->buffer_size, &opt_err)) {
+        GST_ELEMENT_WARNING (src, RESOURCE, SETTINGS, (NULL),
+      GST_INFO_OBJECT (src, "forcibly setting udp buffer of %d bytes",

I will do that.

@@ +1022,3 @@
+                src->buffer_size, opt_err->message));
+        g_error_free (opt_err);
+          src->buffer_size);

I will change the 1023~1024 line to the g_clear_error function.

@@ +1070,3 @@
+     * Try to set the buffer_size by using SO_RCVBUFFORCE */
+    if (rcvsize < src->buffer_size) {
+      rcvsize = src->buffer_size;

Could you explain it more?

Do you mean that a function is needed for the line 1072 and line 1042?
i.e) Is the "rcvsize = src->buffer_size;" line moved into a function?

Or Does I need to make a function for Line 1012~1033 and Line 1068~1096?
Although I make the function for this, the #if and #elif (line 981 and line 1035) is needed.
So I didn't split the function.

I saw that you have almost the same code in 1012-1033 and 1068-1096. Try to make those one function that is called from both places :)

Created attachment 310023 [details]
Modify patchset for applying Sebastian Dröge (slomo)'s comment

Created attachment 310025 [details] [review]
Modify patchset for applying Sebastian Dröge (slomo)'s comment

I tried rebasing this patch on top of master but ended up re-implementing it. We don't care about supporting older glib now.

Created attachment 373377 [details] [review]
udpsrc: factor out gst_udpsrc_get_rcvbuf()

No semantic change.

Created attachment 373378 [details] [review]
updsrc: set udp buffer size forcibly

The udp buffer size is limited to a maximum of around 100K.
Some apps need to set the force bufsize for their own operation.
Use the SO_RCVBUFFORCE option in order to override the rmem_max limit
of linux kernel.

Original patch from Kyungnam Bae <kyungnam.bae@lge.com>

Review of attachment 373378 [details] [review]:

I think it does not address the review comment. The suggestion was to set the recv buffer size the normal way, and if this didn't work, simply try the second method, or warn if that failed (you need privileged user, so it should fail for most use case). And then we don't need yet another property. We do the same for multi-cast groups, but can't remember if it's sex or sink.

Yeah that's the logic I implemented. Or did I miss something?

Forgive me for asking, but does this actually work for you?

Because for me the kernel seems to just cap the actual configured setting at the configured max size and not return FALSE from g_socket_set_option().

Yes it does. Actually it fixes a performance issue when streaming.

The first call (not force) is indeed capped but after the force one it is the to the double of the value as expected.

Tested on 4.14.0 as root.

(In reply to Tim-Philipp Müller from comment #23)
> Forgive me for asking, but does this actually work for you?
> 
> Because for me the kernel seems to just cap the actual configured setting at
> the configured max size and not return FALSE from g_socket_set_option().

I'm guessing the point here is that instead of getting EPERM, you get success with smaller size. So you neither get a warning or an error, all you get is the maximum allowed by the kernel.

Yes indeed, see https://elixir.bootlin.com/linux/v3.4/source/net/core/sock.c#L613
That's why I check the actual value after setting before try to force.

Review of attachment 373378 [details] [review]:

Outch, my review is wrong.

Review of attachment 373377 [details] [review]:

.

Review of attachment 373378 [details] [review]:

The comment should maybe state that this will only work if you have the net.admin privilege.

::: gst/udp/gstudpsrc.c
@@ +1441,1 @@
       }

So, I think we should add the if (val < src->buffer_size), and warn, just to help users a little.

Created attachment 373394 [details] [review]
updsrc: set udp buffer size forcibly

The udp buffer size is limited to a maximum of around 100K.
Some apps need to set the force bufsize for their own operation.
Use the SO_RCVBUFFORCE option in order to override the rmem_max limit
of linux kernel. Require user to have the CAP_NET_ADMIN privilege to
work.

Original patch from Kyungnam Bae <kyungnam.bae@lge.com>

Right, sorry, I missed the other check, thought you relied on the if (!set_options) failing :)

> So, I think we should add the if (val < src->buffer_size), and warn,
> just to help users a little.

Not superimportant, but here the kernel returns 200,000 when reading the value after 100,000 was set (apparently for extra overhead for the internal structs etc.), so maybe on Linux this needs to be if (val / 2 < src->buffer_size) then? Bit ugly, but might be confusing if setting values around the limit if not taken into consideration?

Created attachment 373407 [details] [review]
udpsrc: Balance Linux value of get/set_rcvbuf

On Linux, the kernel returns twice the size as it will allocate extra
space for accouting. We devides this value by two in order to ensure
that get/set value now match. This fixes the set buffer size validation
and allow having a nice warning when the size if surpassed and the
process does not have CAP_NET_ADMIN capabilities.

Attachment 373377 [details] pushed as 58a5f59 - udpsrc: factor out gst_udpsrc_get_rcvbuf()
Attachment 373394 [details] pushed as 9b4a022 - updsrc: set udp buffer size forcibly
Attachment 373407 [details] pushed as c61bcb1 - udpsrc: Balance Linux value of get/set_rcvbuf