[FFmpeg-devel] [PATCH] RTMP client support for lavf
Kostya
kostya.shishkov
Sat Jul 18 19:01:17 CEST 2009
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
> > +
> > +/** RTMP handshake data size */
> > +#define RTMP_HANDSHAKE_PACKET_SIZE 1536
>
> same
>
>
> > +
> > +#define RTMP_CLIENT_PLATFORM "LNX"
>
> LooNiX ?
who cares?
> [...]
> > +/** 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
> > + char playpath[256]; ///< RTMP playpath
> > + ClientState state; ///< current state
> > + int main_channel_id; ///< an additional channel id which is used for some invokes
> > + uint8_t* flv_data; ///< buffer with data for demuxer
> > + int flv_size; ///< current buffer size
> > + int flv_off; ///< number of bytes read from current buffer
>
> > + uint32_t video_ts; ///< current video timestamp
> > + uint32_t audio_ts; ///< current audio timestamp
>
> when it comes to timestamps my first thought tends to be, in what timebase
> are they? that should be in the comment IMHO
added
> > +} RTMPContext;
> > +
> > +#define PLAYER_KEY_OPEN_PART_LEN 30 ///< length of partial key used for first client digest signing
> > +/** Client key used for digest signing */
> > +static const uint8_t rtmp_player_key[] =
> > +{
> > + 0x47, 0x65, 0x6E, 0x75, 0x69, 0x6E, 0x65, 0x20, 0x41, 0x64, 0x6F, 0x62, 0x65,
> > + 0x20, 0x46, 0x6C, 0x61, 0x73, 0x68, 0x20, 0x50, 0x6C, 0x61, 0x79, 0x65, 0x72,
> > + 0x20, 0x30, 0x30, 0x31, 0xF0, 0xEE, 0xC2, 0x4A, 0x80, 0x68, 0xBE, 0xE8, 0x2E,
> > + 0x00, 0xD0, 0xD1, 0x02, 0x9E, 0x7E, 0x57, 0x6E, 0xEC, 0x5D, 0x2D, 0x29, 0x80,
> > + 0x6F, 0xAB, 0x93, 0xB8, 0xE6, 0x36, 0xCF, 0xEB, 0x31, 0xAE
> > +};
> > +
>
> > +#define SERVER_KEY_OPEN_PART_LEN 36 ///< length of partial key used for first server digest signing
> > +/** Key used for RTMP server digest signing */
> > +static const uint8_t rtmp_server_key[] =
> > +{
> > + 0x47, 0x65, 0x6E, 0x75, 0x69, 0x6E, 0x65, 0x20, 0x41, 0x64, 0x6F, 0x62, 0x65,
> > + 0x20, 0x46, 0x6C, 0x61, 0x73, 0x68, 0x20, 0x4D, 0x65, 0x64, 0x69, 0x61, 0x20,
> > + 0x53, 0x65, 0x72, 0x76, 0x65, 0x72, 0x20, 0x30, 0x30, 0x31, // Genuine Adobe Flash Media Server 001
>
> 64 6f 20 79 6f 75 20 63 6f 6e 73 69 64 65 72 20
> 68 65 78 20 6d 6f 72 65 20 72 65 61 64 61 62 6c
> 65 20 74 68 61 6e 20 70 6c 61 69 6e 20 74 65 78
> 74 3f 0a
73 75 72 65 6c 79 20 2d 20 66 6f 72 20 68 61 63
6b 65 72 73 2e 20 4d 61 64 65 20 69 74 20 6d 6f
72 65 20 72 65 61 64 61 62 6c 65 20 74 68 6f 75
67 68 0a
> [...]
> > +
> > +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];
> > + double num = 1.0;
> > + uint8_t bool;
> > +
> > + 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);
> > + rtmp_amf_write_tag(&p, AMF_STRING, "connect");
> > + rtmp_amf_write_tag(&p, AMF_NUMBER, &num);
> > + rtmp_amf_write_tag(&p, AMF_OBJECT, NULL);
> > + rtmp_amf_write_tag(&p, AMF_STRING_IN_OBJECT, "app");
> > + rtmp_amf_write_tag(&p, AMF_STRING, 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);
> > + rtmp_amf_write_tag(&p, AMF_STRING_IN_OBJECT, "flashVer");
> > + rtmp_amf_write_tag(&p, AMF_STRING, ver);
> > + rtmp_amf_write_tag(&p, AMF_STRING_IN_OBJECT, "tcUrl");
> > + rtmp_amf_write_tag(&p, AMF_STRING, tcurl);
> > + bool = 0;
> > + rtmp_amf_write_tag(&p, AMF_STRING_IN_OBJECT, "fpad");
> > + rtmp_amf_write_tag(&p, AMF_NUMBER, &bool);
> > + num = 15.0;
> > + rtmp_amf_write_tag(&p, AMF_STRING_IN_OBJECT, "capabilities");
> > + rtmp_amf_write_tag(&p, AMF_NUMBER, &num);
> > + num = 1639.0;
> > + rtmp_amf_write_tag(&p, AMF_STRING_IN_OBJECT, "audioCodecs");
> > + rtmp_amf_write_tag(&p, AMF_NUMBER, &num);
> > + num = 252.0;
> > + rtmp_amf_write_tag(&p, AMF_STRING_IN_OBJECT, "videoCodecs");
> > + rtmp_amf_write_tag(&p, AMF_NUMBER, &num);
> > + num = 1.0;
> > + rtmp_amf_write_tag(&p, AMF_STRING_IN_OBJECT, "videoFunction");
> > + rtmp_amf_write_tag(&p, AMF_NUMBER, &num);
> > + rtmp_amf_write_tag(&p, AMF_OBJECT_END, NULL);
>
> it feels a little ugly to do things like
> num=1
> write(&num)
> instead of
> write(1);
changed that
> [...]
> > +//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.
> [...]
> > +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?
> [...]
> > + case RTMP_PT_INVOKE:
> > + if (!memcmp(pkt->data, "\002\000\006_error", 9)) {//TODO: search data for error description
>
> yes i agree with the TODO
removed it and added code instead
> [...]
> > +int rtmp_packet_write(URLContext *h, RTMPPacket *pkt,
> > + int chunk_size, RTMPPacket *prev_pkt)
> > +{
> > + uint8_t pkt_hdr[16], *p = pkt_hdr;
> > + int mode = RTMP_PS_TWELVEBYTES;
> > + int off = 0;
> > +
> > +// if (pkt->type != RTMP_PT_INVOKE)
> > +// mode = RTMP_PS_EIGHTBYTES;
> > + bytestream_put_byte(&p, pkt->channel_id | (mode << 6));
>
> that is debuging left over? or unfinished?
> either way random outcommented code with no comment is not ok IMHO
fixed
> [...]
> > +/* maximum possible number of different RTMP channels */
> > +#define RTMP_CHANNELS 64
>
> doxy
done
> [...]
> > +/**
> > + * AMF types used in RTMP packets
> > + */
> > +typedef enum AMFType {
> > + AMF_NUMBER = 0, ///< number (double precision)
> > + AMF_BOOLEAN, ///< boolean value
> > + AMF_STRING, ///< Pascal-style string with length < 65536
> > + AMF_OBJECT, ///< AMF object, contains property names and values
> > + AMF_MOVIE, ///< Flash object
> > + AMF_NULL, ///< NULL value
> > + AMF_UNDEFINED, ///< undefined (return?) value
> > + AMF_REFERENCE, ///< reference
> > + AMF_ECMA_ARRAY, ///< ECMA array, almost like AMF object but has number of entries
> > + AMF_OBJECT_END, ///< marker for end of AMF object or ECMA array
> > + AMF_STRICT_ARRAY, ///< strict array
> > + AMF_DATE, ///< date
> > + AMF_LONG_STRING, ///< Pascal-style string with possible length up to 4GB
> > + AMF_UNSUPPORTED, ///< unsipported feature indicator
> > + AMD_RECORD_SET, ///< record set
> > + AMF_XML_OBJECT, ///< XML object
> > + AMF_TYPED_OBJECT, ///< typed object
> > +
> > + AMF_STRING_IN_OBJECT = 99, ///< internal type used for AMF object field names
> > +} AMFType;
>
> is this duplicated of flv.h ?
yes, about 90%
Now I reuse it from flv.h
> [...]
> > +/**
> > + * 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
> > + */
> > +int rtmp_packet_create(RTMPPacket *pkt, int channel_id, RTMPPacketType type,
> > + int timestamp, int size);
>
> that is missing a ff/av prefix or static keyword
changed
> [...]
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
-------------- next part --------------
A non-text attachment was scrubbed...
Name: rtmp.patch
Type: text/x-diff
Size: 40202 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090718/692b504f/attachment.patch>
More information about the ffmpeg-devel
mailing list