[FFmpeg-devel] [PATCH] RTMP client support for lavf
Michael Niedermayer
michaelni
Mon Jul 20 17:05:41 CEST 2009
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:
> > > $subj
> [...]
> > > +/** RTMP default port */
> > > +#define RTMP_DEFAULT_PORT 1935
> >
> > very usefull comment
>
> of course, it increases number of bytes and lines committed by me
then please seperare it into a seperate patch and file, no need to
clutter this one
>
> > > +
> > > +/** RTMP handshake data size */
> > > +#define RTMP_HANDSHAKE_PACKET_SIZE 1536
> >
> > same
> >
> >
> > > +
> > > +#define RTMP_CLIENT_PLATFORM "LNX"
> >
> > LooNiX ?
>
> who cares?
someone who tries to understand the code possibly
>
> > [...]
> > > +/** protocol handler context */
> > > +typedef struct RTMPContext {
> >
> > > + URLContext* rtmp_hd; ///< context for TCP stream
> >
> > wouldnt a variable name that was related to "context for TCP stream" be
> > clearer?
>
> renamed along with doxy
>
> > > + RTMPPacket prev_pkt[2][RTMP_CHANNELS]; ///< packet history used when reading and sending packets
> >
> > > + int chunk_size; ///< chunk size
> >
> > every comment that duplicates the variable name clutters ones view with
> > useless information
> > every comment that duplicates the variable name clutters ones view with
> > useless information
>
> Tonight on 'It's the mind' we examine the phenomenon of deja vu...
>
> > see? you also dont like it if i repeat a comment twice, and similarly a
> > reader trying to understand the code almost certainly prefers not to find
> > in a comment just the variable name
>
> Personally I just ignore it but I see your point. Now comments should be
> more meaningful and useful
thanks
[...]
> > [...]
> > > +//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
>
> > [...]
> > > +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
> >
> > does it work to just send 0 ?
> > because you are always sending the same anyway ...
>
> it does, but as I understand it, it's also used to measure bandwidth so
> why not give server what it expects?
1536 zero bytes ?
[...]
> +/* needed for gethostname() */
> +#define _XOPEN_SOURCE 600
doxy
[...]
> +static void rtmp_calc_digest(const uint8_t *src, int len, int gap,
> + const uint8_t *key, int keylen, uint8_t *dst)
> +{
> + struct AVSHA *sha;
> + uint8_t hmac_buf[64+32];
> + int i;
> +
> + sha = av_mallocz(av_sha_size);
> +
> + memset(hmac_buf, 0, 64);
uint8_t hmac_buf[64+32]={0};
[...]
> +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;
> + 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;
> + }
why is the av_log commented out?
[...]
> +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);
chunk_size is signed, is it intended for this to be possibly negative?
> + //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);
> + 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
if{ }else
[...]
> Index: libavformat/flv.h
> ===================================================================
> --- libavformat/flv.h (revision 19461)
> +++ libavformat/flv.h (working copy)
> @@ -105,8 +105,10 @@
> AMF_DATA_TYPE_UNDEFINED = 0x06,
> AMF_DATA_TYPE_REFERENCE = 0x07,
> AMF_DATA_TYPE_MIXEDARRAY = 0x08,
> + AMF_DATA_TYPE_OBJECT_END = 0x09,
> AMF_DATA_TYPE_ARRAY = 0x0a,
> AMF_DATA_TYPE_DATE = 0x0b,
> + AMF_DATA_TYPE_LONG_STRING = 0x0c,
> AMF_DATA_TYPE_UNSUPPORTED = 0x0d,
> } AMFDataType;
the changes to flv.h are ok
>
> Index: libavformat/rtmppkt.c
> ===================================================================
> --- libavformat/rtmppkt.c (revision 0)
> +++ libavformat/rtmppkt.c (revision 0)
> @@ -0,0 +1,260 @@
> +/*
> + * RTMP input format
> + * Copyright (c) 2009 Kostya Shishkov
> + *
> + * This file is part of FFmpeg.
> + *
> + * FFmpeg is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * FFmpeg is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with FFmpeg; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> + */
> +
> +/* needed for gethostname() */
> +#define _XOPEN_SOURCE 600
> +
> +#include "libavcodec/bytestream.h"
> +#include "libavutil/avstring.h"
> +#include "avformat.h"
> +
> +#include "rtmppkt.h"
> +#include "flv.h"
> +
> +void ff_amf_write_bool(uint8_t **dst, int val)
> +{
> + bytestream_put_byte(dst, AMF_DATA_TYPE_BOOL);
> + bytestream_put_byte(dst, val);
> +}
> +
> +void ff_amf_write_number(uint8_t **dst, double val)
> +{
> + bytestream_put_byte(dst, AMF_DATA_TYPE_NUMBER);
> + bytestream_put_be64(dst, av_dbl2int(val));
> +}
> +
> +void ff_amf_write_string(uint8_t **dst, const char *str)
> +{
> + bytestream_put_byte(dst, AMF_DATA_TYPE_STRING);
> + bytestream_put_be16(dst, strlen(str));
> + bytestream_put_buffer(dst, str, strlen(str));
> +}
these are duplicates of the flv code ...
i see its possible not trivial to merge them due to API differences but
it at least should be clearly documented that they are duplicates so that
for example a bugfix done to one set is also applied to the 2nd
> +
> +void ff_amf_write_null(uint8_t **dst)
> +{
> + bytestream_put_byte(dst, AMF_DATA_TYPE_NULL);
> +}
> +
> +void ff_amf_write_object_start(uint8_t **dst)
> +{
> + bytestream_put_byte(dst, AMF_DATA_TYPE_OBJECT);
> +}
> +
> +void ff_amf_write_field_name(uint8_t **dst, const char *str)
> +{
> + bytestream_put_be16(dst, strlen(str));
> + bytestream_put_buffer(dst, str, strlen(str));
> +}
> +
> +void ff_amf_write_object_end(uint8_t **dst)
> +{
> + // first two bytes are field name length = 0, AMF object should end with it and end marker
> + bytestream_put_be24(dst, AMF_DATA_TYPE_OBJECT_END);
> +}
> +
> +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 -1;
> + }
> + channel_id = hdr & 0x3F;
> +
> + hdr >>= 6;
> + if (hdr == RTMP_PS_ONEBYTE) {
> + //todo
> + return -1;
> + } else {
> + if (url_read_complete(h, buf, 3) != 3) {
> + return -1;
> + }
> + timestamp = AV_RB24(buf);
> + if (hdr != RTMP_PS_FOURBYTES) {
> + if (url_read_complete(h, buf, 3) != 3) {
> + return -1;
> + }
> + data_size = AV_RB24(buf);
> + if (url_read_complete(h, &type, 1) != 1) {
> + return -1;
> + }
> + if (hdr == RTMP_PS_TWELVEBYTES) {
> + if (url_read(h, buf, 4) != 4) {
> + return -1;
> + }
> + extra = AV_RL32(buf);
> + } else {
> + extra = prev_pkt[channel_id].extra;
> + }
> + } else {
> + data_size = prev_pkt[channel_id].data_size;
> + type = prev_pkt[channel_id].type;
> + extra = prev_pkt[channel_id].extra;
> + }
> + }
> + ff_rtmp_packet_create(p, channel_id, type, timestamp, data_size);
> + p->extra = extra;
> + // save history
> + prev_pkt[channel_id].channel_id = type;
> + prev_pkt[channel_id].type = channel_id;
is that exchange intended?
[...]
> +int ff_rtmp_packet_create(RTMPPacket *pkt, int channel_id, RTMPPacketType type,
> + int timestamp, int size)
> +{
> + pkt->data = av_malloc(size);
> + if (!pkt->data)
> + return -1;
ENOMEM
[...]
> +/**
> + * Creates new RTMP packet with given attributes.
> + *
> + * @param pkt packet
> + * @param channel_id packet channel ID
> + * @param type packet type
> + * @param timestamp packet timestamp
> + * @param size packet size
> + * @return zero on success, -1 otherwise
in ffmpeg <0 is error, please follow this convention, it simplifies returning
error codes in the future (with above a review of any == -1 checks is needed)
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090720/b6b93d97/attachment.pgp>
More information about the ffmpeg-devel
mailing list