[FFmpeg-devel] Realmedia patch

[Luca Abeni]

Hi Ronald,

Ronald S. Bultje wrote:
[...]
> New patch for actual change attached.

This looks ok, except 3 details:

> Index: ffmpeg-svn/libavformat/rtsp.c
> ===================================================================
> --- ffmpeg-svn.orig/libavformat/rtsp.c	2008-09-14 11:09:54.000000000 -0400
> +++ ffmpeg-svn/libavformat/rtsp.c	2008-09-14 11:24:26.000000000 -0400
> @@ -49,6 +49,12 @@
>      RTSP_SERVER_LAST
>  };
>  
> +enum RTSPTransport {
> +    RTSP_TRANSPORT_RTP, /*< Standard-compliant RTP packet */
> +    RTSP_TRANSPORT_RDT, /*< Real Data Packet */
> +    RTSP_TRANSPORT_LAST
> +};

The comments here are confusing, talking about "RTP packet" and
"Real Data Packet" when they are related to the protocol...
The correct comments would be "RTP protocol" and "RDT protocol",
but such comments would be useless. I do not know what's the policy
here (a "RTP protocol" comment for RTSP_TRANSPORT_RTP looks completely
redundant...), someone else should comment on this (otherwise, I'd say
to just remove the comment).

[...]
>              s->duration= (end==AV_NOPTS_VALUE)?AV_NOPTS_VALUE:end-start; // AV_NOPTS_VALUE means live broadcast (and can't seek)
> +        } else if (av_strstart(p, "IsRealDataType:integer;",&p)) {
> +            if (atoi(p) == 1)

I think here you need to check if p is a valid pointer before using atoi()?
(what happens with a broken SDP ending immediately after "integer;"?)
I suspect a get_word() (or get_word_sep()) can be needed.

 > @@ -1323,6 +1338,7 @@
 >          for (i = 0; i < rt->nb_rtsp_streams; i++) {
 >              if (i != 0) av_strlcat(cmd, ",", sizeof(cmd));
 >              ff_rdt_subscribe_rule(cmd, sizeof(cmd), i, 0);
 > +            if (rt->transport == RTSP_TRANSPORT_RDT)
 >              ff_rdt_subscribe_rule2(
 >                  rt->rtsp_streams[i]->rtp_ctx,
 >                  cmd, sizeof(cmd), i, 0);

Why is only ff_rdt_subscribe_rule2() under the "RTSP_TRANSPORT_RDT" case?
Isn't ff_rdt_subscribe_rule() RDT specific too? (otherwise, "ff_rdt_" is
misleading).

Once these details are fixed (or justified), I think the patch can be
committed.


				Luca