[FFmpeg-devel] [PATCH] RTMP client support for lavf

[Diego Biurrun]

On Fri, Jul 17, 2009 at 06:38:46PM +0300, Kostya wrote:
> $subj
> 
> --- Changelog	(revision 19450)
> +++ Changelog	(working copy)
> @@ -28,9 +28,9 @@
>  - DivX (XSUB) subtitle encoder
>  - nonfree libamr support for AMR-NB/WB decoding/encoding removed
>  - Experimental AAC encoder
> +- RTMP support in libavformat
>  
>  
> -

Keep that empty line please.

> --- libavformat/rtmpproto.c	(revision 0)
> +++ libavformat/rtmpproto.c	(revision 0)
> @@ -0,0 +1,636 @@
> +/* needed for gethostname() */
> +#define _XOPEN_SOURCE 600

More and more files need this, maybe it's time we turned it on globally?

silver:/usr/src/ffmpeg $ svn-grep -r _XOPEN_SOURCE *
ffmpeg.c:#define _XOPEN_SOURCE 600
ffserver.c:#define _XOPEN_SOURCE 600
libavcodec/utils.c:#define _XOPEN_SOURCE 600
libavformat/rtpdec.c:#define _XOPEN_SOURCE 600
tools/trasher.c:#define _XOPEN_SOURCE 600

Then again, it's not thaaat many..

> +#include "libavcodec/bytestream.h"
> +#include "libavutil/avstring.h"
> +#include "libavutil/lfg.h"
> +#include "libavutil/sha.h"
> +#include "avformat.h"
> +
> +#include <unistd.h>
> +#include <stdarg.h>
> +#include <sys/time.h>
> +#include "network.h"
> +
> +#include "flv.h"
> +#include "rtmp.h"
> +#include "rtmppkt.h"

Maybe place the system #includes before the local ones..

> +typedef struct RTMPContext {
> +    URLContext*   rtmp_hd;                    ///< context for TCP stream

_hd ?

> +static const uint8_t rtmp_player_key[] =
> +{

Only function declarations should have the opening { on the next line in
K&R style.

> +static const uint8_t rtmp_server_key[] =
> +{

ditto

> +    if (keylen < 64)
> +        memcpy(hmac_buf, key, keylen);
> +    else {
> +        av_sha_init(sha, 256);
> +        av_sha_update(sha,key, keylen);
> +        av_sha_final(sha, hmac_buf);
> +    }
> +
> +    if (gap <= 0)
> +        av_sha_update(sha, src, len);
> +    else { //skip 32 bytes used for storing digest
> +        av_sha_update(sha, src, gap);
> +        av_sha_update(sha, src + gap + 32, len - gap - 32);
> +    }

Michael will tell you that he likes { } around the 'if'-block in these
cases.

> +    case RTMP_PT_INVOKE:
> +        if (!memcmp(pkt->data, "\002\000\006_error", 9)) {//TODO: search data for error description
> +            return -1;
> +        }

pointless {}

> +        if (rpkt.type == RTMP_PT_VIDEO || rpkt.type == RTMP_PT_AUDIO
> +         || rpkt.type == RTMP_PT_NOTIFY) {

This looks weird, IMO better place the || at the end of the line.

> +/**
> + * url syntax: rtp://host:port[?option=val...]

URL

> --- libavformat/rtmppkt.c	(revision 0)
> +++ libavformat/rtmppkt.c	(revision 0)
> @@ -0,0 +1,244 @@
> +
> +/* needed for gethostname() */
> +#define _XOPEN_SOURCE 600

one more, hmmm

> +    switch(type){

switch (type) {

Diego