[v2,3/5] rtsp: Accept options via private avoptions instead of URL options

Message ID 1318504831-10325-3-git-send-email-martin@martin.st
State Committed
Headers show

Commit Message

Martin Storsjö Oct. 13, 2011, 11:20 a.m.
Eventually, the old way of passing options by adding
stuff to the URL can be dropped.

This avoids having to tamper with the user-specified URL to
pass options on the transport mode. This also works better
with redirects, since the options don't need to be parsed out
from the URL.
---
 libavformat/rtsp.c    |   28 ++++++++++++++++++++++++++--
 libavformat/rtsp.h    |   12 +++++++++++-
 libavformat/version.h |    5 ++++-
 3 files changed, 41 insertions(+), 4 deletions(-)

Comments

Luca Barbato Oct. 13, 2011, 11:49 a.m. | #1
On 10/13/11 1:20 PM, Martin Storsjö wrote:
> +    { "rtsp_transport", "RTSP transport protocols", OFFSET(lower_transport_mask), AV_OPT_TYPE_FLAGS, {0}, 0, INT_MAX, DEC|ENC, "rtsp_transport" }, \
> +    { "udp", "UDP", 0, AV_OPT_TYPE_CONST, {1<<  RTSP_LOWER_TRANSPORT_UDP}, 0, 0, DEC|ENC, "rtsp_transport" }, \
> +    { "tcp", "TCP", 0, AV_OPT_TYPE_CONST, {1<<  RTSP_LOWER_TRANSPORT_TCP}, 0, 0, DEC|ENC, "rtsp_transport" }, \
> +    { "udp_multicast", "UDP multicast", 0, AV_OPT_TYPE_CONST, {1<<  RTSP_LOWER_TRANSPORT_UDP_MULTICAST}, 0, 0, DEC, "rtsp_transport" },

I'd use the proto+proto:// syntax.
Martin Storsjö Oct. 13, 2011, 12:02 p.m. | #2
On Thu, 13 Oct 2011, Luca Barbato wrote:

> On 10/13/11 1:20 PM, Martin Storsjö wrote:
>> +    { "rtsp_transport", "RTSP transport protocols", 
>> OFFSET(lower_transport_mask), AV_OPT_TYPE_FLAGS, {0}, 0, INT_MAX, DEC|ENC, 
>> "rtsp_transport" }, \
>> +    { "udp", "UDP", 0, AV_OPT_TYPE_CONST, {1<<  RTSP_LOWER_TRANSPORT_UDP}, 
>> 0, 0, DEC|ENC, "rtsp_transport" }, \
>> +    { "tcp", "TCP", 0, AV_OPT_TYPE_CONST, {1<<  RTSP_LOWER_TRANSPORT_TCP}, 
>> 0, 0, DEC|ENC, "rtsp_transport" }, \
>> +    { "udp_multicast", "UDP multicast", 0, AV_OPT_TYPE_CONST, {1<< 
>> RTSP_LOWER_TRANSPORT_UDP_MULTICAST}, 0, 0, DEC, "rtsp_transport" },
>
> I'd use the proto+proto:// syntax.

That forces us to still keep on modifying the url and strip out that, 
since the normal plain rtsp:// url still needs to be included within the 
requests.

Also it's worse if you're e.g. building a player app, where the rtsp url 
is given as user input (or as input from urls received externally or 
whatever), where the app would have to edit the urls to include this if 
the app deems it necessary, compared to setting these options out of band 
from the actual url.

Moving the options from one place in the url to another isn't really 
progress IMO.

Additionally, that still has the issue of parsing options from URLs and 
redirects - if I give the input url rtsp+tcp://foo/bar, which redirects me 
to rtsp://other/server, I'll reparse that url, forgetting about the 
options passed in the previous one. Sure that can be fixed by even more 
kludges, but that's all what I'm trying to get rid of here.

// Martin
Luca Barbato Oct. 13, 2011, 1:25 p.m. | #3
On 10/13/11 2:02 PM, Martin Storsjö wrote:
> Additionally, that still has the issue of parsing options from URLs and
> redirects - if I give the input url rtsp+tcp://foo/bar, which redirects
> me to rtsp://other/server, I'll reparse that url, forgetting about the
> options passed in the previous one. Sure that can be fixed by even more
> kludges, but that's all what I'm trying to get rid of here.

You made a good point. I'd keep this syntax and use it to fill the 
options but might be just overkill.

lu
Martin Storsjö Oct. 13, 2011, 1:51 p.m. | #4
On Thu, 13 Oct 2011, Luca Barbato wrote:

> On 10/13/11 2:02 PM, Martin Storsjö wrote:
>> Additionally, that still has the issue of parsing options from URLs and
>> redirects - if I give the input url rtsp+tcp://foo/bar, which redirects
>> me to rtsp://other/server, I'll reparse that url, forgetting about the
>> options passed in the previous one. Sure that can be fixed by even more
>> kludges, but that's all what I'm trying to get rid of here.
>
> You made a good point. I'd keep this syntax and use it to fill the options 
> but might be just overkill.

Keep which one, the current, or the new with rtsp+tcp://? The current one 
is just crap IMO, but rtsp+tcp:// would be more convenient, I do agree 
about that. The downside would be that we'd have two different 
nondeprecated ways of doing one thing, which is confusing and 
inconsistent.

// Martin
Luca Barbato Oct. 13, 2011, 2:42 p.m. | #5
On 10/13/11 3:51 PM, Martin Storsjö wrote:
> On Thu, 13 Oct 2011, Luca Barbato wrote:
>
>> On 10/13/11 2:02 PM, Martin Storsjö wrote:
>>> Additionally, that still has the issue of parsing options from URLs and
>>> redirects - if I give the input url rtsp+tcp://foo/bar, which redirects
>>> me to rtsp://other/server, I'll reparse that url, forgetting about the
>>> options passed in the previous one. Sure that can be fixed by even more
>>> kludges, but that's all what I'm trying to get rid of here.
>>
>> You made a good point. I'd keep this syntax and use it to fill the
>> options but might be just overkill.
>
> Keep which one, the current, or the new with rtsp+tcp://?

The new one.

> The current
> one is just crap IMO, but rtsp+tcp:// would be more convenient, I do
> agree about that. The downside would be that we'd have two different
> nondeprecated ways of doing one thing, which is confusing and inconsistent.

only if you mix them up, but yes it might cause more harm than good.

lu
Martin Storsjö Oct. 17, 2011, 11:52 a.m. | #6
On Thu, 13 Oct 2011, Luca Barbato wrote:

>>> You made a good point. I'd keep this syntax and use it to fill the
>>> options but might be just overkill.
>> 
>> Keep which one, the current, or the new with rtsp+tcp://?
>
> The new one.
>
>> The current
>> one is just crap IMO, but rtsp+tcp:// would be more convenient, I do
>> agree about that. The downside would be that we'd have two different
>> nondeprecated ways of doing one thing, which is confusing and inconsistent.
>
> only if you mix them up, but yes it might cause more harm than good.

So, what's the consensus on how to move forward with this?


For new readers of this thread, there's three ways of passing these 
options, one old that we're trying to move away from, one new suggested 
scheme, and one purely avoption based one.


That is, currently, one chooses betwen different transport modes in RTSP 
by adding query parameters, e.g. like this:

avplay rtsp://server/path?tcp

These parameters are stripped out within the RTSP demuxer and only used 
for setting parameter internally there. This is obviously quite messy, 
especially if the original path would contain other query parameters (e.g. 
"avplay rtsp://server/path?param=value&tcp", where the user expects 
param=value to be sent to the server, but not the &tcp part). This is the 
status quo that we want to get rid of.


The second choice is purely avoptions based, like this:

avplay -rtsp_transport tcp rtsp://server/path

This completely avoids having to meddle with the actual RTSP url, and is 
easy to use for users of the library.


The third option as suggested by Luca, would be to move to something like 
this:

avplay rtsp+tcp://server/path

This is easier for the commandline avplay/avconv user, but more complex 
for users of the library (where the library caller would have to munge the 
url to add the +tcp part, and libavformat would have to remove it again 
when adding the plain rtsp://server/path url into the requests).


So, opinions please. AVOptions, protocol+protocol://, or both?

// Martin
Luca Barbato Oct. 17, 2011, 12:44 p.m. | #7
On 10/17/11 4:52 AM, Martin Storsjö wrote:
> So, what's the consensus on how to move forward with this?

You have a nice patchset sitting here that could probably go as is, in 
the future I'd like to provide the +proto option, to our tools at least, 
since it is simpler for the user.

lu
Martin Storsjö Oct. 17, 2011, 12:56 p.m. | #8
On Mon, 17 Oct 2011, Luca Barbato wrote:

> On 10/17/11 4:52 AM, Martin Storsjö wrote:
>> So, what's the consensus on how to move forward with this?
>
> You have a nice patchset sitting here that could probably go as is, in the 
> future I'd like to provide the +proto option, to our tools at least, since it 
> is simpler for the user.

Ok then.

The patchset is still missing ok on patch #3 and #5 iirc.

// Martin
Anton Khirnov Oct. 17, 2011, 4:39 p.m. | #9
On Thu, 13 Oct 2011 14:20:29 +0300, Martin Storsjö <martin@martin.st> wrote:
> Eventually, the old way of passing options by adding
> stuff to the URL can be dropped.
> 
> This avoids having to tamper with the user-specified URL to
> pass options on the transport mode. This also works better
> with redirects, since the options don't need to be parsed out
> from the URL.
> ---
>  libavformat/rtsp.c    |   28 ++++++++++++++++++++++++++--
>  libavformat/rtsp.h    |   12 +++++++++++-
>  libavformat/version.h |    5 ++++-
>  3 files changed, 41 insertions(+), 4 deletions(-)
> 
> diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
> index e4c4c98..23c7b24 100644
> --- a/libavformat/rtsp.c
> +++ b/libavformat/rtsp.c
> @@ -59,9 +59,17 @@
>  
>  #define OFFSET(x) offsetof(RTSPState, x)
>  #define DEC AV_OPT_FLAG_DECODING_PARAM
> +#define ENC AV_OPT_FLAG_ENCODING_PARAM
>  const AVOption ff_rtsp_options[] = {
>      { "initial_pause",  "Don't start playing the stream immediately", OFFSET(initial_pause), AV_OPT_TYPE_INT, {0}, 0, 1, DEC },
>      FF_RTP_FLAG_OPTS(RTSPState, rtp_muxer_flags),
> +    { "rtsp_transport", "RTSP transport protocols", OFFSET(lower_transport_mask), AV_OPT_TYPE_FLAGS, {0}, 0, INT_MAX, DEC|ENC, "rtsp_transport" }, \

flags should have min set to INT_MIN, so 'all' keyword works. same for rtsp_flags

Otherwise looks fine to me, you can push with the above fixed.
Martin Storsjö Oct. 17, 2011, 4:59 p.m. | #10
On Mon, 17 Oct 2011, Anton Khirnov wrote:

>
> On Thu, 13 Oct 2011 14:20:29 +0300, Martin Storsjö <martin@martin.st> wrote:
>> Eventually, the old way of passing options by adding
>> stuff to the URL can be dropped.
>>
>> This avoids having to tamper with the user-specified URL to
>> pass options on the transport mode. This also works better
>> with redirects, since the options don't need to be parsed out
>> from the URL.
>> ---
>>  libavformat/rtsp.c    |   28 ++++++++++++++++++++++++++--
>>  libavformat/rtsp.h    |   12 +++++++++++-
>>  libavformat/version.h |    5 ++++-
>>  3 files changed, 41 insertions(+), 4 deletions(-)
>>
>> diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
>> index e4c4c98..23c7b24 100644
>> --- a/libavformat/rtsp.c
>> +++ b/libavformat/rtsp.c
>> @@ -59,9 +59,17 @@
>>
>>  #define OFFSET(x) offsetof(RTSPState, x)
>>  #define DEC AV_OPT_FLAG_DECODING_PARAM
>> +#define ENC AV_OPT_FLAG_ENCODING_PARAM
>>  const AVOption ff_rtsp_options[] = {
>>      { "initial_pause",  "Don't start playing the stream immediately", OFFSET(initial_pause), AV_OPT_TYPE_INT, {0}, 0, 1, DEC },
>>      FF_RTP_FLAG_OPTS(RTSPState, rtp_muxer_flags),
>> +    { "rtsp_transport", "RTSP transport protocols", OFFSET(lower_transport_mask), AV_OPT_TYPE_FLAGS, {0}, 0, INT_MAX, DEC|ENC, "rtsp_transport" }, \
>
> flags should have min set to INT_MIN, so 'all' keyword works. same for rtsp_flags
>
> Otherwise looks fine to me, you can push with the above fixed.

Pushed with this changed, thanks for the reviews!

// Martin

Patch

diff --git a/libavformat/rtsp.c b/libavformat/rtsp.c
index e4c4c98..23c7b24 100644
--- a/libavformat/rtsp.c
+++ b/libavformat/rtsp.c
@@ -59,9 +59,17 @@ 
 
 #define OFFSET(x) offsetof(RTSPState, x)
 #define DEC AV_OPT_FLAG_DECODING_PARAM
+#define ENC AV_OPT_FLAG_ENCODING_PARAM
 const AVOption ff_rtsp_options[] = {
     { "initial_pause",  "Don't start playing the stream immediately", OFFSET(initial_pause), AV_OPT_TYPE_INT, {0}, 0, 1, DEC },
     FF_RTP_FLAG_OPTS(RTSPState, rtp_muxer_flags),
+    { "rtsp_transport", "RTSP transport protocols", OFFSET(lower_transport_mask), AV_OPT_TYPE_FLAGS, {0}, 0, INT_MAX, DEC|ENC, "rtsp_transport" }, \
+    { "udp", "UDP", 0, AV_OPT_TYPE_CONST, {1 << RTSP_LOWER_TRANSPORT_UDP}, 0, 0, DEC|ENC, "rtsp_transport" }, \
+    { "tcp", "TCP", 0, AV_OPT_TYPE_CONST, {1 << RTSP_LOWER_TRANSPORT_TCP}, 0, 0, DEC|ENC, "rtsp_transport" }, \
+    { "udp_multicast", "UDP multicast", 0, AV_OPT_TYPE_CONST, {1 << RTSP_LOWER_TRANSPORT_UDP_MULTICAST}, 0, 0, DEC, "rtsp_transport" },
+    { "http", "HTTP tunneling", 0, AV_OPT_TYPE_CONST, {(1 << RTSP_LOWER_TRANSPORT_HTTP)}, 0, 0, DEC, "rtsp_transport" },
+    { "rtsp_flags", "RTSP flags", OFFSET(rtsp_flags), AV_OPT_TYPE_FLAGS, {0}, 0, INT_MAX, DEC, "rtsp_flags" },
+    { "filter_src", "Only receive packets from the negotiated peer IP", 0, AV_OPT_TYPE_CONST, {RTSP_FLAG_FILTER_SRC}, 0, 0, DEC, "rtsp_flags" },
     { NULL },
 };
 
@@ -1310,15 +1318,23 @@  int ff_rtsp_connect(AVFormatContext *s)
     char *option_list, *option, *filename;
     int port, err, tcp_fd;
     RTSPMessageHeader reply1 = {0}, *reply = &reply1;
-    int lower_transport_mask = 0;
+    int lower_transport_mask = rt->lower_transport_mask;
     char real_challenge[64] = "";
     struct sockaddr_storage peer;
     socklen_t peer_len = sizeof(peer);
 
     if (!ff_network_init())
         return AVERROR(EIO);
-redirect:
+
     rt->control_transport = RTSP_MODE_PLAIN;
+    if (lower_transport_mask & (1 << RTSP_LOWER_TRANSPORT_HTTP)) {
+        lower_transport_mask  = 1 << RTSP_LOWER_TRANSPORT_TCP;
+        rt->control_transport = RTSP_MODE_TUNNEL;
+    }
+    if (rt->rtsp_flags & RTSP_FLAG_FILTER_SRC)
+        rt->filter_source = 1;
+
+redirect:
     /* extract hostname and port */
     av_url_split(NULL, 0, auth, sizeof(auth),
                  host, sizeof(host), &port, path, sizeof(path), s->filename);
@@ -1328,6 +1344,7 @@  redirect:
     if (port < 0)
         port = RTSP_DEFAULT_PORT;
 
+#if FF_API_RTSP_URL_OPTIONS
     /* search for options */
     option_list = strrchr(path, '?');
     if (option_list) {
@@ -1335,6 +1352,7 @@  redirect:
          * the options back into the same string. */
         filename = option_list;
         while (option_list) {
+            int handled = 1;
             /* move the option pointer */
             option = ++option_list;
             option_list = strchr(option_list, '&');
@@ -1360,10 +1378,16 @@  redirect:
                 memmove(++filename, option, len);
                 filename += len;
                 if (option_list) *filename = '&';
+                handled = 0;
             }
+            if (handled)
+                av_log(s, AV_LOG_WARNING, "Options passed via URL are "
+                                          "deprecated, use -rtsp_transport "
+                                          "and -rtsp_flags instead.\n");
         }
         *filename = 0;
     }
+#endif
 
     if (!lower_transport_mask)
         lower_transport_mask = (1 << RTSP_LOWER_TRANSPORT_NB) - 1;
diff --git a/libavformat/rtsp.h b/libavformat/rtsp.h
index 5327b00..ba87c53 100644
--- a/libavformat/rtsp.h
+++ b/libavformat/rtsp.h
@@ -38,7 +38,10 @@  enum RTSPLowerTransport {
     RTSP_LOWER_TRANSPORT_UDP = 0,           /**< UDP/unicast */
     RTSP_LOWER_TRANSPORT_TCP = 1,           /**< TCP; interleaved in RTSP */
     RTSP_LOWER_TRANSPORT_UDP_MULTICAST = 2, /**< UDP/multicast */
-    RTSP_LOWER_TRANSPORT_NB
+    RTSP_LOWER_TRANSPORT_NB,
+    RTSP_LOWER_TRANSPORT_HTTP = 8,          /**< HTTP tunneled - not a proper
+                                                 transport mode as such,
+                                                 only for use via AVOptions */
 };
 
 /**
@@ -350,8 +353,15 @@  typedef struct RTSPState {
 
     /** Whether the server accepts the x-Dynamic-Rate header */
     int accept_dynamic_rate;
+
+    /**
+     * Various option flags for the RTSP muxer/demuxer.
+     */
+    int rtsp_flags;
 } RTSPState;
 
+#define RTSP_FLAG_FILTER_SRC  0x1
+
 /**
  * Describes a single stream, as identified by a single m= line block in the
  * SDP content. In the case of RDT, one RTSPStream can represent multiple
diff --git a/libavformat/version.h b/libavformat/version.h
index 82a07db..fb8eeff 100644
--- a/libavformat/version.h
+++ b/libavformat/version.h
@@ -25,7 +25,7 @@ 
 
 #define LIBAVFORMAT_VERSION_MAJOR 53
 #define LIBAVFORMAT_VERSION_MINOR  8
-#define LIBAVFORMAT_VERSION_MICRO  0
+#define LIBAVFORMAT_VERSION_MICRO  1
 
 #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \
                                                LIBAVFORMAT_VERSION_MINOR, \
@@ -86,5 +86,8 @@ 
 #ifndef FF_API_TIMESTAMP
 #define FF_API_TIMESTAMP               (LIBAVFORMAT_VERSION_MAJOR < 54)
 #endif
+#ifndef FF_API_RTSP_URL_OPTIONS
+#define FF_API_RTSP_URL_OPTIONS        (LIBAVFORMAT_VERSION_MAJOR < 54)
+#endif
 
 #endif /* AVFORMAT_VERSION_H */