[FFmpeg-devel] Realmedia patch

[Luca Abeni]

Hi Ronald,

Ronald S. Bultje wrote:
[...]
>> I think the best way to go is to
>> - first fix rtsp.c to make it more generic and to remove the
>>    "RTSP ===> RTP" assumption
>> - after that, introduce RDT support with a second patch.
>>
>> And I believe the most difficult part is doing the first step
>> in a clean way.
> 
> I'd probably just go and fix it as we go and integrate Realmedia
> support...

Well, I would have preferred the other way around, but I give up...

[...]
> Attached is a patch that introduces an enum ServerType and scans over
> it during OPTIONS while we detect server type. I'll use that in other
> patches in the future.

Index: ffmpeg-svn/libavformat/rtsp.c
===================================================================
--- ffmpeg-svn.orig/libavformat/rtsp.c	2008-08-27 07:38:19.000000000 -0400
+++ ffmpeg-svn/libavformat/rtsp.c	2008-08-27 16:53:47.000000000 -0400
@@ -42,6 +42,12 @@
      RTSP_STATE_PAUSED,
  };

+enum RTSPServerType {
+    RTSP_SERVER_RTP = 0, /*< Standard-compliant RTP-server */
+    RTSP_SERVER_RDT,     /*< Realmedia-style server */
+    RTSP_SERVER_LAST
+};
+
  typedef struct RTSPState {
      URLContext *rtsp_hd; /* RTSP TCP connexion handle */
      int nb_rtsp_streams;
@@ -55,6 +61,7 @@
      int seq;        /* RTSP command sequence number */
      char session_id[512];
      enum RTSPProtocol protocol;
+    enum RTSPServerType server_type;
      char last_reply[2048]; /* XXX: allocate ? */
      RTPDemuxContext *cur_rtp;
  } RTSPState;

Why the "= 0" for RTSP_SERVER_RTP?
Apart from this, I think this hunk is ok. If other people
do not complain in 2 or 3 days, commit it.


@@ -710,6 +717,9 @@
          reply->seq = strtol(p, NULL, 10);
      } else if (av_stristart(p, "Range:", &p)) {
          rtsp_parse_range_npt(p, &reply->range_start, &reply->range_end);
+    } else if (av_stristart(p, "RealChallenge1:", &p)) {
+        skip_spaces(&p);
+        av_strlcpy(reply->real_challenge, p, sizeof(reply->real_challenge));
      }
  }

Same as above, but please commit it separately (of course, in this commit
you also add the last hunk of this patch - the "real_challenge" field in
RTSPHeader).


@@ -1073,6 +1084,39 @@
      rt->rtsp_hd = rtsp_hd;
      rt->seq = 0;

+    /* request options supported by the server; this also detects server type */
+    for (rt->server_type = 0; rt->server_type < RTSP_SERVER_LAST;
+         rt->server_type++) {
+        snprintf(cmd, sizeof(cmd),
+                 "OPTIONS %s RTSP/1.0\r\n", s->filename);
+        if (rt->server_type == RTSP_SERVER_RDT)
+            av_strlcat(cmd,
+                       /**
+                        * The following entries are required for proper
+                        * streaming from a Realmedia server.
+                        * @param CompanyID is a 16-byte ID in base64
+                        * @param ClientChallenge is a hash of the other
+                        *        three params
+                        */

Can you add a comment saying where the values we are using come from?
Also, are we really sure about the above description? I received a
private email saying that it is CompanyID which is computed based on
ClientChallenge (which is probably a random number?), PlayerStarttime,
and GUID. I do not know if this is true or not...

+                       "ClientChallenge: 9e26d33f2984236010ef6253fb1887f7\r\n"
+                       "PlayerStarttime: [28/03/2003:22:50:23 00:00]\r\n"
+                       "CompanyID: KnKV4M4I/B2FjJ1TToLycw==\r\n"
+                       "GUID: 00000000-0000-0000-0000-000000000000\r\n",
+                       sizeof(cmd));
+        rtsp_send_cmd(s, cmd, reply, NULL);
+        if (reply->status_code != RTSP_STATUS_OK) {
+            err = AVERROR_INVALIDDATA;
+            goto fail;
+        } else if (rt->server_type != RTSP_SERVER_RDT &&
+                   reply->real_challenge[0]) {

The "rt->server_type != RTSP_SERVER_RDT" looks fragile (consider
the case when we will have more RTSP server types).
But if you change the "for()" loop structure (see next comment),
then this condition can be ok (but you have to add
"rt->server_type = RTSP_SERVER_RDT" before continue).

+            continue;
+        } else {
+            if (rt->server_type == RTSP_SERVER_RDT)
+                strcpy(real_challenge, reply->real_challenge);
+            break;
+        }
+    }

I do not like how the "for()" loop above is structured.
How do we exit from the loop in the "standard RTSP" case?
I suspect we would have "rt->server_type == RTSP_SERVER_RDT"?
Also, I think that the "rt->server_type++" in the for statement
is not good, and rt->server_type should be explicitly set to a new
value when some condition happens. For example, you can have
something like the following (untested):

     for (rt->server_type = RTSP_SERVER_RTP;;) {
         snprintf(cmd, sizeof(cmd),
                  "OPTIONS %s RTSP/1.0\r\n", s->filename);
         if (rt->server_type == RTSP_SERVER_RDT)
             av_strlcat(cmd,"ClientChallenge: 9e26d33f2984236010ef6253fb1887f7\r\n"
                            "PlayerStarttime: [28/03/2003:22:50:23 00:00]\r\n"
                            "CompanyID: KnKV4M4I/B2FjJ1TToLycw==\r\n"
                            "GUID: 00000000-0000-0000-0000-000000000000\r\n", sizeof(cmd));
         rtsp_send_cmd(s, cmd, reply, NULL);
         if (reply->status_code != RTSP_STATUS_OK) {
             err = AVERROR_INVALIDDATA;
             goto fail;
         }

         if ((rt->server_type != RTSP_SERVER_RDT) && reply->real_challenge[0]) {
             rt->server_type = RTSP_SERVER_RDT;
             continue;
         }

         if (rt->server_type == RTSP_SERVER_RDT)
            strcpy(real_challenge, reply->real_challenge);
         }

         break;
     }



				Luca