[jitsi-dev] refreshing patches


#1

I've run git svn rebase on my local workspace and re-generated the patch
set for all things that weren't applied by Seb in r301

This includes the items from both of my emails yesterday, and also some
formatting improvements for some of the code.

This set of patches should apply cleanly to the latest SVN

Some of these changes are necessary for Lumicall to build cleanly
against ice4j

0001-Remove-extra-spaces-from-Candidate.toString-so-it-ca.patch (895 Bytes)

0002-Ensure-log-messages-contain-the-STUN-error-codes.patch (1.23 KB)

0003-Reformatting-patch-log-message-with-error-code-detai.patch (1.28 KB)

0004-Use-notify-instead-of-notifyAll-because-only-one-oth.patch (1.61 KB)

0005-Enhance-DelegatingDatagramSocket-to-obtain-delegates.patch (6.38 KB)

0006-Reformatting-patch-Enhance-DelegatingDatagramSocket-.patch (2.41 KB)

0007-IPv6-add-system-property-for-disabling-IPv6-host-can.patch (2.2 KB)


#2

Hi Daniel,

Sorry for the delay, there is a lot of pending work. Applying your patches is still on my "to do" list. I hope to commit them in the following weeks.

Best regards,
Vincent

···

On 06/06/2012 01:18 PM, Daniel Pocock wrote:

I've run git svn rebase on my local workspace and re-generated the patch
set for all things that weren't applied by Seb in r301

This includes the items from both of my emails yesterday, and also some
formatting improvements for some of the code.

This set of patches should apply cleanly to the latest SVN

Some of these changes are necessary for Lumicall to build cleanly
against ice4j

--
Vincent Lucas, Ph.D. Jitsi developer
chenzo@jitsi.org http://jitsi.org


#3

Hi Daniel,

Thank you very much for submitting your patches. I have started to apply patches 1, 2, 3, 4 and 7: integrated into ICE4J build #126 (svn revision #312).

Comments for patches 5, 6 and 7:

- #7) The property was renamed into "org.ice4j.ipv6.DISABLED" (to respect Jitsi property name convention).

- #5 and #6) Are not yet incorporated. Indeed, I do not see the point of these patches. Maybe you can give me some additional explanation:
---- Is it designed modify only the SO_RECVBUF?
Or does your DatagramSocketFactory is there to implement another functionality?
---- Can't you achieve the same thing by setting the SO_RECVBUF directly by calling the DelegatingDatagramSocket#setReceiveBufferSize(int size)?
Or by setting a custom class which extends the DataramSocket class as a delegating socket to the DelegatingDatagramSocket?

Best regards,
Vincent

···

On 06/06/2012 01:18 PM, Daniel Pocock wrote:

I've run git svn rebase on my local workspace and re-generated the patch
set for all things that weren't applied by Seb in r301

This includes the items from both of my emails yesterday, and also some
formatting improvements for some of the code.

This set of patches should apply cleanly to the latest SVN

Some of these changes are necessary for Lumicall to build cleanly
against ice4j

--
Vincent Lucas, Ph.D. Jitsi developer
chenzo@jitsi.org http://jitsi.org


#4

Hello Daniel,

I have forgotten: your patches are acknowledged on Jitsi contributor web page.

Best regards,
Vincent

···

On 07/06/2012 05:55 PM, Vincent Lucas wrote:

Hi Daniel,

Thank you very much for submitting your patches. I have started to apply
patches 1, 2, 3, 4 and 7: integrated into ICE4J build #126 (svn revision
#312).

Comments for patches 5, 6 and 7:

- #7) The property was renamed into "org.ice4j.ipv6.DISABLED" (to
respect Jitsi property name convention).

- #5 and #6) Are not yet incorporated. Indeed, I do not see the point of
these patches. Maybe you can give me some additional explanation:
---- Is it designed modify only the SO_RECVBUF?
Or does your DatagramSocketFactory is there to implement another
functionality?
---- Can't you achieve the same thing by setting the SO_RECVBUF directly
by calling the DelegatingDatagramSocket#setReceiveBufferSize(int size)?
Or by setting a custom class which extends the DataramSocket class as a
delegating socket to the DelegatingDatagramSocket?

Best regards,
Vincent

On 06/06/2012 01:18 PM, Daniel Pocock wrote:

I've run git svn rebase on my local workspace and re-generated the patch
set for all things that weren't applied by Seb in r301

This includes the items from both of my emails yesterday, and also some
formatting improvements for some of the code.

This set of patches should apply cleanly to the latest SVN

Some of these changes are necessary for Lumicall to build cleanly
against ice4j

--
Vincent Lucas, Ph.D. Jitsi developer
chenzo@jitsi.org http://jitsi.org


#5

In r312, I notice you have this:

+ boolean isIPv6Disabled = StackProperties.getBoolean(
+ "org.ice4j.ipv6.DISABLED",
+ false);

···

On 06/07/12 17:55, Vincent Lucas wrote:

Hi Daniel,

Thank you very much for submitting your patches. I have started to
apply patches 1, 2, 3, 4 and 7: integrated into ICE4J build #126 (svn
revision #312).

Comments for patches 5, 6 and 7:

- #7) The property was renamed into "org.ice4j.ipv6.DISABLED" (to
respect Jitsi property name convention).

+

but maybe you should have:

+ boolean isIPv6Disabled = StackProperties.getBoolean(
+ StackProperties.DISABLE_IPV6,
+ false);
+

and of course you would need to rename

+ * The name of the property used to disabled IPv6 support.
+ */
+ public static final String ENABLE_IPv6 = "org.ice4j.ipv6.DISABLED";
+

- #5 and #6) Are not yet incorporated. Indeed, I do not see the point
of these patches. Maybe you can give me some additional explanation:
---- Is it designed modify only the SO_RECVBUF?
Or does your DatagramSocketFactory is there to implement another
functionality?

DatagramSocketFactory:

There is a JNI implementation of DatagramSocket in Sipdroid code. The
factory pattern allows Lumicall to use such a socket if necessary.

---- Can't you achieve the same thing by setting the SO_RECVBUF
directly by calling the
DelegatingDatagramSocket#setReceiveBufferSize(int size)?
Or by setting a custom class which extends the DataramSocket class as
a delegating socket to the DelegatingDatagramSocket?

If I let ice4j create the socket first, and then I try to set
SO_RECVBUF, it is very slow, the operation blocks for seconds

If I set the buffer size early, there is no such performance issue.

That is why I need to set SO_RECVBUF in this way.

If you accepted the factory patch, then I could implement the SO_RECVBUF
in a custom factory class myself. However, I feel it is a very common
requirement, so good to have this method in DelegatingDatagramSocket

Best regards,
Vincent

On 06/06/2012 01:18 PM, Daniel Pocock wrote:

I've run git svn rebase on my local workspace and re-generated the patch
set for all things that weren't applied by Seb in r301

This includes the items from both of my emails yesterday, and also some
formatting improvements for some of the code.

This set of patches should apply cleanly to the latest SVN

Some of these changes are necessary for Lumicall to build cleanly
against ice4j


#6

Hello Daniel,

Hi Daniel,

Thank you very much for submitting your patches. I have started to
apply patches 1, 2, 3, 4 and 7: integrated into ICE4J build #126 (svn
revision #312).

Comments for patches 5, 6 and 7:

- #7) The property was renamed into "org.ice4j.ipv6.DISABLED" (to
respect Jitsi property name convention).

In r312, I notice you have this:

+ boolean isIPv6Disabled = StackProperties.getBoolean(
+ "org.ice4j.ipv6.DISABLED",
+ false);
+

but maybe you should have:

+ boolean isIPv6Disabled = StackProperties.getBoolean(
+ StackProperties.DISABLE_IPV6,
+ false);
+

and of course you would need to rename

+ * The name of the property used to disabled IPv6 support.
+ */
+ public static final String ENABLE_IPv6 = "org.ice4j.ipv6.DISABLED";
+

Thank you for picking up this mistake (corrected by Ice4j svn revision #127).

- #5 and #6) Are not yet incorporated. Indeed, I do not see the point
of these patches. Maybe you can give me some additional explanation:
---- Is it designed modify only the SO_RECVBUF?
Or does your DatagramSocketFactory is there to implement another
functionality?

DatagramSocketFactory:

There is a JNI implementation of DatagramSocket in Sipdroid code. The
factory pattern allows Lumicall to use such a socket if necessary.

---- Can't you achieve the same thing by setting the SO_RECVBUF
directly by calling the
DelegatingDatagramSocket#setReceiveBufferSize(int size)?
Or by setting a custom class which extends the DataramSocket class as
a delegating socket to the DelegatingDatagramSocket?

If I let ice4j create the socket first, and then I try to set
SO_RECVBUF, it is very slow, the operation blocks for seconds

If I set the buffer size early, there is no such performance issue.

That is why I need to set SO_RECVBUF in this way.

If you accepted the factory patch, then I could implement the SO_RECVBUF
in a custom factory class myself. However, I feel it is a very common
requirement, so good to have this method in DelegatingDatagramSocket

I begin to understand, thank your very much for the explanation.

So actually your patch propose the following thing:

1) To create and store a static DatagramSocketFactory into the DelegatingDatagramSocket class and a static int called "defaultReceiveBufferSize".

2) When calling a DelegatingDatagramSocket constructor it creates a corresponding delegate socket from the static factory (if available). And thereafter, calls the setReceiveBufferSize(defaultReceiveBufferSize) to set the size of the super "java" DatagramSocket (which will never be used since the delegate socket is set) and not the socket created by the DatagramSocketFactory.

To conclude, it seems that you only need to have a custom SocketFactory (the definition of the received buffer size for your specific socket is done outside the scope of these patches).

Thereby, is there something which prevents the use of the Socket.setSocketImplFactory() [0] in your application (maybe some Android related issue)?

Best regards,
Vincent

[0] http://docs.oracle.com/javase/1.5.0/docs/api/java/net/Socket.html#setSocketImplFactory(java.net.SocketImplFactory)

···

On 07/07/2012 04:40 PM, Daniel Pocock wrote:

On 06/07/12 17:55, Vincent Lucas wrote:

Best regards,
Vincent

On 06/06/2012 01:18 PM, Daniel Pocock wrote:

I've run git svn rebase on my local workspace and re-generated the patch
set for all things that weren't applied by Seb in r301

This includes the items from both of my emails yesterday, and also some
formatting improvements for some of the code.

This set of patches should apply cleanly to the latest SVN

Some of these changes are necessary for Lumicall to build cleanly
against ice4j

--
Vincent Lucas, Ph.D. Jitsi developer
chenzo@jitsi.org http://jitsi.org


#7

Hello Daniel,

Hi Daniel,

Thank you very much for submitting your patches. I have started to
apply patches 1, 2, 3, 4 and 7: integrated into ICE4J build #126 (svn
revision #312).

Comments for patches 5, 6 and 7:

- #7) The property was renamed into "org.ice4j.ipv6.DISABLED" (to
respect Jitsi property name convention).

In r312, I notice you have this:

+ boolean isIPv6Disabled = StackProperties.getBoolean(
+ "org.ice4j.ipv6.DISABLED",
+ false);
+

but maybe you should have:

+ boolean isIPv6Disabled = StackProperties.getBoolean(
+ StackProperties.DISABLE_IPV6,
+ false);
+

and of course you would need to rename

+ * The name of the property used to disabled IPv6 support.
+ */
+ public static final String ENABLE_IPv6 = "org.ice4j.ipv6.DISABLED";
+

Thank you for picking up this mistake (corrected by Ice4j svn revision
#127).

- #5 and #6) Are not yet incorporated. Indeed, I do not see the point
of these patches. Maybe you can give me some additional explanation:
---- Is it designed modify only the SO_RECVBUF?
Or does your DatagramSocketFactory is there to implement another
functionality?

DatagramSocketFactory:

There is a JNI implementation of DatagramSocket in Sipdroid code. The
factory pattern allows Lumicall to use such a socket if necessary.

---- Can't you achieve the same thing by setting the SO_RECVBUF
directly by calling the
DelegatingDatagramSocket#setReceiveBufferSize(int size)?
Or by setting a custom class which extends the DataramSocket class as
a delegating socket to the DelegatingDatagramSocket?

If I let ice4j create the socket first, and then I try to set
SO_RECVBUF, it is very slow, the operation blocks for seconds

If I set the buffer size early, there is no such performance issue.

That is why I need to set SO_RECVBUF in this way.

If you accepted the factory patch, then I could implement the SO_RECVBUF
in a custom factory class myself. However, I feel it is a very common
requirement, so good to have this method in DelegatingDatagramSocket

I begin to understand, thank your very much for the explanation.

So actually your patch propose the following thing:

1) To create and store a static DatagramSocketFactory into the
DelegatingDatagramSocket class and a static int called
"defaultReceiveBufferSize".

Yes, these are actually two independent patches, they could be committed
independently as distinct revisions in SVN

I can re-write them as such if that makes it easier

2) When calling a DelegatingDatagramSocket constructor it creates a
corresponding delegate socket from the static factory (if available).
And thereafter, calls the
setReceiveBufferSize(defaultReceiveBufferSize) to set the size of the
super "java" DatagramSocket (which will never be used since the
delegate socket is set) and not the socket created by the
DatagramSocketFactory.

To conclude, it seems that you only need to have a custom
SocketFactory (the definition of the received buffer size for your
specific socket is done outside the scope of these patches).

Not necessarily - a SIP over UDP user might want a different RX buffer
size for the SIP and RTP sockets

Therefore, it is good to have a setDefaultReceiveBufferSize method just
for ice4j to create suitable RTP sockets

Thereby, is there something which prevents the use of the
Socket.setSocketImplFactory() [0] in your application (maybe some
Android related issue)?

Yes: notice in the javadoc [0], "The factory can be specified only once.
" - this is not so flexible.

In particular, my application also uses UDP sockets for various things,
such as DNS (dnsjava.jar) and SIP and even the push-to-talk mode
(multicast RTP with no ICE). It might not be desirable to use the same
socket factory for each protocol.

···

On 09/07/12 14:57, Vincent Lucas wrote:

On 07/07/2012 04:40 PM, Daniel Pocock wrote:

On 06/07/12 17:55, Vincent Lucas wrote:

Best regards,
Vincent

[0]
http://docs.oracle.com/javase/1.5.0/docs/api/java/net/Socket.html#setSocketImplFactory(java.net.SocketImplFactory)

Best regards,
Vincent

On 06/06/2012 01:18 PM, Daniel Pocock wrote:

I've run git svn rebase on my local workspace and re-generated the
patch
set for all things that weren't applied by Seb in r301

This includes the items from both of my emails yesterday, and also
some
formatting improvements for some of the code.

This set of patches should apply cleanly to the latest SVN

Some of these changes are necessary for Lumicall to build cleanly
against ice4j


#8

Hi Daniel,

Your patches 5 and 6 are commited to ice4j svn revision #314.

I have tried to lighten the modifications, thus the DatagramSocketFactory interface is limited to a single function "createUnboundDatagramSocket()", which must return an unbound DatagramSocket: i.e. return new DatagramSocket((SocketAddress) null).
I hope this include your use case.

Thank you once again for submitting these patches and for your explanations.

Regards,
Vincent

···

On 07/09/2012 07:37 PM, Daniel Pocock wrote:

On 09/07/12 14:57, Vincent Lucas wrote:

Hello Daniel,

On 07/07/2012 04:40 PM, Daniel Pocock wrote:

On 06/07/12 17:55, Vincent Lucas wrote:

Hi Daniel,

Thank you very much for submitting your patches. I have started to
apply patches 1, 2, 3, 4 and 7: integrated into ICE4J build #126 (svn
revision #312).

Comments for patches 5, 6 and 7:

- #7) The property was renamed into "org.ice4j.ipv6.DISABLED" (to
respect Jitsi property name convention).

In r312, I notice you have this:

+ boolean isIPv6Disabled = StackProperties.getBoolean(
+ "org.ice4j.ipv6.DISABLED",
+ false);
+

but maybe you should have:

+ boolean isIPv6Disabled = StackProperties.getBoolean(
+ StackProperties.DISABLE_IPV6,
+ false);
+

and of course you would need to rename

+ * The name of the property used to disabled IPv6 support.
+ */
+ public static final String ENABLE_IPv6 = "org.ice4j.ipv6.DISABLED";
+

Thank you for picking up this mistake (corrected by Ice4j svn revision
#127).

- #5 and #6) Are not yet incorporated. Indeed, I do not see the point
of these patches. Maybe you can give me some additional explanation:
---- Is it designed modify only the SO_RECVBUF?
Or does your DatagramSocketFactory is there to implement another
functionality?

DatagramSocketFactory:

There is a JNI implementation of DatagramSocket in Sipdroid code. The
factory pattern allows Lumicall to use such a socket if necessary.

---- Can't you achieve the same thing by setting the SO_RECVBUF
directly by calling the
DelegatingDatagramSocket#setReceiveBufferSize(int size)?
Or by setting a custom class which extends the DataramSocket class as
a delegating socket to the DelegatingDatagramSocket?

If I let ice4j create the socket first, and then I try to set
SO_RECVBUF, it is very slow, the operation blocks for seconds

If I set the buffer size early, there is no such performance issue.

That is why I need to set SO_RECVBUF in this way.

If you accepted the factory patch, then I could implement the SO_RECVBUF
in a custom factory class myself. However, I feel it is a very common
requirement, so good to have this method in DelegatingDatagramSocket

I begin to understand, thank your very much for the explanation.

So actually your patch propose the following thing:

1) To create and store a static DatagramSocketFactory into the
DelegatingDatagramSocket class and a static int called
"defaultReceiveBufferSize".

Yes, these are actually two independent patches, they could be committed
independently as distinct revisions in SVN

I can re-write them as such if that makes it easier

2) When calling a DelegatingDatagramSocket constructor it creates a
corresponding delegate socket from the static factory (if available).
And thereafter, calls the
setReceiveBufferSize(defaultReceiveBufferSize) to set the size of the
super "java" DatagramSocket (which will never be used since the
delegate socket is set) and not the socket created by the
DatagramSocketFactory.

To conclude, it seems that you only need to have a custom
SocketFactory (the definition of the received buffer size for your
specific socket is done outside the scope of these patches).

Not necessarily - a SIP over UDP user might want a different RX buffer
size for the SIP and RTP sockets

Therefore, it is good to have a setDefaultReceiveBufferSize method just
for ice4j to create suitable RTP sockets

Thereby, is there something which prevents the use of the
Socket.setSocketImplFactory() [0] in your application (maybe some
Android related issue)?

Yes: notice in the javadoc [0], "The factory can be specified only once.
" - this is not so flexible.

In particular, my application also uses UDP sockets for various things,
such as DNS (dnsjava.jar) and SIP and even the push-to-talk mode
(multicast RTP with no ICE). It might not be desirable to use the same
socket factory for each protocol.

Best regards,
Vincent

[0]
http://docs.oracle.com/javase/1.5.0/docs/api/java/net/Socket.html#setSocketImplFactory(java.net.SocketImplFactory)

Best regards,
Vincent

On 06/06/2012 01:18 PM, Daniel Pocock wrote:

I've run git svn rebase on my local workspace and re-generated the
patch
set for all things that weren't applied by Seb in r301

This includes the items from both of my emails yesterday, and also
some
formatting improvements for some of the code.

This set of patches should apply cleanly to the latest SVN

Some of these changes are necessary for Lumicall to build cleanly
against ice4j

--
Vincent Lucas, Ph.D. Jitsi developer
chenzo@jitsi.org http://jitsi.org


#9

I think that is sufficient - I have modified the Lumicall code, now
testing it

Thank you for integrating these changes.

Could you please include .classpath and .project for Eclipse in the top
of the ice4j tree? I have attached samples.

.classpath (501 Bytes)

.project (364 Bytes)

···

On 11/07/12 15:25, Vincent Lucas wrote:

Hi Daniel,

Your patches 5 and 6 are commited to ice4j svn revision #314.

I have tried to lighten the modifications, thus the
DatagramSocketFactory interface is limited to a single function
"createUnboundDatagramSocket()", which must return an unbound
DatagramSocket: i.e. return new DatagramSocket((SocketAddress) null).
I hope this include your use case.