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

Ronald S. Bultje rsbultje
Sat Dec 29 18:41:09 CET 2007


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
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: alternate-protocol.patch
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20071229/abbbf9bb/attachment.txt>



More information about the ffmpeg-devel mailing list