[FFmpeg-devel] Realmedia patch

[Ronald S. Bultje]

Hi Luca,

On Thu, Aug 28, 2008 at 3:11 AM, Luca Abeni <lucabe72 at email.it> wrote:
> +                       /**
> +                        * 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...

I'll say they're "interrelated" in some way, rather than what it says
now. The values of the strings come from mplayer (but as said before,
implementation is completely different so I don't think license is an
issue).

>
> +                       "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;
>     }

I guess this is better, I see your point. I'll add it this way.

Full patch attached, I'll commit in 3 chunks (one for RTSPServerType,
one for RTSPReply:real_challenge and one for the actual options) if
approved.

Ronald
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: rtsp-options.patch
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080828/354be2e0/attachment.asc>