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

[Ronald S. Bultje]

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
>
> 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