[PATCH] rtsp - alternate protocol
Hi, attached patch allows the rtsp module to retry with alternate protocols if the first try fails. This allows the default setup to work with both UDP-only and TCP-only servers, even if the default is set to the wrong one. Ronald
Ronald S. Bultje wrote:
Hi,
attached patch allows the rtsp module to retry with alternate protocols if the first try fails. This allows the default setup to work with both UDP-only and TCP-only servers, even if the default is set to the wrong one.
Could you try to use something different than a goto? lu -- Luca Barbato Gentoo Council Member Gentoo/linux Gentoo/PPC http://dev.gentoo.org/~lu_zero
Luca Barbato <lu_zero at gentoo.org> writes:
Ronald S. Bultje wrote:
Hi,
attached patch allows the rtsp module to retry with alternate protocols if the first try fails. This allows the default setup to work with both UDP-only and TCP-only servers, even if the default is set to the wrong one.
Could you try to use something different than a goto?
What's to complain about regarding that goto? -- M?ns Rullg?rd mans at mansr.com
M?ns Rullg?rd wrote:
Luca Barbato <lu_zero at gentoo.org> writes:
Ronald S. Bultje wrote:
Hi,
attached patch allows the rtsp module to retry with alternate protocols if the first try fails. This allows the default setup to work with both UDP-only and TCP-only servers, even if the default is set to the wrong one.
Could you try to use something different than a goto?
What's to complain about regarding that goto?
I don't like the way it is used here. Ideally the best would be something that iterates over using a while. That would make simpler and less cluttered add more transports later. lu -- Luca Barbato Gentoo Council Member Gentoo/linux Gentoo/PPC http://dev.gentoo.org/~lu_zero
On Fri, Dec 28, 2007 at 04:29:03PM +0100, Luca Barbato wrote:
M?ns Rullg?rd wrote:
Luca Barbato <lu_zero at gentoo.org> writes:
Ronald S. Bultje wrote:
Hi,
attached patch allows the rtsp module to retry with alternate protocols if the first try fails. This allows the default setup to work with both UDP-only and TCP-only servers, even if the default is set to the wrong one.
Could you try to use something different than a goto?
What's to complain about regarding that goto?
I don't like the way it is used here.
Ideally the best would be something that iterates over using a while.
That would make simpler and less cluttered add more transports later.
I dont understand why you need a goto or while() ? a simple: if(protocol_mask & BLAH_UDP){ try udp if(succeess) return 0 } if(protocol_mask & BLAH_TCP){ try tcp if(succeess) return 0; } return ESOME_APPROPRIAT_EERROR; [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I hate to see young programmers poisoned by the kind of thinking Ulrich Drepper puts forward since it is simply too narrow -- Roman Shaposhnik
Hi, On Dec 28, 2007 10:59 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
I dont understand why you need a goto or while() ?
a simple: if(protocol_mask & BLAH_UDP){ try udp if(succeess) return 0 } if(protocol_mask & BLAH_TCP){ try tcp if(succeess) return 0; }
Most code between udp, udp-multicast and tcp is shared, so this'd lead to duplication between try udp and try tcp. The for is already there, I can just re-use that. I can also drop rtsp_default_mask, as Michael suggested, I actually wanted to do that when I thought about it later, but I was already off by then. New patch doing that attached, interested in comments on this approach. Ronald
On Fri, 28 Dec 2007 12:10:48 -0500 "Ronald S. Bultje" <rsbultje at gmail.com> wrote:
Hi,
On Dec 28, 2007 10:59 AM, Michael Niedermayer <michaelni at gmx.at> wrote:
I dont understand why you need a goto or while() ?
a simple: if(protocol_mask & BLAH_UDP){ try udp if(succeess) return 0 } if(protocol_mask & BLAH_TCP){ try tcp if(succeess) return 0; }
Most code between udp, udp-multicast and tcp is shared, so this'd lead to duplication between try udp and try tcp. The for is already there, I can just re-use that.
I can also drop rtsp_default_mask, as Michael suggested, I actually wanted to do that when I thought about it later, but I was already off by then. New patch doing that attached, interested in comments on this approach.
[...]
Index: ffmpeg/libavformat/rtsp.h =================================================================== --- ffmpeg.orig/libavformat/rtsp.h 2007-12-27 17:39:59.000000000 -0500 +++ ffmpeg/libavformat/rtsp.h 2007-12-28 11:57:57.000000000 -0500 @@ -29,6 +29,7 @@ RTSP_PROTOCOL_RTP_UDP = 0, RTSP_PROTOCOL_RTP_TCP = 1, RTSP_PROTOCOL_RTP_UDP_MULTICAST = 2, + RTSP_PROTOCOL_RTP_LAST = 3 };
You shouldn't set an explict value to RTP_LAST to make adding new protocol easier. Moreover, I guess you don't want it to be part of API/ABI. This would require bumping major version when adding a new protocol. So you should probably add a comment such as: /**< This is not part of public API and shouldn't be used outside of ffmpeg */
Index: ffmpeg/libavformat/rtsp.c =================================================================== --- ffmpeg.orig/libavformat/rtsp.c 2007-12-28 12:08:05.000000000 -0500 +++ ffmpeg/libavformat/rtsp.c 2007-12-28 12:09:21.000000000 -0500 @@ -78,8 +78,6 @@ /* XXX: currently, the only way to change the protocols consists in changing this variable */
-int rtsp_default_protocols = (1 << RTSP_PROTOCOL_RTP_UDP);
Strictly speeking, this breaks API/ABI so this requires major version bump. Aurel
On Fri, 28 Dec 2007 14:24:33 -0500 "Ronald S. Bultje" <rsbultje at gmail.com> wrote:
Hi,
On Dec 28, 2007 2:19 PM, Aurelien Jacobs <aurel at gnuage.org> wrote:
-int rtsp_default_protocols = (1 << RTSP_PROTOCOL_RTP_UDP);
Strictly speeking, this breaks API/ABI so this requires major version bump.
Do I need to put an #ifdef VERSION < xxx around it?
Either an #ifdef VERSION or a version bump. I personaly don't care. Aurel
Hi, On Dec 28, 2007 2:42 PM, Aurelien Jacobs <aurel at gnuage.org> wrote:
Either an #ifdef VERSION or a version bump. I personaly don't care.
No need yet for a version bump, this stuff will go away by itself... New version of patch#3 attached. Ronald
On Fri, Dec 28, 2007 at 02:57:08PM -0500, Ronald S. Bultje wrote:
Hi,
On Dec 28, 2007 2:42 PM, Aurelien Jacobs <aurel at gnuage.org> wrote:
Either an #ifdef VERSION or a version bump. I personaly don't care.
No need yet for a version bump, this stuff will go away by itself... New version of patch#3 attached.
patch ok [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Observe your enemies, for they first find out your faults. -- Antisthenes
Hi, On Dec 28, 2007 3:53 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Fri, Dec 28, 2007 at 02:57:08PM -0500, Ronald S. Bultje wrote:
On Dec 28, 2007 2:42 PM, Aurelien Jacobs <aurel at gnuage.org> wrote:
Either an #ifdef VERSION or a version bump. I personaly don't care.
No need yet for a version bump, this stuff will go away by itself... New version of patch#3 attached.
patch ok
Here's a re-post of all three then, just to make it easier to apply. Ronald
On Fri, 28 Dec 2007 16:23:45 -0500 "Ronald S. Bultje" <rsbultje at gmail.com> wrote:
Hi,
On Dec 28, 2007 3:53 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Fri, Dec 28, 2007 at 02:57:08PM -0500, Ronald S. Bultje wrote:
On Dec 28, 2007 2:42 PM, Aurelien Jacobs <aurel at gnuage.org> wrote:
Either an #ifdef VERSION or a version bump. I personaly don't care.
No need yet for a version bump, this stuff will go away by itself... New version of patch#3 attached.
patch ok
Here's a re-post of all three then, just to make it easier to apply.
[...]
Index: ffmpeg/libavformat/rtsp.h =================================================================== --- ffmpeg.orig/libavformat/rtsp.h 2007-12-28 13:30:50.000000000 -0500 +++ ffmpeg/libavformat/rtsp.h 2007-12-28 13:33:11.000000000 -0500 @@ -29,6 +29,7 @@ RTSP_PROTOCOL_RTP_UDP = 0, RTSP_PROTOCOL_RTP_TCP = 1, RTSP_PROTOCOL_RTP_UDP_MULTICAST = 2, + RTSP_PROTOCOL_RTP_LAST = 3 };
You still specify an explicit value for RTP_LAST, which is bad IMO. Have you read my previous comments about this ? Aurel
Hi, On Dec 28, 2007 5:04 PM, Aurelien Jacobs <aurel at gnuage.org> wrote:
You still specify an explicit value for RTP_LAST, which is bad IMO. Have you read my previous comments about this ?
Oh, right, I intended to add the comment you mentioned, but forgot, sorry. New patch will come tomorrow, I was about to go off to bed. Ronald
On Fri, Dec 28, 2007 at 04:23:45PM -0500, Ronald S. Bultje wrote:
Hi,
On Dec 28, 2007 3:53 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Fri, Dec 28, 2007 at 02:57:08PM -0500, Ronald S. Bultje wrote:
On Dec 28, 2007 2:42 PM, Aurelien Jacobs <aurel at gnuage.org> wrote:
Either an #ifdef VERSION or a version bump. I personaly don't care.
No need yet for a version bump, this stuff will go away by itself... New version of patch#3 attached.
patch ok
Here's a re-post of all three then, just to make it easier to apply.
Ronald
Index: ffmpeg/libavformat/rtsp.c =================================================================== --- ffmpeg.orig/libavformat/rtsp.c 2007-12-28 13:30:50.000000000 -0500 +++ ffmpeg/libavformat/rtsp.c 2007-12-28 13:33:47.000000000 -0500 @@ -842,6 +842,25 @@ av_free(rt->rtsp_streams); }
+static inline int +num_bits_set(int mask) +{ + int i, n = 0;; + for (i = 0; i < sizeof(int) * 8; i++) + if (mask & (1 << i)) + n++; + return n; +} +static inline int +unset_lowest_bit(int mask) +{ + int i; + for (i = 0; i < sizeof(int) * 8; i++) + if (mask & (1 << i)) + return mask &~ (1 << i); + return 0; +}
forget this mess it could be done with one line x&(x-1) but besides i dont see why any of this would be needed
+ static int rtsp_read_header(AVFormatContext *s, AVFormatParameters *ap) { @@ -883,7 +902,8 @@ }
if (!protocol_mask) - protocol_mask = rtsp_default_protocols; + for (i = 0; i < RTSP_PROTOCOL_RTP_LAST; i++) + protocol_mask |= (1 << i);
this is (1<<RTSP_PROTOCOL_RTP_LAST) - 1 but i really dont like it
/* open the tcp connexion */ snprintf(tcpname, sizeof(tcpname), "tcp://%s:%d", host, port); @@ -978,6 +998,11 @@ "Transport: %s\r\n", rtsp_st->control_url, transport); rtsp_send_cmd(s, cmd, reply, NULL); + if (reply->status_code == 461 /* Unsupported protocol */ && i == 0 && + num_bits_set(protocol_mask) >= 2) { + protocol_mask = unset_lowest_bit(protocol_mask);
+ j = RTSP_RTP_PORT_MIN; i--; continue;
NO, try to write clean code, please! or it will take till 2009 to get this into commitable shape
+ } else
else after continue, whats the sense of this? error -> remove bit from protocol_mask protocol_mask == 0 -> fail else try next Also there is NO reason why the protocols would be choosen in the same order as the bits, unset_lowest_bit() is a fragile hack when someone changes the order it breaks silently [...] [rtsp_default_protocols] Still ok, though its badly incomplete but its ok as first step, it does no harm as is ... And please do not send more than a single patch per email and please do not repost patches, always reply if needed. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Breaking DRM is a little like attempting to break through a door even though the window is wide open and the only thing in the house is a bunch of things you dont want and which you would get tomorrow for free anyway
Hi, On Dec 28, 2007 5:12 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
@@ -978,6 +998,11 @@ "Transport: %s\r\n", rtsp_st->control_url, transport); rtsp_send_cmd(s, cmd, reply, NULL); + if (reply->status_code == 461 /* Unsupported protocol */ && i == 0 && + num_bits_set(protocol_mask) >= 2) { + protocol_mask = unset_lowest_bit(protocol_mask);
+ j = RTSP_RTP_PORT_MIN; i--; continue;
NO, try to write clean code, please! or it will take till 2009 to get this into commitable shape
Well, for what looks easy to implement to me (= small patches), it's either a goto (which people disliked) or this. Are there other ways to do this? Ronald
On Sat, Dec 29, 2007 at 04:15:34AM -0500, Ronald S. Bultje wrote:
Hi,
On Dec 28, 2007 5:12 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
@@ -978,6 +998,11 @@ "Transport: %s\r\n", rtsp_st->control_url, transport); rtsp_send_cmd(s, cmd, reply, NULL); + if (reply->status_code == 461 /* Unsupported protocol */ && i == 0 && + num_bits_set(protocol_mask) >= 2) { + protocol_mask = unset_lowest_bit(protocol_mask);
+ j = RTSP_RTP_PORT_MIN; i--; continue;
NO, try to write clean code, please! or it will take till 2009 to get this into commitable shape
Well, for what looks easy to implement to me (= small patches), it's either a goto (which people disliked) or this. Are there other ways to do this?
what i meant was: j = RTSP_RTP_PORT_MIN; i--; continue; instead of j = RTSP_RTP_PORT_MIN; i--; continue; [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I wish the Xiph folks would stop pretending they've got something they do not. Somehow I fear this will remain a wish. -- M?ns Rullg?rd
Hi, On Dec 28, 2007 5:12 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
On Fri, Dec 28, 2007 at 04:23:45PM -0500, Ronald S. Bultje wrote:
@@ -842,6 +842,25 @@ av_free(rt->rtsp_streams); }
+static inline int +num_bits_set(int mask) +{ + int i, n = 0;; + for (i = 0; i < sizeof(int) * 8; i++) + if (mask & (1 << i)) + n++; + return n; +} +static inline int +unset_lowest_bit(int mask) +{ + int i; + for (i = 0; i < sizeof(int) * 8; i++) + if (mask & (1 << i)) + return mask &~ (1 << i); + return 0; +}
forget this mess it could be done with one line x&(x-1) but besides i dont see why any of this would be needed
I kept the second, in a modified form (see below).
@@ -883,7 +902,8 @@ }
if (!protocol_mask) - protocol_mask = rtsp_default_protocols; + for (i = 0; i < RTSP_PROTOCOL_RTP_LAST; i++) + protocol_mask |= (1 << i);
this is (1<<RTSP_PROTOCOL_RTP_LAST) - 1 but i really dont like it
I think this is the easiest way. Why else is it a bitmask?
@@ -978,6 +998,11 @@ "Transport: %s\r\n", rtsp_st->control_url, transport); rtsp_send_cmd(s, cmd, reply, NULL); + if (reply->status_code == 461 /* Unsupported protocol */ && i == 0 && + num_bits_set(protocol_mask) >= 2) { + protocol_mask = unset_lowest_bit(protocol_mask);
+ j = RTSP_RTP_PORT_MIN; i--; continue;
NO, try to write clean code, please! or it will take till 2009 to get this into commitable shape
Changed as suggested.
+ } else
else after continue, whats the sense of this?
This is more for indenting consistency, I personally find if/else blocks to read easier this way, even if the else isn't needed.
error -> remove bit from protocol_mask protocol_mask == 0 -> fail else try next
I used this instead now.
Also there is NO reason why the protocols would be choosen in the same order as the bits, unset_lowest_bit() is a fragile hack when someone changes the order it breaks silently
I modified this into explicit ordering as used in _open(), that is the best way to do it (i.e. even if bit order changes, we're guaranteed to try each exactly once.
And please do not send more than a single patch per email and please do not repost patches, always reply if needed.
What if the second is just a reindent of #1? New patch (only this one) attached. Ronald
Hi, On Dec 29, 2007 12:41 PM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
New patch (only this one) attached.
Would anyone have time to review this one? Or should I leave this to Luca who's now also working on this? Ronald
Hi Ronald, Ronald S. Bultje wrote:
Hi,
On Dec 29, 2007 12:41 PM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
New patch (only this one) attached.
Would anyone have time to review this one? Maybe Luca B. (the RTSP maintainer) can comment on it (if he did not already).
Or should I leave this to Luca who's now also working on this? I am not really working on this part of the transport protocol selection: I just wrote a patch to allow forcing a specific protocol (this used to be possible by using rtsp_default_protocols, and is not possible when such variable is removed).
Since my patch is touching the code modified by your one, I was thinking about re-generating the patch after your one has been applied. Thanks, Luca
On Sat, Dec 29, 2007 at 12:41:09PM -0500, Ronald S. Bultje wrote: [...]
Index: ffmpeg/libavformat/rtsp.c =================================================================== --- ffmpeg.orig/libavformat/rtsp.c 2007-12-28 13:30:50.000000000 -0500 +++ ffmpeg/libavformat/rtsp.c 2007-12-29 12:19:33.000000000 -0500 @@ -842,6 +842,18 @@ av_free(rt->rtsp_streams); }
+static int +unset_lowest_protocol(int mask) +{ + if (mask & RTSP_PROTOCOL_RTP_UDP) + return mask & ~RTSP_PROTOCOL_RTP_UDP; + else if (mask & RTSP_PROTOCOL_RTP_TCP) + return mask & ~RTSP_PROTOCOL_RTP_TCP; + else if (mask & RTSP_PROTOCOL_RTP_TCP) + return mask & ~RTSP_PROTOCOL_RTP_UDP_MULTICAST; + return 0; +} + static int rtsp_read_header(AVFormatContext *s, AVFormatParameters *ap) { @@ -883,7 +895,7 @@ }
if (!protocol_mask) - protocol_mask = rtsp_default_protocols; + protocol_mask = (1<<RTSP_PROTOCOL_RTP_LAST) - 1;
/* open the tcp connexion */ snprintf(tcpname, sizeof(tcpname), "tcp://%s:%d", host, port); @@ -978,6 +990,12 @@ "Transport: %s\r\n", rtsp_st->control_url, transport); rtsp_send_cmd(s, cmd, reply, NULL); + if (reply->status_code == 461 /* Unsupported protocol */ && i == 0 && + (protocol_mask = unset_lowest_protocol(protocol_mask)) != 0) { + j = RTSP_RTP_PORT_MIN; + i--; + continue; + } else
the code still breaks if i reorder the ifs() for TCP and UDP also the way you implement this is ugly you just list a bunch of variables, reset them and run a continue this will just break if any new variable is added you must re execute the original code which sets the variables NOT duplicate it and iam almost certain this code leaks everything from allocated memory to various opened streams the loop looks like it is for opening multiple streams and NOT for iterating over protocols, a continue should only be executed to go to the next stream NEVER to go back to a previous stream after hacking 2 variables and leaving all allocated resources for the streams to leak [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Breaking DRM is a little like attempting to break through a door even though the window is wide open and the only thing in the house is a bunch of things you dont want and which you would get tomorrow for free anyway
Hi, On Jan 4, 2008 7:14 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
Index: ffmpeg/libavformat/rtsp.c =================================================================== --- ffmpeg.orig/libavformat/rtsp.c 2007-12-28 13:30:50.000000000-0500 +++ ffmpeg/libavformat/rtsp.c 2007-12-29 12:19:33.000000000 -0500 @@ -842,6 +842,18 @@ av_free(rt->rtsp_streams); }
+static int +unset_lowest_protocol(int mask) +{ + if (mask & RTSP_PROTOCOL_RTP_UDP) + return mask & ~RTSP_PROTOCOL_RTP_UDP; + else if (mask & RTSP_PROTOCOL_RTP_TCP) + return mask & ~RTSP_PROTOCOL_RTP_TCP; + else if (mask & RTSP_PROTOCOL_RTP_TCP) + return mask & ~RTSP_PROTOCOL_RTP_UDP_MULTICAST; + return 0; +} + static int rtsp_read_header(AVFormatContext *s, AVFormatParameters *ap) { @@ -883,7 +895,7 @@ }
if (!protocol_mask) - protocol_mask = rtsp_default_protocols; + protocol_mask = (1<<RTSP_PROTOCOL_RTP_LAST) - 1;
/* open the tcp connexion */ snprintf(tcpname, sizeof(tcpname), "tcp://%s:%d", host, port); @@ -978,6 +990,12 @@ "Transport: %s\r\n", rtsp_st->control_url, transport); rtsp_send_cmd(s, cmd, reply, NULL); + if (reply->status_code == 461 /* Unsupported protocol */ && i == 0 && + (protocol_mask = unset_lowest_protocol(protocol_mask)) !=
On Sat, Dec 29, 2007 at 12:41:09PM -0500, Ronald S. Bultje wrote: [...] 0) {
+ j = RTSP_RTP_PORT_MIN; + i--; + continue; + } else
the code still breaks if i reorder the ifs() for TCP and UDP also the way you implement this is ugly you just list a bunch of variables, reset them and run a continue this will just break if any new variable is added you must re execute the original code which sets the variables NOT duplicate it
and iam almost certain this code leaks everything from allocated memory to various opened streams the loop looks like it is for opening multiple streams and NOT for iterating over protocols, a continue should only be executed to go to the next stream NEVER to go back to a previous stream after hacking 2 variables and leaving all allocated resources for the streams to leak
So what's the preferred way to fix this, move code into a separate function and add a fail: handler in it to free memory, return an error and re-try with another protocol? If you dislike the continue (which I understand and agree with), and other dislike the goto (which I would normally prefer), then that's the last thing I can think of to do this... Ronald
On Sat, Jan 05, 2008 at 09:21:52AM -0500, Ronald S. Bultje wrote:
Hi,
On Jan 4, 2008 7:14 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
Index: ffmpeg/libavformat/rtsp.c =================================================================== --- ffmpeg.orig/libavformat/rtsp.c 2007-12-28 13:30:50.000000000-0500 +++ ffmpeg/libavformat/rtsp.c 2007-12-29 12:19:33.000000000 -0500 @@ -842,6 +842,18 @@ av_free(rt->rtsp_streams); }
+static int +unset_lowest_protocol(int mask) +{ + if (mask & RTSP_PROTOCOL_RTP_UDP) + return mask & ~RTSP_PROTOCOL_RTP_UDP; + else if (mask & RTSP_PROTOCOL_RTP_TCP) + return mask & ~RTSP_PROTOCOL_RTP_TCP; + else if (mask & RTSP_PROTOCOL_RTP_TCP) + return mask & ~RTSP_PROTOCOL_RTP_UDP_MULTICAST; + return 0; +} + static int rtsp_read_header(AVFormatContext *s, AVFormatParameters *ap) { @@ -883,7 +895,7 @@ }
if (!protocol_mask) - protocol_mask = rtsp_default_protocols; + protocol_mask = (1<<RTSP_PROTOCOL_RTP_LAST) - 1;
/* open the tcp connexion */ snprintf(tcpname, sizeof(tcpname), "tcp://%s:%d", host, port); @@ -978,6 +990,12 @@ "Transport: %s\r\n", rtsp_st->control_url, transport); rtsp_send_cmd(s, cmd, reply, NULL); + if (reply->status_code == 461 /* Unsupported protocol */ && i == 0 && + (protocol_mask = unset_lowest_protocol(protocol_mask)) !=
On Sat, Dec 29, 2007 at 12:41:09PM -0500, Ronald S. Bultje wrote: [...] 0) {
+ j = RTSP_RTP_PORT_MIN; + i--; + continue; + } else
the code still breaks if i reorder the ifs() for TCP and UDP also the way you implement this is ugly you just list a bunch of variables, reset them and run a continue this will just break if any new variable is added you must re execute the original code which sets the variables NOT duplicate it
and iam almost certain this code leaks everything from allocated memory to various opened streams the loop looks like it is for opening multiple streams and NOT for iterating over protocols, a continue should only be executed to go to the next stream NEVER to go back to a previous stream after hacking 2 variables and leaving all allocated resources for the streams to leak
So what's the preferred way to fix this, move code into a separate function and add a fail: handler in it to free memory, return an error and re-try with another protocol? If you dislike the continue (which I understand and agree with), and other dislike the goto (which I would normally prefer), then that's the last thing I can think of to do this...
Dont ask me how to do it, i dont know the code though the suggestion of a seperate function sounds good ... maybe something like: balh(context, protocol){ if(protocol == TCP){ }else if(protocol == UDP){ } common code return 0; fail: cleanup } while(protocol_mask){ int protocol= protocol_mask & ~(protocol_mask-1); if(balh(context, protocol) >= 0) break; protocol_mask -= protocol; } [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Republics decline into democracies and democracies degenerate into despotisms. -- Aristotle
Hi, 2008/1/5 Michael Niedermayer <michaelni at gmx.at>:
On Sat, Jan 05, 2008 at 09:21:52AM -0500, Ronald S. Bultje wrote:
Hi,
On Jan 4, 2008 7:14 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
Index: ffmpeg/libavformat/rtsp.c =================================================================== --- ffmpeg.orig/libavformat/rtsp.c 2007-12-28 13:30: 50.000000000-0500 +++ ffmpeg/libavformat/rtsp.c 2007-12-29 12:19:33.000000000 -0500 @@ -842,6 +842,18 @@ av_free(rt->rtsp_streams); }
+static int +unset_lowest_protocol(int mask) +{ + if (mask & RTSP_PROTOCOL_RTP_UDP) + return mask & ~RTSP_PROTOCOL_RTP_UDP; + else if (mask & RTSP_PROTOCOL_RTP_TCP) + return mask & ~RTSP_PROTOCOL_RTP_TCP; + else if (mask & RTSP_PROTOCOL_RTP_TCP) + return mask & ~RTSP_PROTOCOL_RTP_UDP_MULTICAST; + return 0; +} + static int rtsp_read_header(AVFormatContext *s, AVFormatParameters *ap) { @@ -883,7 +895,7 @@ }
if (!protocol_mask) - protocol_mask = rtsp_default_protocols; + protocol_mask = (1<<RTSP_PROTOCOL_RTP_LAST) - 1;
/* open the tcp connexion */ snprintf(tcpname, sizeof(tcpname), "tcp://%s:%d", host, port); @@ -978,6 +990,12 @@ "Transport: %s\r\n", rtsp_st->control_url, transport); rtsp_send_cmd(s, cmd, reply, NULL); + if (reply->status_code == 461 /* Unsupported protocol */ && i == 0 && + (protocol_mask = unset_lowest_protocol(protocol_mask)) !=
On Sat, Dec 29, 2007 at 12:41:09PM -0500, Ronald S. Bultje wrote: [...] 0) {
+ j = RTSP_RTP_PORT_MIN; + i--; + continue; + } else
the code still breaks if i reorder the ifs() for TCP and UDP also the way you implement this is ugly you just list a bunch of variables, reset them and run a continue this will just break if any new variable is added you must re execute the original code which sets the variables NOT duplicate it
So what's the preferred way to fix this, move code into a separate function and add a fail: handler in it to free memory, return an error and re-try with another protocol?
[..]
seperate function sounds good ...
Like this? Ronald
Hi,
On Jan 18, 2008 12:24 PM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> 2008/1/5 Michael Niedermayer <michaelni at gmx.at>:
>
> > On Sat, Jan 05, 2008 at 09:21:52AM -0500, Ronald S. Bultje wrote:
> > > On Jan 4, 2008 7:14 PM, Michael Niedermayer <michaelni at gmx.at > wrote:
> > > > On Sat, Dec 29, 2007 at 12:41:09PM -0500, Ronald S. Bultje wrote:
> > > > [...]
> > > > > Index: ffmpeg/libavformat/rtsp.c
> > > > >
> > ===================================================================
> > > > > --- ffmpeg.orig/libavformat/rtsp.c 2007-12-28 13:30:
> > 50.000000000-0500
> > > > > +++ ffmpeg/libavformat/rtsp.c 2007-12-29 12:19:33.000000000 -0500
> > > > > @@ -842,6 +842,18 @@
> > > > > av_free(rt->rtsp_streams);
> > > > > }
> > > > >
> > > > > +static int
> > > > > +unset_lowest_protocol(int mask)
> > > > > +{
> > > > > + if (mask & RTSP_PROTOCOL_RTP_UDP)
> > > > > + return mask & ~RTSP_PROTOCOL_RTP_UDP;
> > > > > + else if (mask & RTSP_PROTOCOL_RTP_TCP)
> > > > > + return mask & ~RTSP_PROTOCOL_RTP_TCP;
> > > > > + else if (mask & RTSP_PROTOCOL_RTP_TCP)
> > > > > + return mask & ~RTSP_PROTOCOL_RTP_UDP_MULTICAST;
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > static int rtsp_read_header(AVFormatContext *s,
> > > > > AVFormatParameters *ap)
> > > > > {
> > > > > @@ -883,7 +895,7 @@
> > > > > }
> > > > >
> > > > > if (!protocol_mask)
> > > > > - protocol_mask = rtsp_default_protocols;
> > > > > + protocol_mask = (1<<RTSP_PROTOCOL_RTP_LAST) - 1;
> > > > >
> > > > > /* open the tcp connexion */
> > > > > snprintf(tcpname, sizeof(tcpname), "tcp://%s:%d", host,
> > port);
> > > > > @@ -978,6 +990,12 @@
> > > > > "Transport: %s\r\n",
> > > > > rtsp_st->control_url, transport);
> > > > > rtsp_send_cmd(s, cmd, reply, NULL);
> > > > > + if (reply->status_code == 461 /* Unsupported protocol */
> > && i
> > > > == 0 &&
> > > > > + (protocol_mask =
> > unset_lowest_protocol(protocol_mask)) !=
> > > > 0) {
> > > > > + j = RTSP_RTP_PORT_MIN;
> > > > > + i--;
> > > > > + continue;
> > > > > + } else
> > > >
> > > > the code still breaks if i reorder the ifs() for TCP and UDP
> > > > also the way you implement this is ugly
> > > > you just list a bunch of variables, reset them and run a continue
> > > > this will just break if any new variable is added
> > > > you must re execute the original code which sets the variables NOT
> > > > duplicate it
> > >
> > > So what's the preferred way to fix this, move code into a separate
> > function
> > > and add a fail: handler in it to free memory, return an error and
> > re-try
> > > with another protocol?
> >
> [..]
>
> > seperate function sounds good ...
>
>
> Like this?
>
Against this, the alternate-protocol.patch would look as attached.
Ronald
On Fri, Jan 18, 2008 at 12:24:31PM -0500, Ronald S. Bultje wrote:
Hi,
2008/1/5 Michael Niedermayer <michaelni at gmx.at>:
On Sat, Jan 05, 2008 at 09:21:52AM -0500, Ronald S. Bultje wrote:
Hi,
On Jan 4, 2008 7:14 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
Index: ffmpeg/libavformat/rtsp.c =================================================================== --- ffmpeg.orig/libavformat/rtsp.c 2007-12-28 13:30: 50.000000000-0500 +++ ffmpeg/libavformat/rtsp.c 2007-12-29 12:19:33.000000000 -0500 @@ -842,6 +842,18 @@ av_free(rt->rtsp_streams); }
+static int +unset_lowest_protocol(int mask) +{ + if (mask & RTSP_PROTOCOL_RTP_UDP) + return mask & ~RTSP_PROTOCOL_RTP_UDP; + else if (mask & RTSP_PROTOCOL_RTP_TCP) + return mask & ~RTSP_PROTOCOL_RTP_TCP; + else if (mask & RTSP_PROTOCOL_RTP_TCP) + return mask & ~RTSP_PROTOCOL_RTP_UDP_MULTICAST; + return 0; +} + static int rtsp_read_header(AVFormatContext *s, AVFormatParameters *ap) { @@ -883,7 +895,7 @@ }
if (!protocol_mask) - protocol_mask = rtsp_default_protocols; + protocol_mask = (1<<RTSP_PROTOCOL_RTP_LAST) - 1;
/* open the tcp connexion */ snprintf(tcpname, sizeof(tcpname), "tcp://%s:%d", host, port); @@ -978,6 +990,12 @@ "Transport: %s\r\n", rtsp_st->control_url, transport); rtsp_send_cmd(s, cmd, reply, NULL); + if (reply->status_code == 461 /* Unsupported protocol */ && i == 0 && + (protocol_mask = unset_lowest_protocol(protocol_mask)) !=
On Sat, Dec 29, 2007 at 12:41:09PM -0500, Ronald S. Bultje wrote: [...] 0) {
+ j = RTSP_RTP_PORT_MIN; + i--; + continue; + } else
the code still breaks if i reorder the ifs() for TCP and UDP also the way you implement this is ugly you just list a bunch of variables, reset them and run a continue this will just break if any new variable is added you must re execute the original code which sets the variables NOT duplicate it
So what's the preferred way to fix this, move code into a separate function and add a fail: handler in it to free memory, return an error and re-try with another protocol?
[..]
seperate function sounds good ...
Like this? [...] - if (protocol_mask & (1 << RTSP_PROTOCOL_RTP_UDP)) { + if (protocol & (1 << RTSP_PROTOCOL_RTP_UDP)) {
this renaming should be in a seperate patch and a protocol should rather be == RTSP_PROTOCOL_RTP_UDP not protocol == 1 << RTSP_PROTOCOL_RTP_UDP IMHO [...]
+ } else { + av_log(NULL, AV_LOG_ERROR, "Unknown protocol %d\n", protocol); + err = -1; + goto fail;
how can this happen? if it cant it shouldnt be checked for or a assert()
} + snprintf(cmd, sizeof(cmd),
cosmetic [...]
+fail: + return err; +}
is this sufficient cleanup? arent there some url_open() in there or something if its enough the goto fail should be replaced by return err
+ +static int rtsp_read_header(AVFormatContext *s, + AVFormatParameters *ap) +{ + RTSPState *rt = s->priv_data; + char host[1024], path[1024], tcpname[1024], cmd[2048], *option_list, *option; + URLContext *rtsp_hd; + int port, ret, err; + RTSPHeader reply1, *reply = &reply1; + unsigned char *content = NULL; + int protocol_mask = 0; + + /* extract hostname and port */ + url_split(NULL, 0, NULL, 0, + host, sizeof(host), &port, path, sizeof(path), s->filename); + if (port < 0) + port = RTSP_DEFAULT_PORT;
please seprate the cosmetic function split from the functional changes [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB In a rich man's house there is no place to spit but his face. -- Diogenes of Sinope
On Fri, Dec 28, 2007 at 09:53:24PM +0100, Michael Niedermayer wrote:
On Fri, Dec 28, 2007 at 02:57:08PM -0500, Ronald S. Bultje wrote:
On Dec 28, 2007 2:42 PM, Aurelien Jacobs <aurel at gnuage.org> wrote:
Either an #ifdef VERSION or a version bump. I personaly don't care.
No need yet for a version bump, this stuff will go away by itself... New version of patch#3 attached.
patch ok
May I propose granting Ronald commit access? His patches may need some review rounds to be acceptable, but he has a history of following through and beating them into shape. Also, there are a ton of his patches floating around and we have trouble getting all the approved ones applied. I always get confused which ones can be committed and which ones cannot. It would be a pity to have them slip through the cracks. Moreover he seems to intend to continue working on FFmpeg in the future. Admittedly, he is an auto* fanboy, but I'm sure we can make him stick to the party line eventually ;-p Diego
On Sat, Dec 29, 2007 at 12:08:59AM +0100, Diego Biurrun wrote:
On Fri, Dec 28, 2007 at 09:53:24PM +0100, Michael Niedermayer wrote:
On Fri, Dec 28, 2007 at 02:57:08PM -0500, Ronald S. Bultje wrote:
On Dec 28, 2007 2:42 PM, Aurelien Jacobs <aurel at gnuage.org> wrote:
Either an #ifdef VERSION or a version bump. I personaly don't care.
No need yet for a version bump, this stuff will go away by itself... New version of patch#3 attached.
patch ok
May I propose granting Ronald commit access?
iam against it!
His patches may need some review rounds to be acceptable, but he has a history of following through and beating them into shape. Also, there
he has a history of broken patches with alot of stuff never being approved I see RDT and related code, 90% is not in svn I see the url_split() patches, first cleanup (introduced a bug) and a purpoted fix which introduced more bugs but all this is one page of code Then the resolve_host() stuff, it should have been replaced by standard (POSIX) functions, this never happened various http improvements like seeking support, these did reach svn and work fine AFAIK Then there are many small fixes all over the place, these are nice ... I certainly dont want to loose ronald as contributor, but iam scared of him having direct svn write access! Also theres the enourmiously painfull fighting with you if id choose to revoke his account later in case it turns out that too much buggy code reaches svn ... If i just had a sourceforge like interface where i could close someones account with a mouse click i would mind less to give write access out. But with current root at mphq, no
are a ton of his patches floating around and we have trouble getting all the approved ones applied. I always get confused which ones can be committed and which ones cannot. It would be a pity to have them slip through the cracks. Moreover he seems to intend to continue working on FFmpeg in the future.
I also have my problems keeping track of his patches, i think if he would keep 1 patch per thread and never create a new thread for the same patch. Nor post another patch in the same thread, it would be much easier to keep track of his patches. Anyway its not a big issue for me to commit approved patches from him myself.
Admittedly, he is an auto* fanboy, but I'm sure we can make him stick to the party line eventually ;-p
The auto* fanboyism is not an issue. Him commiting Makefile.am changes by mistake is neither a big thing. First because he would have to svn add it first, and second because its very noticeable and easily removed again. Its messy and hackish code with subtle bugs that iam affraid of! [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB No snowflake in an avalanche ever feels responsible. -- Voltaire
Michael Niedermayer wrote:
On Sat, Dec 29, 2007 at 12:08:59AM +0100, Diego Biurrun wrote:
are a ton of his patches floating around and we have trouble getting all the approved ones applied. I always get confused which ones can be committed and which ones cannot. It would be a pity to have them slip through the cracks. Moreover he seems to intend to continue working on FFmpeg in the future.
I also have my problems keeping track of his patches, i think if he would keep 1 patch per thread and never create a new thread for the same patch. Nor post another patch in the same thread, it would be much easier to keep track of his patches. Anyway its not a big issue for me to commit approved patches from him myself.
either you or the maintainer of the code he sends patches against. I, like Diego (note that i put some comas :) ), am a bit lost with Ronald's patches... -- Ben Purple Labs S.A. www.purplelabs.com
On Sat, Dec 29, 2007 at 01:54:26AM +0100, Michael Niedermayer wrote:
On Sat, Dec 29, 2007 at 12:08:59AM +0100, Diego Biurrun wrote:
On Fri, Dec 28, 2007 at 09:53:24PM +0100, Michael Niedermayer wrote:
On Fri, Dec 28, 2007 at 02:57:08PM -0500, Ronald S. Bultje wrote:
On Dec 28, 2007 2:42 PM, Aurelien Jacobs <aurel at gnuage.org> wrote:
Either an #ifdef VERSION or a version bump. I personaly don't care.
No need yet for a version bump, this stuff will go away by itself... New version of patch#3 attached.
patch ok
May I propose granting Ronald commit access?
iam against it!
His patches may need some review rounds to be acceptable, but he has a history of following through and beating them into shape. Also, there
he has a history of broken patches with alot of stuff never being approved I see RDT and related code, 90% is not in svn I see the url_split() patches, first cleanup (introduced a bug) and a purpoted fix which introduced more bugs but all this is one page of code Then the resolve_host() stuff, it should have been replaced by standard (POSIX) functions, this never happened various http improvements like seeking support, these did reach svn and work fine AFAIK Then there are many small fixes all over the place, these are nice ...
I certainly dont want to loose ronald as contributor, but iam scared of him having direct svn write access!
All of this would only be a problem if he committed non-approved patches. Avoiding this cannot be so hard, can it?
Also theres the enourmiously painfull fighting with you if id choose to revoke his account later in case it turns out that too much buggy code reaches svn ... If i just had a sourceforge like interface where i could close someones account with a mouse click i would mind less to give write access out. But with current root at mphq, no
Nonsense.
are a ton of his patches floating around and we have trouble getting all the approved ones applied. I always get confused which ones can be committed and which ones cannot. It would be a pity to have them slip through the cracks. Moreover he seems to intend to continue working on FFmpeg in the future.
I also have my problems keeping track of his patches, i think if he would keep 1 patch per thread and never create a new thread for the same patch. Nor post another patch in the same thread, it would be much easier to keep track of his patches. Anyway its not a big issue for me to commit approved patches from him myself.
Then please commit them, because many patches are being dropped on the floor, which is very unfortunate. Diego
On Wed, Jan 02, 2008 at 08:01:00PM +0100, Diego Biurrun wrote:
On Sat, Dec 29, 2007 at 01:54:26AM +0100, Michael Niedermayer wrote:
On Sat, Dec 29, 2007 at 12:08:59AM +0100, Diego Biurrun wrote:
On Fri, Dec 28, 2007 at 09:53:24PM +0100, Michael Niedermayer wrote:
On Fri, Dec 28, 2007 at 02:57:08PM -0500, Ronald S. Bultje wrote:
On Dec 28, 2007 2:42 PM, Aurelien Jacobs <aurel at gnuage.org> wrote:
Either an #ifdef VERSION or a version bump. I personaly don't care.
No need yet for a version bump, this stuff will go away by itself... New version of patch#3 attached.
patch ok
May I propose granting Ronald commit access?
iam against it!
His patches may need some review rounds to be acceptable, but he has a history of following through and beating them into shape. Also, there
he has a history of broken patches with alot of stuff never being approved I see RDT and related code, 90% is not in svn I see the url_split() patches, first cleanup (introduced a bug) and a purpoted fix which introduced more bugs but all this is one page of code Then the resolve_host() stuff, it should have been replaced by standard (POSIX) functions, this never happened various http improvements like seeking support, these did reach svn and work fine AFAIK Then there are many small fixes all over the place, these are nice ...
I certainly dont want to loose ronald as contributor, but iam scared of him having direct svn write access!
All of this would only be a problem if he committed non-approved patches. Avoiding this cannot be so hard, can it?
Noone was able to keep track of his patches ... You assume he, himself succeeds keeping track of them :) [...]
are a ton of his patches floating around and we have trouble getting all the approved ones applied. I always get confused which ones can be committed and which ones cannot. It would be a pity to have them slip through the cracks. Moreover he seems to intend to continue working on FFmpeg in the future.
I also have my problems keeping track of his patches, i think if he would keep 1 patch per thread and never create a new thread for the same patch. Nor post another patch in the same thread, it would be much easier to keep track of his patches. Anyway its not a big issue for me to commit approved patches from him myself.
Then please commit them, because many patches are being dropped on the floor, which is very unfortunate.
Well i dont keep track of patches after approving them ... Ill change that from now on (for ronalds patches), but i cant do anything for past patches from him anymore. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I know you won't believe me, but the highest form of Human Excellence is to question oneself and others. -- Socrates
On Wed, Jan 02, 2008 at 11:25:15PM +0100, Michael Niedermayer wrote:
On Wed, Jan 02, 2008 at 08:01:00PM +0100, Diego Biurrun wrote:
On Sat, Dec 29, 2007 at 01:54:26AM +0100, Michael Niedermayer wrote:
On Sat, Dec 29, 2007 at 12:08:59AM +0100, Diego Biurrun wrote:
May I propose granting Ronald commit access?
iam against it!
His patches may need some review rounds to be acceptable, but he has a history of following through and beating them into shape. Also, there
he has a history of broken patches with alot of stuff never being approved I see RDT and related code, 90% is not in svn I see the url_split() patches, first cleanup (introduced a bug) and a purpoted fix which introduced more bugs but all this is one page of code Then the resolve_host() stuff, it should have been replaced by standard (POSIX) functions, this never happened various http improvements like seeking support, these did reach svn and work fine AFAIK Then there are many small fixes all over the place, these are nice ...
I certainly dont want to loose ronald as contributor, but iam scared of him having direct svn write access!
All of this would only be a problem if he committed non-approved patches. Avoiding this cannot be so hard, can it?
Noone was able to keep track of his patches ... You assume he, himself succeeds keeping track of them :)
Without a doubt he is in the best position to keep track of his own patches :p
Anyway its not a big issue for me to commit approved patches from him myself.
Then please commit them, because many patches are being dropped on the floor, which is very unfortunate.
Well i dont keep track of patches after approving them ... Ill change that from now on (for ronalds patches), but i cant do anything for past patches from him anymore.
Just tracking the patches is unlikely to help, commit them :p Diego
Hi, On Jan 2, 2008 5:25 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
Noone was able to keep track of his patches ... You assume he, himself succeeds keeping track of them :)<http://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel>
I use quilt, so it sort of works right now, but wouldn't with svn write access, because my patches wouldn't fit against "vanilla" svn - that's just how quilt works... If I were ever to get svn access, I'd probably move to git to make applying from random points of my tree easier... All the patches you've mentioned (including the ipv6 work) is still in my tree, I'm just waiting until the libosssup-thing has landed so I can continue from there (as suggested in the ipv6-thread). Ronald
On Wed, Jan 02, 2008 at 07:30:06PM -0500, Ronald S. Bultje wrote:
Hi,
On Jan 2, 2008 5:25 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
Noone was able to keep track of his patches ... You assume he, himself succeeds keeping track of them :)<http://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel>
I use quilt, so it sort of works right now, but wouldn't with svn write access, because my patches wouldn't fit against "vanilla" svn - that's just how quilt works... If I were ever to get svn access, I'd probably move to git to make applying from random points of my tree easier...
well, after rethinking it again iam ok with you having write access, if you agree to the development policy (http://ffmpeg.mplayerhq.hu/general.html#SEC24) and always read through svn di XYZ before svn ci XYZ and make sure its really the patch you intend to commit (yes iam paranoid) [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Frequently ignored awnser#1 FFmpeg bugs should be sent to our bugtracker, user questions for the command line tools ffmpeg, ffplay, ... as well as questions about how to use libav* should be sent to the ffmpeg-user mailinglist.
On Thu, Jan 03, 2008 at 03:44:13AM +0100, Michael Niedermayer wrote:
On Wed, Jan 02, 2008 at 07:30:06PM -0500, Ronald S. Bultje wrote:
On Jan 2, 2008 5:25 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
Noone was able to keep track of his patches ... You assume he, himself succeeds keeping track of them :)<http://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel>
I use quilt, so it sort of works right now, but wouldn't with svn write access, because my patches wouldn't fit against "vanilla" svn - that's just how quilt works... If I were ever to get svn access, I'd probably move to git to make applying from random points of my tree easier...
well, after rethinking it again iam ok with you having write access, if you agree to the development policy (http://ffmpeg.mplayerhq.hu/general.html#SEC24) and always read through svn di XYZ before svn ci XYZ and make sure its really the patch you intend to commit (yes iam paranoid)
Ronald agreed to the policy, I just created his account. His username is rbultje. Welcome aboard Ronald. Diego
On Fri, Dec 28, 2007 at 09:53:24PM +0100, Michael Niedermayer wrote:
On Fri, Dec 28, 2007 at 02:57:08PM -0500, Ronald S. Bultje wrote:
On Dec 28, 2007 2:42 PM, Aurelien Jacobs <aurel at gnuage.org> wrote:
Either an #ifdef VERSION or a version bump. I personaly don't care.
No need yet for a version bump, this stuff will go away by itself... New version of patch#3 attached.
patch ok
Applied. Diego
On Fri, Dec 28, 2007 at 07:49:39AM -0500, Ronald S. Bultje wrote:
Hi,
attached patch allows the rtsp module to retry with alternate protocols if the first try fails. This allows the default setup to work with both UDP-only and TCP-only servers, even if the default is set to the wrong one.
Ronald
Index: ffmpeg/libavformat/rtsp.c =================================================================== --- ffmpeg.orig/libavformat/rtsp.c 2007-12-28 07:23:00.000000000 -0500 +++ ffmpeg/libavformat/rtsp.c 2007-12-28 07:46:14.000000000 -0500 @@ -1025,6 +1025,7 @@ transport[0] = '\0';
/* RTP/UDP */ +retry_protocol: if (protocol_mask & (1 << RTSP_PROTOCOL_RTP_UDP)) { char buf[256];
@@ -1079,6 +1080,14 @@ rtsp_st->control_url, res, csum, transport, rt->session_id); rtsp_send_cmd(s, cmd, reply, NULL); + if (reply->status_code == 461 /* Unsupported protocol */ && i == 0 && + protocol_mask == rtsp_default_protocols && + (protocol_mask == (1 << RTSP_PROTOCOL_RTP_UDP) || + protocol_mask == (1 << RTSP_PROTOCOL_RTP_TCP))) { + protocol_mask = (protocol_mask == (1 << RTSP_PROTOCOL_RTP_UDP)) ? + (1 << RTSP_PROTOCOL_RTP_TCP) : (1 << RTSP_PROTOCOL_RTP_UDP); + goto retry_protocol; + } else
this code looks (very) buggy, i think theres no gurantee that protocol_mask == rtsp_default_protocols at the begin also if protocol_mask is either one or the other then its clear that only that should be tried to try both protocol_mask would be 3 at least half of the code in rtsp.c makes me think thats intended, the other half makes me think that protocol_mask should not be a bitmask but rather equal to 0 or 1 for UDP/TCP either way rtsp_default_protocols should be droped its redundant and a mess also it doesnt work on some OSs with PIC after -Bsymbolic has been added, not that it did work before on all ... Thats of course unrelated to the patch but the patch depends on rtsp_default_protocols which shouldnt be there at all ... [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Freedom in capitalist society always remains about the same as it was in ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
Hi Michael, Michael Niedermayer wrote: [...]
either way rtsp_default_protocols should be droped its redundant and a mess also it doesnt work on some OSs with PIC after -Bsymbolic has been added, not that it did work before on all ... Thats of course unrelated to the patch but the patch depends on rtsp_default_protocols which shouldnt be there at all ... I have a patch removing rtsp_default_protocols... I did not submit it before because it is missing the ffplay part (I just added in this moment the minimal changes needed to compile ffplay).
The patch removes rtsp_default_protocols, fixes option parsing in rtsp.c (otherwise it would be impossible to select rtp over tcp), and removes rtsp_default_protocols usage from ffplay.c. If the patch is ok, I can commit it in three times (first fix option parsing, then remove rtsp_default_protocols usage from ffplay, and then remove such variable from rtsp.c). Thanks, Luca
On Fri, Dec 28, 2007 at 06:42:29PM +0100, Luca Abeni wrote:
Hi Michael,
either way rtsp_default_protocols should be droped its redundant and a mess also it doesnt work on some OSs with PIC after -Bsymbolic has been added, not that it did work before on all ... Thats of course unrelated to the patch but the patch depends on rtsp_default_protocols which shouldnt be there at all ... I have a patch removing rtsp_default_protocols... I did not submit it before because it is missing the ffplay part (I just added in this moment
Michael Niedermayer wrote: [...] the minimal changes needed to compile ffplay).
The patch removes rtsp_default_protocols, fixes option parsing in rtsp.c (otherwise it would be impossible to select rtp over tcp), and removes rtsp_default_protocols usage from ffplay.c.
If the patch is ok, I can commit it in three times (first fix option parsing, then remove rtsp_default_protocols usage from ffplay, and then remove such variable from rtsp.c).
patch ok if the following hunk is intended [...]
@@ -864,7 +859,7 @@ port = RTSP_DEFAULT_PORT;
/* search for options */ - option_list = strchr(path, '?'); + option_list = strchr(s->filename, '?'); if (option_list) { /* remove the options from the path */ *option_list++ = 0;
[...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I am the wisest man alive, for I know one thing, and that is that I know nothing. -- Socrates
On Fri, Dec 28, 2007 at 07:28:21PM +0100, Michael Niedermayer wrote:
On Fri, Dec 28, 2007 at 06:42:29PM +0100, Luca Abeni wrote:
Hi Michael,
either way rtsp_default_protocols should be droped its redundant and a mess also it doesnt work on some OSs with PIC after -Bsymbolic has been added, not that it did work before on all ... Thats of course unrelated to the patch but the patch depends on rtsp_default_protocols which shouldnt be there at all ... I have a patch removing rtsp_default_protocols... I did not submit it before because it is missing the ffplay part (I just added in this moment
Michael Niedermayer wrote: [...] the minimal changes needed to compile ffplay).
The patch removes rtsp_default_protocols, fixes option parsing in rtsp.c (otherwise it would be impossible to select rtp over tcp), and removes rtsp_default_protocols usage from ffplay.c.
If the patch is ok, I can commit it in three times (first fix option parsing, then remove rtsp_default_protocols usage from ffplay, and then remove such variable from rtsp.c).
patch ok if the following hunk is intended
btw, i do prefer ronalds solution with respect to the rtp_tcp option, that is it should just be droped! [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Everything should be made as simple as possible, but not simpler. -- Albert Einstein
Hi Michael, Michael Niedermayer wrote: [...]
If the patch is ok, I can commit it in three times (first fix option parsing, then remove rtsp_default_protocols usage from ffplay, and then remove such variable from rtsp.c).
patch ok if the following hunk is intended
btw, i do prefer ronalds solution with respect to the rtp_tcp option, that is it should just be droped! Ok, that's even easier. Let's apply Ronald's patch first, and I'll regenerate my one later (if needed).
Thanks, Luca
If the patch is ok, I can commit it in three times (first fix option parsing, then remove rtsp_default_protocols usage from ffplay, and then remove such variable from rtsp.c).
patch ok if the following hunk is intended
[...]
@@ -864,7 +859,7 @@ port = RTSP_DEFAULT_PORT;
/* search for options */ - option_list = strchr(path, '?'); + option_list = strchr(s->filename, '?'); Yes, this is needed for unbreaking option parsing (so, without this change it is not possible to select a specific transport protocol). As far as I can see, RTSP options are currently not detected, because url_split() is removing the "?..." part of the URL from path, so
Hi Michael, Michael Niedermayer wrote: [...] option_list is always NULL... Maybe there has been a change in url_split() and the code in rtsp.c has not been updated? Thanks, Luca
On Fri, Dec 28, 2007 at 09:16:49PM +0100, Luca Abeni wrote:
Hi Michael,
If the patch is ok, I can commit it in three times (first fix option parsing, then remove rtsp_default_protocols usage from ffplay, and then remove such variable from rtsp.c).
patch ok if the following hunk is intended
[...]
@@ -864,7 +859,7 @@ port = RTSP_DEFAULT_PORT;
/* search for options */ - option_list = strchr(path, '?'); + option_list = strchr(s->filename, '?'); Yes, this is needed for unbreaking option parsing (so, without this change it is not possible to select a specific transport protocol). As far as I can see, RTSP options are currently not detected, because url_split() is removing the "?..." part of the URL from path, so
Michael Niedermayer wrote: [...] option_list is always NULL... Maybe there has been a change in url_split() and the code in rtsp.c has not been updated?
?foobar should be part of the path i think ... url_split() might be buggy ... [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Democracy is the form of government in which you can choose your dictator
Hi, On Dec 28, 2007 3:52 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
?foobar should be part of the path i think ... url_split() might be buggy ...
It may be cut off if the URL has no /, I have a vague memory of something like that. Ronald
Hi Michael, Michael Niedermayer wrote: [...]
Maybe there has been a change in url_split() and the code in rtsp.c has not been updated?
?foobar should be part of the path i think ... url_split() might be buggy ... Ok; so there is a bug in url_split()... If I use rtsp://localhost:5454/test1-rtsp.mpg?tcp as an URL, path is set to "test1-rtsp.mpg"... I think the following code (contained in url_split()) is responsible for this behaviour: /* separate path from hostname */ if ((ls = strchr(p, '/'))) { if ((q = strchr(ls, '?'))) av_strlcpy(path, ls, FFMIN(path_size, q - ls + 1)); else av_strlcpy(path, ls, path_size); } else if (!(ls = strchr(p, '?'))) ls = &p[strlen(p)]; // XXX If I understand the code correctly, it is explicitly copying in path only the part of the URL contained between "/" and "?".
Should it be fixed by removing the "strchr(p, '?')"? Wouldn't this break something else? Tomorrow I'll make some tests... Thanks, Luca
Hi Luca, On Dec 28, 2007 4:24 PM, Luca Abeni <lucabe72 at email.it> wrote:
Michael Niedermayer wrote:
Maybe there has been a change in url_split() and the code in rtsp.c has not been updated?
?foobar should be part of the path i think ... url_split() might be buggy ... Ok; so there is a bug in url_split()... If I use rtsp://localhost:5454/test1-rtsp.mpg?tcp as an URL, path is set to "test1-rtsp.mpg"... I think the following code (contained in url_split()) is responsible for this behaviour: /* separate path from hostname */ if ((ls = strchr(p, '/'))) { if ((q = strchr(ls, '?'))) av_strlcpy(path, ls, FFMIN(path_size, q - ls + 1)); else av_strlcpy(path, ls, path_size); } else if (!(ls = strchr(p, '?'))) ls = &p[strlen(p)]; // XXX If I understand the code correctly, it is explicitly copying in path only the part of the URL contained between "/" and "?".
Should it be fixed by removing the "strchr(p, '?')"? Wouldn't this break something else?
That sounds correct, I think you can commit that. Ronald
If I understand the code correctly, it is explicitly copying in path only the part of the URL contained between "/" and "?".
Should it be fixed by removing the "strchr(p, '?')"? Wouldn't this break something else?
That sounds correct, I think you can commit that. Uhmmm... That was my impression too... But this change would basically reverse r10775, which claim to fix a regression. So, fixing a bug risks to re-introduce an old regression... I need to investigate this issue a
Hi Ronald, Ronald S. Bultje wrote: [...] little bit more. Thanks, Luca
Hi Ronald, Luca Abeni wrote:
If I understand the code correctly, it is explicitly copying in path only the part of the URL contained between "/" and "?".
Should it be fixed by removing the "strchr(p, '?')"? Wouldn't this break something else?
That sounds correct, I think you can commit that. Uhmmm... That was my impression too... But this change would basically reverse r10775, which claim to fix a regression. So, fixing a bug risks to re-introduce an old regression... I need to investigate this issue a
Ronald S. Bultje wrote: [...] little bit more.
It seems that the situation is more messy than I expected: 1) url_split() has been rewritten in r10605 2) the rewrite introduced a regression, by changing the behaviour of url_split() 3) r10775 claimed to fix the regression by introducing yet another different behaviour in url_split(). And this is the change that broke option parsing in rtsp.c. Some data points: 1) Before r10605, rtsp://localhost:5454/test1-rtsp.mpg?tcp resulted in path="/test1-rtsp.mpg?tcp" 2) After r10605, the same url produced path="/test1-rtsp.mpg?tcp", so no change for this test case... 3) r10775 "fixed" something so that the result is now path="/test1-rtsp.mpg?". 4) So, I tried a different url: rtsp://localhost?tcp. The results are path="?tcp" before r10605, path="" after r10605, and path="" after r10775. Again, that commit does not seem to fix any regression. So, can anyone explain what's the supposed behaviour of url_split()? Which testcase was fixed by r10775? Ronald, it looks like this is your stuff (all these patches are by you). Can you have a look? Thanks, Luca
On Fri, Dec 28, 2007 at 11:12:54PM +0100, Luca Abeni wrote:
Hi Ronald,
Luca Abeni wrote:
If I understand the code correctly, it is explicitly copying in path only the part of the URL contained between "/" and "?".
Should it be fixed by removing the "strchr(p, '?')"? Wouldn't this break something else?
That sounds correct, I think you can commit that. Uhmmm... That was my impression too... But this change would basically reverse r10775, which claim to fix a regression. So, fixing a bug risks to re-introduce an old regression... I need to investigate this issue a
Ronald S. Bultje wrote: [...] little bit more.
It seems that the situation is more messy than I expected: 1) url_split() has been rewritten in r10605 2) the rewrite introduced a regression, by changing the behaviour of url_split() 3) r10775 claimed to fix the regression by introducing yet another different behaviour in url_split(). And this is the change that broke option parsing in rtsp.c.
argh ... ive just approved r10775 because ronald said it restores original behavior i even EXPLICITLY complained that ot would cut the ? part off see: http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2007-October/036857.html and http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2007-October/036859.html you can revert r10775 or any part of it as you see fit!
Some data points: 1) Before r10605, rtsp://localhost:5454/test1-rtsp.mpg?tcp resulted in path="/test1-rtsp.mpg?tcp" 2) After r10605, the same url produced path="/test1-rtsp.mpg?tcp", so no change for this test case... 3) r10775 "fixed" something so that the result is now path="/test1-rtsp.mpg?". 4) So, I tried a different url: rtsp://localhost?tcp. The results are path="?tcp" before r10605, path="" after r10605, and path="" after r10775. Again, that commit does not seem to fix any regression.
So, can anyone explain what's the supposed behaviour of url_split()? Which testcase was fixed by r10775?
iam not sure but if i guess i think it prevents ?tcp from ending in the hostname [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB When the tyrant has disposed of foreign enemies by conquest or treaty, and there is nothing more to fear from them, then he is always stirring up some war or other, in order that the people may require a leader. -- Plato
On Fri, Dec 28, 2007 at 11:51:37PM +0100, Michael Niedermayer wrote:
On Fri, Dec 28, 2007 at 11:12:54PM +0100, Luca Abeni wrote:
Hi Ronald,
Luca Abeni wrote:
If I understand the code correctly, it is explicitly copying in path only the part of the URL contained between "/" and "?".
Should it be fixed by removing the "strchr(p, '?')"? Wouldn't this break something else?
That sounds correct, I think you can commit that. Uhmmm... That was my impression too... But this change would basically reverse r10775, which claim to fix a regression. So, fixing a bug risks to re-introduce an old regression... I need to investigate this issue a
Ronald S. Bultje wrote: [...] little bit more.
It seems that the situation is more messy than I expected: 1) url_split() has been rewritten in r10605 2) the rewrite introduced a regression, by changing the behaviour of url_split() 3) r10775 claimed to fix the regression by introducing yet another different behaviour in url_split(). And this is the change that broke option parsing in rtsp.c.
argh ... ive just approved r10775 because ronald said it restores original behavior i even EXPLICITLY complained that ot would cut the ? part off see: http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2007-October/036857.html and http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2007-October/036859.html
you can revert r10775 or any part of it as you see fit!
Some data points: 1) Before r10605, rtsp://localhost:5454/test1-rtsp.mpg?tcp resulted in path="/test1-rtsp.mpg?tcp" 2) After r10605, the same url produced path="/test1-rtsp.mpg?tcp", so no change for this test case... 3) r10775 "fixed" something so that the result is now path="/test1-rtsp.mpg?". 4) So, I tried a different url: rtsp://localhost?tcp. The results are path="?tcp" before r10605, path="" after r10605, and path="" after r10775. Again, that commit does not seem to fix any regression.
So, can anyone explain what's the supposed behaviour of url_split()? Which testcase was fixed by r10775?
iam not sure but if i guess i think it prevents ?tcp from ending in the hostname
heres a attempt to fix it (completely untested, theres a good chance it contains trivial mistakes but ill comit it if noone points at a problem soon *g*) @@ -2941,7 +2941,7 @@ char *path, int path_size, const char *url) { - const char *p, *ls, *at, *col, *brk, *q; + const char *p, *ls, *at, *col, *brk; if (port_ptr) *port_ptr = -1; if (proto_size > 0) proto[0] = 0; @@ -2962,12 +2962,12 @@ } /* separate path from hostname */ - if ((ls = strchr(p, '/'))) { - if ((q = strchr(ls, '?'))) - av_strlcpy(path, ls, FFMIN(path_size, q - ls + 1)); - else + ls = strchr(p, '/'); + if(!ls) + ls = strchr(p, '?'); + if(ls) av_strlcpy(path, ls, path_size); - } else if (!(ls = strchr(p, '?'))) + else ls = &p[strlen(p)]; // XXX /* the rest is hostname, use that to parse auth/port */ [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is dangerous to be right in matters on which the established authorities are wrong. -- Voltaire
Hi Michael, Michael Niedermayer wrote: [...]
So, can anyone explain what's the supposed behaviour of url_split()? Which testcase was fixed by r10775? iam not sure but if i guess i think it prevents ?tcp from ending in the hostname
heres a attempt to fix it (completely untested, theres a good chance it contains trivial mistakes but ill comit it if noone points at a problem soon *g*) [...]
I tried your patch, and it obviously fixes option parsing in rtsp.c (with your patch applied, the "?..." is back in "path"). I do not know if it re-introduce the regression cured by r10775, because I did not understand the testcase... However, after applying your patch rtsp.c still has problems if options are passed in the URL: such options are now correctly recognised, but they are not removed from the URL, so the RTSP server sees requests for URLS like "rtsp://localhost:5454/test1-rtsp.mpg?tcp" instead of "rtsp://localhost:5454/test1-rtsp.mpg". This happens because the code in rtsp_read_header() removes the options from the "path" variable (while parsing them), but does not remove the options from "s->filename"; RTSP requests are forged based on s->filename, not on path... I did not notice this problem before, because I focused on the first one (fixed by your patch), and my fix for the first problem incidentally fixed the second one. I see three different solutions to the second problem (options not removed from the URL in RTSP commands): 1) changing "option_list = strchr(path, '?');" in "option_list = strchr(s->filename, '?');", as my original patch did. 2) using path instead of "s->filename" in the RTSP packets (I still have to check how invasive this patch would be) 3) modify s->filename removing the options from it too... Thanks, Luca
On Sat, Dec 29, 2007 at 09:55:08AM +0100, Luca Abeni wrote:
Hi Michael,
Michael Niedermayer wrote: [...]
So, can anyone explain what's the supposed behaviour of url_split()? Which testcase was fixed by r10775? iam not sure but if i guess i think it prevents ?tcp from ending in the hostname
heres a attempt to fix it (completely untested, theres a good chance it contains trivial mistakes but ill comit it if noone points at a problem soon *g*) [...]
I tried your patch, and it obviously fixes option parsing in rtsp.c (with your patch applied, the "?..." is back in "path"). I do not know if it re-introduce the regression cured by r10775, because I did not understand the testcase...
However, after applying your patch rtsp.c still has problems if options are passed in the URL: such options are now correctly recognised, but they are not removed from the URL, so the RTSP server sees requests for URLS like "rtsp://localhost:5454/test1-rtsp.mpg?tcp" instead of "rtsp://localhost:5454/test1-rtsp.mpg".
from the "path" variable (while parsing them), but does not remove the
This happens because the code in rtsp_read_header() removes the options options from "s->filename"; RTSP requests are forged based on s->filename, not on path... I did not notice this problem before, because I focused on the first one (fixed by your patch), and my fix for the first problem incidentally fixed the second one. I see three different solutions to the second problem (options not removed from the URL in RTSP commands): 1) changing "option_list = strchr(path, '?');" in "option_list = strchr(s->filename, '?');", as my original patch did. 2) using path instead of "s->filename" in the RTSP packets (I still have to check how invasive this patch would be) 3) modify s->filename removing the options from it too...
theres a bigger problem rtsp://localhost:5454/test1-rtsp.mpg?tcp and rtsp://localhost:5454/test1-rtsp.mpg might be 2 distinct urls, "?tcp" cannot just be dropped from the url passing application options by ?XYZ is not ok as such. What is needed first is to change the thing so it doesnt colide with valid urls. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB There will always be a question for which you do not know the correct awnser.
Hi Michael, Michael Niedermayer wrote: [...]
I see three different solutions to the second problem (options not removed from the URL in RTSP commands): 1) changing "option_list = strchr(path, '?');" in "option_list = strchr(s->filename, '?');", as my original patch did. 2) using path instead of "s->filename" in the RTSP packets (I still have to check how invasive this patch would be) 3) modify s->filename removing the options from it too...
theres a bigger problem rtsp://localhost:5454/test1-rtsp.mpg?tcp and rtsp://localhost:5454/test1-rtsp.mpg
might be 2 distinct urls, "?tcp" cannot just be dropped from the url passing application options by ?XYZ is not ok as such.
Ok... So, I guess the problem is that this code has never worked correctly. This confused me, because I was under the impression that the RTSP option parsing code used to work in the past, and I was trying to restore its original behaviour... But now I realize that it could not work, since the beginning. Hence, applying your patch seems to be the right thing to do (I am still running some tests and bisecting the svn history to fully understand this url_split() mess, but your patch looks like the safest thing to do).
What is needed first is to change the thing so it doesnt colide with valid urls.
Ok. Looking at the rest of the code, I suspect a good solution would be something like rtsp://localhost:5454?tcp/test1-rtsp.mpg... And we will have to overwrite s->filename for removing the "?..." options that are between port number and path. I'll test this idea when I'll have finished the url_split() tests... Thanks, Luca
On Sat, Dec 29, 2007 at 11:31:06AM +0100, Luca Abeni wrote: [...]
What is needed first is to change the thing so it doesnt colide with valid urls.
Ok. Looking at the rest of the code, I suspect a good solution would be something like rtsp://localhost:5454?tcp/test1-rtsp.mpg... And we will have to overwrite s->filename for removing the "?..." options that are between port number and path.
or just add a field to AVFormatContext specifying the preferred protocol [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is dangerous to be right in matters on which the established authorities are wrong. -- Voltaire
Hi Michael, Michael Niedermayer wrote: [...]
Ok. Looking at the rest of the code, I suspect a good solution would be something like rtsp://localhost:5454?tcp/test1-rtsp.mpg... And we will have to overwrite s->filename for removing the "?..." options that are between port number and path.
or just add a field to AVFormatContext specifying the preferred protocol
Yes, that would work too. And it would be simpler. But... Is it a good idea to add a new field to AVFormatContext every time that a format requires some new information, which is not used by other formats? Just trying to evaluate all the possible advantages and disadvantages... Or maybe we can use a new AVFormatContext's flag? (this would avoid changing AVFormatContext). Thanks, Luca
On Sat, Dec 29, 2007 at 12:35:57PM +0100, Luca Abeni wrote:
Hi Michael,
Michael Niedermayer wrote: [...]
Ok. Looking at the rest of the code, I suspect a good solution would be something like rtsp://localhost:5454?tcp/test1-rtsp.mpg... And we will have to overwrite s->filename for removing the "?..." options that are between port number and path.
or just add a field to AVFormatContext specifying the preferred protocol
Yes, that would work too. And it would be simpler. But... Is it a good idea to add a new field to AVFormatContext every time that a format requires some new information, which is not used by other formats? Just trying to evaluate all the possible advantages and disadvantages...
Is rt(s)p really the only thing that has a tcp/udp switch need? Couldnt mms benefit from it too? And maybe others ... And the wasted byte in AVFormatContext should be weighted against the space needed for the code to parse ?tcp/udp. That also isnt usefull for other (de)muxers ...
Or maybe we can use a new AVFormatContext's flag? (this would avoid changing AVFormatContext).
A flag is tricky for the current TCP/UDP/UDP_MULTICAST. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Opposition brings concord. Out of discord comes the fairest harmony. -- Heraclitus
Michael Niedermayer wrote: [...]
Ok. Looking at the rest of the code, I suspect a good solution would be something like rtsp://localhost:5454?tcp/test1-rtsp.mpg... And we will have to overwrite s->filename for removing the "?..." options that are between port number and path. or just add a field to AVFormatContext specifying the preferred protocol Yes, that would work too. And it would be simpler. But... Is it a good idea to add a new field to AVFormatContext every time that a format requires some new information, which is not used by other formats? Just trying to evaluate all the possible advantages and disadvantages...
Is rt(s)p really the only thing that has a tcp/udp switch need? Couldnt mms benefit from it too? And maybe others ... Double thinking about it, yes, this field can be useful to mms and maybe other protocols (I was under the impression that mms, mmsh, mmsu, and similar are considered as different protocols, but using an AVFormatContext field to select the correct protocol makes sense).
[...]
Or maybe we can use a new AVFormatContext's flag? (this would avoid changing AVFormatContext).
A flag is tricky for the current TCP/UDP/UDP_MULTICAST.
I was thinking about a flag for TCP vs UDP and a flag for unicast vs multicast (ok, that would create the problem with the impossible multicast-tcp combination). Anyway, in the next days I'll work on a patch for adding a flag to AVFormatContext and using it for selecting the transport protocol. Thanks, Luca
Hi all, Luca Abeni wrote: [...]
Anyway, in the next days I'll work on a patch for adding a flag to
sorry, this is a typo; I obviously wanted to write "a field"
AVFormatContext and using it for selecting the transport protocol.
I attach a first version of such patch. To make the patch independent from RTSP, I added an enum AVTransportProtocol. I also modified ffplay to properly set the new AVFormatContext field for selecting TCP when needed (since the transport protocol cannot be selected through an option in the URL, the "-rtp_tcp" option cannot be removed). As I said, I have no problems in applying Ronald's patch first, and re-generating my patch after that. And, BTW, happy new year to everyone!!! Luca
On Mon, Dec 31, 2007 at 02:57:52PM +0100, Luca Abeni wrote:
Hi all,
Luca Abeni wrote: [...]
Anyway, in the next days I'll work on a patch for adding a flag to
sorry, this is a typo; I obviously wanted to write "a field"
AVFormatContext and using it for selecting the transport protocol.
I attach a first version of such patch. To make the patch independent
from RTSP, I added an enum AVTransportProtocol. I also modified ffplay to properly set the new AVFormatContext field for selecting TCP when needed (since the transport protocol cannot be selected through an option in the URL, the "-rtp_tcp" option cannot be removed).
As I said, I have no problems in applying Ronald's patch first, and re-generating my patch after that.
And, BTW, happy new year to everyone!!! [...] -#ifdef CONFIG_RTSP_DEMUXER static void opt_rtp_tcp(void) { /* only tcp protocol */ - rtsp_default_protocols = (1 << RTSP_PROTOCOL_RTP_TCP); + rtsp_default_protocols = AVTRANSPORT_TCP; } -#endif
this would be unneeded if ffplay would use AVOptions, but thats seperate and belongs in some future patch ... if you want to volunteer to implement it :)
static void opt_sync(const char *arg) { Index: libavformat/avformat.h =================================================================== --- libavformat/avformat.h (revision 11361) +++ libavformat/avformat.h (working copy) @@ -371,6 +371,13 @@
#define MAX_STREAMS 20
+enum AVTransportProtocol { + AVTRANSPORT_UDP, /**< Use UDP as a transport protocol for networking formats */ + AVTRANSPORT_TCP, /**< Use UDP as a transport protocol for networking + formats */ + AVTRANSPORT_MULTICAST, /**< Use a multicast transport protocol for networking formats */ +}; + /** * format I/O context. * New fields can be added to the end with minor version bumps. @@ -476,6 +483,11 @@ * demuxing: set by user */ enum CodecID subtitle_codec_id; + /** + * Forced network protocol. + * demuxing: set by user + */ + enum AVTransportProtocol transport_protocol; } AVFormatContext;
you are forgetting to bump the minor version num and you have to add an entry to AVOption in utils.c [...]
+ switch (s->transport_protocol) { + case AVTRANSPORT_UDP: protocol_mask = (1<< RTSP_PROTOCOL_RTP_UDP); break; + case AVTRANSPORT_TCP: protocol_mask = (1<< RTSP_PROTOCOL_RTP_TCP); break; + case AVTRANSPORT_MULTICAST: protocol_mask = (1<< RTSP_PROTOCOL_RTP_UDP_MULTICAST); break; }
vertical align, and is this remapping needed? cant u use the AVTRANSPORT values in RTSP too? [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I count him braver who overcomes his desires than him who conquers his enemies for the hardest victory is over self. -- Aristotle
Hi Michael, first of all, thanks for the feedback. Michael Niedermayer wrote: [...]
[...]
-#ifdef CONFIG_RTSP_DEMUXER static void opt_rtp_tcp(void) { /* only tcp protocol */ - rtsp_default_protocols = (1 << RTSP_PROTOCOL_RTP_TCP); + rtsp_default_protocols = AVTRANSPORT_TCP; } -#endif
this would be unneeded if ffplay would use AVOptions, but thats seperate and belongs in some future patch
I did not know much about AVOptions... I am looking at them right now, and I'll update the patch to use AVOptions.
... if you want to volunteer to implement it :)
I see that the whole ffplay.c has to be converted, right? I'll have a look, and see if I can find some time for this conversion. [...]
enum CodecID subtitle_codec_id; + /** + * Forced network protocol. + * demuxing: set by user + */ + enum AVTransportProtocol transport_protocol; } AVFormatContext;
you are forgetting to bump the minor version num
Opss... I knew I was forgetting something... Sorry about that; I'll fix the patch and resend.
and you have to add an entry to AVOption in utils.c
Ok. I now understand this... I'm going to fix this problem.
[...]
+ switch (s->transport_protocol) { + case AVTRANSPORT_UDP: protocol_mask = (1<< RTSP_PROTOCOL_RTP_UDP); break; + case AVTRANSPORT_TCP: protocol_mask = (1<< RTSP_PROTOCOL_RTP_TCP); break; + case AVTRANSPORT_MULTICAST: protocol_mask = (1<< RTSP_PROTOCOL_RTP_UDP_MULTICAST); break; }
vertical align, and is this remapping needed? cant u use the AVTRANSPORT values in RTSP too?
I was under the impression that protocol_mask wants to be a bitmask, while transport_protocol is a simple constant... Anyway, I see that there are some patches changing this code; I'll redo my patch after they are applied, and if possible I'll directly use AVTRANSPORT_*, removing the RTSP_PROTOCOL_* constants. Thanks, Luca
On Wed, Jan 02, 2008 at 10:38:18AM +0100, Luca Abeni wrote:
Hi Michael,
first of all, thanks for the feedback.
Michael Niedermayer wrote: [...]
[...]
-#ifdef CONFIG_RTSP_DEMUXER static void opt_rtp_tcp(void) { /* only tcp protocol */ - rtsp_default_protocols = (1 << RTSP_PROTOCOL_RTP_TCP); + rtsp_default_protocols = AVTRANSPORT_TCP; } -#endif
this would be unneeded if ffplay would use AVOptions, but thats seperate and belongs in some future patch
I did not know much about AVOptions... I am looking at them right now, and I'll update the patch to use AVOptions.
... if you want to volunteer to implement it :)
I see that the whole ffplay.c has to be converted, right? I'll have a look, and see if I can find some time for this conversion.
theres no need to convert all at once, ffmpeg.c also still has tons of old style options whats nice if one would add avoption support for ffplay.c would be that many options would suddenly become available ... [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB I hate to see young programmers poisoned by the kind of thinking Ulrich Drepper puts forward since it is simply too narrow -- Roman Shaposhnik
Hi Michael, Michael Niedermayer wrote: [...]
heres a attempt to fix it (completely untested, theres a good chance it contains trivial mistakes but ill comit it if noone points at a problem soon *g*)
@@ -2941,7 +2941,7 @@ char *path, int path_size, const char *url) { - const char *p, *ls, *at, *col, *brk, *q; + const char *p, *ls, *at, *col, *brk;
I did some tests with various url_split() versions and with current svn + your patch, and it seems to me that your patch fixes almost all the testcases. I only found a case about which I am not sure: the URL rtp://localhost?localport=5000 gives ?localport=5000 as path. I suspect this is wrong (?localport=5000 should be the path for rtp://localhost/?localport=5000, right?). The attached patch tries to fix this case (assuming that it is a bug), and does not seem to break anything. But looking at url_split()'s history, I cannot claim that this patch does not introduce any regression... I hope my change is correct, but I'd like to hear other people's opinions too... I also attach an interdiff between your original patch and my updated one. Thanks, Luca
On Sat, Dec 29, 2007 at 12:19:12PM +0100, Luca Abeni wrote:
Hi Michael,
Michael Niedermayer wrote: [...]
heres a attempt to fix it (completely untested, theres a good chance it contains trivial mistakes but ill comit it if noone points at a problem soon *g*) @@ -2941,7 +2941,7 @@ char *path, int path_size, const char *url) { - const char *p, *ls, *at, *col, *brk, *q; + const char *p, *ls, *at, *col, *brk;
I did some tests with various url_split() versions and with current svn + your patch, and it seems to me that your patch fixes almost all the testcases. I only found a case about which I am not sure: the URL rtp://localhost?localport=5000 gives ?localport=5000 as path. I suspect this is wrong (?localport=5000 should be the path for rtp://localhost/?localport=5000, right?).
IIRC rich said '?' counts as host / path seperator if theres no '/' either way ive commited my patch. We can always commit your interdiff if we find a case where it causes problems. [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Let us carefully observe those good qualities wherein our enemies excel us and endeavor to excel them, by avoiding what is faulty, and imitating what is excellent in them. -- Plutarch
Hi Michael, Michael Niedermayer wrote: [...]
I did some tests with various url_split() versions and with current svn + your patch, and it seems to me that your patch fixes almost all the testcases. I only found a case about which I am not sure: the URL rtp://localhost?localport=5000 gives ?localport=5000 as path. I suspect this is wrong (?localport=5000 should be the path for rtp://localhost/?localport=5000, right?).
IIRC rich said '?' counts as host / path seperator if theres no '/'
Ah, ok... I was not aware of this convention
either way ive commited my patch. We can always commit your interdiff if we find a case where it causes problems.
Ok. I did some more tests, and I do not see any problems. So, I guess the code is ok as it is now. BTW, I seem to remember some discussions some time ago about people having problems with youtube urls... I do not know if such problems were resolved or not, but maybe your patch just fixed them :) Thanks, Luca
Hi, On Dec 28, 2007 5:12 PM, Luca Abeni <lucabe72 at email.it> wrote:
It seems that the situation is more messy than I expected: 1) url_split() has been rewritten in r10605 2) the rewrite introduced a regression, by changing the behaviour of url_split() 3) r10775 claimed to fix the regression by introducing yet another different behaviour in url_split(). And this is the change that broke option parsing in rtsp.c.
Some data points: 1) Before r10605, rtsp://localhost:5454/test1-rtsp.mpg?tcp resulted in path="/test1-rtsp.mpg?tcp" 2) After r10605, the same url produced path="/test1-rtsp.mpg?tcp", so no change for this test case... 3) r10775 "fixed" something so that the result is now path="/test1-rtsp.mpg?". 4) So, I tried a different url: rtsp://localhost?tcp. The results are path="?tcp" before r10605, path="" after r10605, and path="" after r10775. Again, that commit does not seem to fix any regression.
That one (w/o the / in the URL) is supposed to be fixed, I remember testing that (we were actively working on ipv6 issues), and receiving confirmation [1] that others saw it fixed too. Did I mess up that badly? Ronald [1] http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2007-October/036849.html
Hi Ronald, Ronald S. Bultje wrote: [...]
Some data points: 1) Before r10605, rtsp://localhost:5454/test1-rtsp.mpg?tcp resulted in path="/test1-rtsp.mpg?tcp" 2) After r10605, the same url produced path="/test1-rtsp.mpg?tcp", so no change for this test case... 3) r10775 "fixed" something so that the result is now path="/test1-rtsp.mpg?". 4) So, I tried a different url: rtsp://localhost?tcp. The results are path="?tcp" before r10605, path="" after r10605, and path="" after r10775. Again, that commit does not seem to fix any regression.
That one (w/o the / in the URL) is supposed to be fixed, I remember testing that (we were actively working on ipv6 issues), and receiving confirmation [1] that others saw it fixed too. Did I mess up that badly?
I investigated the problem a little bit more, and it seems that: the "?..." options must be removed from the host name, but not from the path (also see last message from Michael). r10775 fixed host parsing (which was causing the ffplay regression), but broke path parsing (by removing "?..." from it), and noone noticed that. Now, Michael's patch seems to fix almost everything (I only found a suspicious case that I am testing). Thanks, Luca
participants (8)
-
aurel＠gnuage.org -
benoit.fouet＠purplelabs.com -
diego＠biurrun.de -
lu_zero＠gentoo.org -
lucabe72＠email.it -
mans＠mansr.com -
michaelni＠gmx.at -
rsbultje＠gmail.com