[FFmpeg-devel] [PATCH] rtsp - alternate protocol

[Michael Niedermayer]

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:
> > >
> > > > 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?
[...]
> -        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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080119/0f4b25e3/attachment.pgp>