On Mon, Jul 21, 2014 at 10:20:52PM +0200, [email protected] wrote:
> From: Marvin Scholz <[email protected]>
> 
> This adds the Icecast protocol to libav, this is basically a convenience 
> wrapper
> for the http protocol 

Just

  Icecast is basically a convenience wrapper around the HTTP protocol.

is enough.

> ---
>  libavformat/icecast.c | 230 
> ++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 230 insertions(+)
>  create mode 100644 libavformat/icecast.c

Add docs, version bump, changelog entry, see

https://www.libav.org/developer.html#New-codecs-or-formats-checklist

Also, this is not getting compiled w/o Makefile entry.

> --- /dev/null
> +++ b/libavformat/icecast.c
> @@ -0,0 +1,230 @@
> +/*
> + * Icecast protocol for avconv client
> + * Copyright (c) 2000, 2001 Fabrice Bellard

Fabrice wrote this 13 years ago?  Somehow I doubt it.

> +#include "libavutil/avstring.h"
> +#include "avformat.h"
> +#include "internal.h"
> +#include "network.h"
> +#include "http.h"
> +#include "os_support.h"
> +#include "httpauth.h"
> +#include "url.h"
> +#include "libavutil/opt.h"

https://wiki.libav.org/CodingStyle/HeaderOrdering

> +typedef struct {
> +} IcecastContext;

Please no more anonymously typedeffed structs.

> +static const AVOption options[] = {
> +{"ice_genre", "set stream genre", OFFSET(genre), AV_OPT_TYPE_STRING, { 0 }, 
> 0, 0, E },
> +{"ice_name", "set stream description", OFFSET(name), AV_OPT_TYPE_STRING, { 0 
> }, 0, 0, E },
> +{"ice_description", "set stream description", OFFSET(description), 
> AV_OPT_TYPE_STRING, { 0 }, 0, 0, E },
> +{"ice_url", "set stream website", OFFSET(url), AV_OPT_TYPE_STRING, { 0 }, 0, 
> 0, E },
> +{"ice_public", "set if stream is public", OFFSET(public), AV_OPT_TYPE_INT, 
> {.i64 = 0}, 0, 1, E },
> +{"user_agent", "override User-Agent header", OFFSET(user_agent), 
> AV_OPT_TYPE_STRING, {.str = DEFAULT_USER_AGENT}, 0, 0, E },
> +{"password", "set password", OFFSET(pass), AV_OPT_TYPE_STRING, { 0 }, 0, 0, 
> E },
> +{"content_type", "set content-type, MUST be set if not audio/mpeg", 
> OFFSET(content_type), AV_OPT_TYPE_STRING, { 0 }, 0, 0, E },
> +{"legacy_icecast", "use legacy SOURCE method, for Icecast < v2.4", 
> OFFSET(legacy_icecast), AV_OPT_TYPE_INT, {.i64 = 0}, 0, 1, E },
> +{NULL}
> +};

Use proper indentation please and add spaces inside {}.

> +static char * cat_header(char buf[], const char key[], const char value[])
> +{
> +    int len = strlen(key) + strlen(value) + 5;
> +    int is_first = !buf;
> +    

trailing whitespace, more below

> +    if (buf)
> +        len += strlen(buf);
> +    if (!(buf = (char*) av_realloc(buf, len * sizeof(char))))

pointless void* cast and sizeof(char) is kind of silly

> +static int comp_head(const uint8_t *haystack, int hay_size, const uint8_t 
> *needle, int ne_size)

nit: long line

> +    if (ne_size > hay_size)
> +        return -1;
> +    for (int b = 0; b < ne_size; b++) {
> +        if (haystack[b] != needle[b]){
> +            return -1;
> +        }
> +    }

nit: unnecessary {}

Your re

> +static int icecast_open(URLContext *h, const char *uri, int flags)
> +{
> +    IcecastContext *s = h->priv_data;
> +    
> +    // Dict to set options that we pass to HTTP protocol
> +    AVDictionary *opt_dict = NULL;
> +    
> +    // Build header strings
> +    if (s->name && !EMPTY(s->name))
> +        s->headers = cat_header(s->headers, "Ice-Name", s->name);
> +    if (s->description && !EMPTY(s->description))
> +        s->headers = cat_header(s->headers, "Ice-Description", 
> s->description);
> +    if (s->url && !EMPTY(s->url))
> +        s->headers = cat_header(s->headers, "Ice-URL", s->url);
> +    if (s->genre && !EMPTY(s->genre))
> +        s->headers = cat_header(s->headers, "Ice-Genre", s->genre);
> +    if (s->public)
> +        s->headers = cat_header(s->headers, "Ice-Public", "1");
> +    else
> +        s->headers = cat_header(s->headers, "Ice-Public", "0");
> +    
> +    // Set options
> +    av_dict_set(&opt_dict, "method", s->legacy_icecast ? "SOURCE" : "PUT", 
> 0);
> +    av_dict_set(&opt_dict, "auth_type", "basic", 0);
> +    if (s->headers && !EMPTY(s->headers))
> +        av_dict_set(&opt_dict, "headers", s->headers, 0);
> +    if (s->content_type && !EMPTY(s->content_type))
> +        av_dict_set(&opt_dict, "content_type", s->content_type, 0);
> +    
> +    // URI part variables
> +    char h_url[1024];
> +    char host[1024];
> +    char auth[1024];
> +    char path[1024];
> +    int port;

This won't compile, mixing declarations and statements is forbidden.

> +    // Parse URI
> +    av_url_split(NULL, 0, auth, sizeof(auth), host, sizeof(host), &port, 
> path, sizeof(path), uri);

Break lines over 80 characters where easily possible, like here, more below.

> +    // Check for auth data in URI
> +    if (auth[0] != '\0'){
> +        char *p = strchr(auth, ':');
> +        if (p){

space between ) and {, more below

> +            // Setting user and pass from URI
> +            if (s->user != NULL) av_free(s->user);

Break the line after the if.

> +            s->user = (char*) av_malloc(sizeof(char) * user_len);

see above with realloc, more below

_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel

Reply via email to