[FFmpeg-devel] [PATCH] RTMP client support for lavf
Kostya
kostya.shishkov
Wed Jul 22 06:58:05 CEST 2009
On Tue, Jul 21, 2009 at 11:30:26PM +0200, Michael Niedermayer wrote:
> On Tue, Jul 21, 2009 at 11:04:09AM +0300, Kostya wrote:
> > On Mon, Jul 20, 2009 at 05:05:41PM +0200, Michael Niedermayer wrote:
> > > On Sat, Jul 18, 2009 at 08:01:17PM +0300, Kostya wrote:
> > > > On Sat, Jul 18, 2009 at 11:29:34AM +0200, Michael Niedermayer wrote:
> > > > > On Fri, Jul 17, 2009 at 06:38:46PM +0300, Kostya wrote:
> [...]
> > [...]
> > > > > > +//TODO: Move HMAC code somewhere. Eventually.
> > > > >
> > > > > good idea!
> > > > > also it should be a seperate patch
> > > >
> > > > err, I've not found a place in libavcrypto to add it; also I suspect you
> > > > want generic version of HMAC performing on, say, MD5 as well.
> > >
> > > as you mention it, yes i surely would want that
> >
> > Please wait until I design API for it. Meanwhile, I'll leave this here
> > (since in most cases I need digest of two blocks and such calculation
> > saves me some memcpy()s).
> >
> [...]
> > > [...]
> > >
> > > > +/* needed for gethostname() */
> > > > +#define _XOPEN_SOURCE 600
> > >
> > > doxy
> >
> > no, it's not
>
> can you change that?
> any reason why not?
This is #define used to enable gethostname() in some included file and
it's not related to actual code. Hmm, I think I can drop it now
completely.
> [...]
>
>
> > +static void gen_connect(URLContext *s, RTMPContext *rt, const char *proto,
> > + const char *host, int port, const char *app)
> > +{
> > + RTMPPacket pkt;
> > + uint8_t ver[32], *p;
> > + char tcurl[512];
> > +
> > + ff_rtmp_packet_create(&pkt, RTMP_VIDEO_CHANNEL, RTMP_PT_INVOKE, 0, 4096);
> > + p = pkt.data;
> > +
> > + snprintf(tcurl, sizeof(tcurl), "%s://%s:%d/%s", proto, host, port, app);
> > + ff_amf_write_string(&p, "connect");
> > + ff_amf_write_number(&p, 1.0);
> > + ff_amf_write_object_start(&p);
> > + ff_amf_write_field_name(&p, "app");
> > + ff_amf_write_string(&p, app);
> > +
> > + snprintf(ver, sizeof(ver), "%s %d,%d,%d,%d", RTMP_CLIENT_PLATFORM, RTMP_CLIENT_VER1,
> > + RTMP_CLIENT_VER2, RTMP_CLIENT_VER3, RTMP_CLIENT_VER4);
> > + ff_amf_write_field_name(&p, "flashVer");
> > + ff_amf_write_string(&p, ver);
> > + ff_amf_write_field_name(&p, "tcUrl");
> > + ff_amf_write_string(&p, tcurl);
> > + ff_amf_write_field_name(&p, "fpad");
> > + ff_amf_write_bool(&p, 0);
> > + ff_amf_write_field_name(&p, "capabilities");
> > + ff_amf_write_number(&p, 15.0);
> > + ff_amf_write_field_name(&p, "audioCodecs");
> > + ff_amf_write_number(&p, 1639.0);
> > + ff_amf_write_field_name(&p, "videoCodecs");
> > + ff_amf_write_number(&p, 252.0);
> > + ff_amf_write_field_name(&p, "videoFunction");
> > + ff_amf_write_number(&p, 1.0);
> > + ff_amf_write_object_end(&p);
> > +
> > + pkt.data_size = p - pkt.data;
> > +
> > + ff_rtmp_packet_write(rt->stream, &pkt, rt->chunk_size, rt->prev_pkt[1]);
> > +}
> > +
> > +static void gen_create_stream(URLContext *s, RTMPContext *rt)
>
> I think you should document at the top of the file what the gen_ prefix
> means, i think ive guessed it correctly but knowing it makes the code
> more understandable ...
I will
> [...]
> > +static int rtmp_handshake(URLContext *s, RTMPContext *rt)
> > +{
> > + AVLFG rnd;
> > + uint8_t tosend [RTMP_HANDSHAKE_PACKET_SIZE+1];
> > + uint8_t clientdata[RTMP_HANDSHAKE_PACKET_SIZE];
> > + uint8_t serverdata[RTMP_HANDSHAKE_PACKET_SIZE+1];
> > + int i;
> > + int server_pos, client_pos;
>
> > + uint8_t digest[32];
> > +
> > + //av_log(s, AV_LOG_DEBUG, "Handshaking...\n");
> > +
> > + av_lfg_init(&rnd, 0xDEADC0DE);
> > + // generate handshake packet - 1536 bytes of pseudorandom data
> > + tosend[0] = 3; //unencrypted data
> > + memset(tosend+1, 0, 4);
> > + //write client "version"
> > + tosend[5] = RTMP_CLIENT_VER1;
> > + tosend[6] = RTMP_CLIENT_VER2;
> > + tosend[7] = RTMP_CLIENT_VER3;
> > + tosend[8] = RTMP_CLIENT_VER4;
>
> uint8_t digest[32]={ ...}
Ahem, no need to initialize digest[] since hash function will write to
it anyway. And if you meant tosend[], I think current initialization
for it is clearer.
> > + for (i = 9; i <= RTMP_HANDSHAKE_PACKET_SIZE; i++)
> > + tosend[i] = av_lfg_get(&rnd) >> 24;
> > + client_pos = rtmp_handshake_imprint_with_digest(tosend + 1);
> > +
> > + url_write(rt->stream, tosend, RTMP_HANDSHAKE_PACKET_SIZE + 1);
> > + i = url_read_complete(rt->stream, serverdata, RTMP_HANDSHAKE_PACKET_SIZE + 1);
> > + if (i != RTMP_HANDSHAKE_PACKET_SIZE + 1) {
> > + //av_log(s, AV_LOG_ERROR, "Cannot read RTMP handshake response\n");
> > + return -1;
> > + }
> > + i = url_read_complete(rt->stream, clientdata, RTMP_HANDSHAKE_PACKET_SIZE);
> > + if (i != RTMP_HANDSHAKE_PACKET_SIZE) {
> > + //av_log(s, AV_LOG_ERROR, "Cannot read RTMP handshake response\n");
> > + return -1;
> > + }
> > +
> > + //av_log(s, AV_LOG_DEBUG, "Server version %d.%d.%d.%d\n",
> > + // serverdata[5], serverdata[6], serverdata[7], serverdata[8]);
> > +
> > + server_pos = rtmp_validate_digest(serverdata + 1, 772);
> > + if (!server_pos) {
> > + server_pos = rtmp_validate_digest(serverdata + 1, 8);
> > + if (!server_pos) {
> > + //av_log(s, AV_LOG_ERROR, "Server response validating failed\n");
> > + return -1;
> > + }
>
> Is there a reason why the code fails if the validation fails?
Yes - that means server does not use the same handshake protocol and it
will expect client to perform handshake again and again until both sides
agree on validity.
> [...]
> > +static int rtmp_parse_result(URLContext *s, RTMPContext *rt, RTMPPacket *pkt)
> > +{
> > + int i, t;
> > +
> > + switch (pkt->type) {
> > + case RTMP_PT_CHUNK_SIZE:
> > + if (pkt->data_size != 4) {
> > + //av_log(s, AV_LOG_ERROR, "Chunk size change packet is not 4 (%d)\n",
> > + // pkt->data_size);
> > + return -1;
> > + }
> > + rt->chunk_size = AV_RB32(pkt->data);
> > + if (rt->chunk_size <= 0) {
> > + //av_log(s, AV_LOG_ERROR, "Incorrect chunk size %d\n", rt->chunk_size);
> > + return -1;
> > + }
>
> this leaves an invalid chunk_size in the context, is it guranteed that its
> not used?
Yes, it will return EIO in packet_read().
> > + //av_log(s, AV_LOG_DEBUG, "New chunk size = %d\n", rt->chunk_size);
> > + break;
> > + case RTMP_PT_PING:
> > + t = AV_RB16(pkt->data);
> > + if (t == 6)
> > + gen_pong(s, rt, pkt);
> > + break;
> > + case RTMP_PT_INVOKE:
> > + if (!memcmp(pkt->data, "\002\000\006_error", 9)) {
> > + uint8_t tmpstr[256];
> > +
> > + if (!ff_amf_find_field(pkt->data + 9, "description", tmpstr, sizeof(tmpstr)))
>
> > + av_log(NULL/*s*/, AV_LOG_ERROR, "Server error: %s\n",tmpstr);
>
> it could be a CONTEXT
> that is #defined appropriately depending on major ver
hmm... interesting idea
> > + return -1;
> > + }
> > + if (!memcmp(pkt->data, "\002\000\007_result", 10)) {
> > + switch (rt->state) {
> > + case STATE_HANDSHAKED:
> > + gen_create_stream(s, rt);
> > + rt->state = STATE_CONNECTING;
> > + break;
> > + case STATE_CONNECTING:
> > + //extract a number from result
> > + if (pkt->data[10] || pkt->data[19] != 5 || pkt->data[20]) {
> > + av_log(NULL, AV_LOG_WARNING, "Unexpected reply on connect()\n");
> > + } else {
> > + rt->main_channel_id = (int) av_int2dbl(AV_RB64(pkt->data + 21));
> > + }
> > + gen_play(s, rt);
> > + rt->state = STATE_READY;
> > + break;
> > + }
> > + }
> > + if (!memcmp(pkt->data, "\002\000\010onStatus", 11)) {
> > + const uint8_t* ptr = pkt->data + 11;
> > + uint8_t tmpstr[256];
> > + int t;
> > +
>
> > + for (i = 0; i < 2; i++) {
> > + t = ff_amf_skip_data(ptr);
>
> segfault
will add size check
> > + if (t < 0)
> > + return 1;
> > + ptr += t;
> > + }
>
> > + t = ff_amf_find_field(ptr, "level", tmpstr, sizeof(tmpstr));
>
> segfault
>
>
> > + if (!t && !strcmp(tmpstr, "error")) {
> > + if (!ff_amf_find_field(ptr, "description", tmpstr, sizeof(tmpstr)))
> > + av_log(NULL/*s*/, AV_LOG_ERROR, "Server error: %s\n",tmpstr);
> > + return -1;
> > + }
> > + t = ff_amf_find_field(ptr, "code", tmpstr, sizeof(tmpstr));
> > + if (!t && !strcmp(tmpstr, "NetStream.Play.Start")) {
> > + rt->state = STATE_PLAYING;
> > + return 0;
> > + }
> > + }
> > + break;
> > + }
> > + return 0;
> > +}
> > +
>
> > +static int get_packet(URLContext *s, int for_header)
> > +{
> > + RTMPContext *rt = s->priv_data;
> > + struct timespec ts;
> > + int ret;
> > +
> > + ts.tv_sec = 0;
> > + ts.tv_nsec = 500000000;
> > +
> > + for(;;) {
> > + RTMPPacket rpkt;
> > + int has_data = 0;
> > + if ((ret = ff_rtmp_packet_read(rt->stream, &rpkt,
> > + rt->chunk_size, rt->prev_pkt[0])) != 0) {
> > + if (ret > 0) {
> > + nanosleep(&ts, NULL);
>
> ehm, shouldnt that support returning EAGAIN ...
will change
> [...]
> > +/**
> > + * URL syntax: rtmp://host[:port]/path
> > + */
>
> The text is good but its a little terse, missing what the function does ...
indeed
> [...]
> > +URLProtocol rtmp_protocol = {
> > + "rtmp",
> > + rtmp_open,
> > + rtmp_read,
> > + rtmp_write,
> > + NULL, /* seek */
>
> speaking of seeking, doesnt rtmp support seeking of some kind?
It does, the question is how to invoke it since it requires sending a
command to server "seek to Nth second" and FLV demuxer does not know
about that.
Can we introduce protocol-aware seeking, i.e. seek() call from demuxer
will try to call such seek and if it fails - generic one.
> [...]
> > +int ff_rtmp_packet_read(URLContext *h, RTMPPacket *p,
> > + int chunk_size, RTMPPacket *prev_pkt)
> > +{
> > + uint8_t hdr, t, buf[16];
> > + int channel_id, timestamp, data_size, offset = 0, extra = 0;
> > + uint8_t type;
> > +
> > + if (url_read(h, &hdr, 1) != 1) {
> > + return AVERROR(EIO);
> > + }
> > + channel_id = hdr & 0x3F;
> > +
> > + hdr >>= 6;
> > + if (hdr == RTMP_PS_ONEBYTE) {
> > + //todo
> > + return -1;
> > + } else {
> > + if (url_read_complete(h, buf, 3) != 3) {
> > + return AVERROR(EIO);
> > + }
> > + timestamp = AV_RB24(buf);
> > + if (hdr != RTMP_PS_FOURBYTES) {
> > + if (url_read_complete(h, buf, 3) != 3) {
> > + return AVERROR(EIO);
> > + }
> > + data_size = AV_RB24(buf);
> > + if (url_read_complete(h, &type, 1) != 1) {
> > + return AVERROR(EIO);
> > + }
> > + if (hdr == RTMP_PS_TWELVEBYTES) {
> > + if (url_read_complete(h, buf, 4) != 4) {
> > + return AVERROR(EIO);
> > + }
> > + extra = AV_RL32(buf);
>
> extra is a signed int, its value depends on how many bits int has this
> cant be correct
will change
patch will be sent later
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
More information about the ffmpeg-devel
mailing list