[FFmpeg-devel] [GSoC] Proof-of-concept HTTP Server

[Nicolas George]

Le sextidi 6 germinal, an CCXXIII, Stephan Holljes a écrit :
> I hope this time the patch is formatted correctly. I also attached it
> in case it is corrupted again.

Sorry, still wrapped. See there:
http://ffmpeg.org/pipermail/ffmpeg-devel/2015-March/170768.html

Especially the lines with the options after #define OFFSET. I suspect you
can not achieve something correct with gmail's web interface. The attached
version is fine, though.

> I changed the indentation of the code and used ffurl_open() instead of
> creating my own listening socket.
> 
> I am still having some trouble with the Content-Type header. I would
> guess creating functions like http_write_header() as the counterpart
> for http_read_header() would be the most appropriate approach?

The request in the client part also has headers, so the code should be
shared. But I believe that goes beyond the scope of the qualification task.
For now, a constant header with hardcoded content-type seems fine.

> On Sun, Mar 22, 2015 at 10:33 AM, Nicolas George <george at nsup.org> wrote:

Please remember not to top-post on FFmpeg's mailing lists; if you do not
know what it means, look it up.

Also, I believe the deadline for applications is drawing near. Please make
sure you did all paperwork necessary for Google.

See below for technical comments on the patch itself:

> From 3ec0259abd9025a1fba06a9cc4c8ee771675f5d7 Mon Sep 17 00:00:00 2001
> From: klaxa <klaxa1337 at googlemail.com>
> Date: Thu, 26 Mar 2015 03:40:07 +0100
> Subject: [PATCH] Implemented simple listen mode for http.

The first line of a Git message starts usually with the context: not "do foo
for bar" but "bar: do foo".

> 
> ---
>  libavformat/http.c | 33 ++++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)

There is a small section about http in doc/protocols.texi, it would be nice
to have the option. Just something short:

"listen: enable experimental HTTP server mode"

> 
> diff --git a/libavformat/http.c b/libavformat/http.c
> index da3c9be..472d8dd 100644
> --- a/libavformat/http.c
> +++ b/libavformat/http.c
> @@ -96,6 +96,8 @@ typedef struct HTTPContext {
>      int send_expect_100;
>      char *method;
>      int reconnect;
> +    int listen;
> +    int header_sent;
>  } HTTPContext;
>  
>  #define OFFSET(x) offsetof(HTTPContext, x)
> @@ -127,6 +129,7 @@ static const AVOption options[] = {
>      { "end_offset", "try to limit the request to bytes preceding this offset", OFFSET(end_off), AV_OPT_TYPE_INT64, { .i64 = 0 }, 0, INT64_MAX, D },
>      { "method", "Override the HTTP method", OFFSET(method), AV_OPT_TYPE_STRING, { .str = NULL }, 0, 0, E },
>      { "reconnect", "auto reconnect after disconnect before EOF", OFFSET(reconnect), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 1, D },

> +    { "listen", "listen on HTTP", OFFSET(listen), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 1, D },

Does the proof of concept work for input, output, or both? The "D" at the
end seems to indicate it is only for receiving, but your patch changes
http_write().

>      { NULL }
>  };
>  
> @@ -300,7 +303,9 @@ static int http_open(URLContext *h, const char *uri, int flags,
>                       AVDictionary **options)
>  {
>      HTTPContext *s = h->priv_data;
> -    int ret;

> +    int port, ret = 0;
> +    char hostname[1024];
> +    char lower_url[100];

Since these are only used in the listen case, maybe declare them in the
corresponding bloc.

>  
>      if( s->seekable == 1 )
>          h->is_streamed = 0;
> @@ -321,6 +326,20 @@ static int http_open(URLContext *h, const char *uri, int flags,
>                     "No trailing CRLF found in HTTP header.\n");
>      }
>  
> +    if (s->listen) {
> +        av_url_split(NULL, 0, NULL, 0, hostname, sizeof(hostname), &port,
> +                     NULL, 0, uri);
> +        ff_url_join(lower_url, sizeof(lower_url), "tcp", NULL, hostname, port,
> +                NULL);

> +        av_dict_set(options, "listen", "1", AV_DICT_APPEND);

Are you sure about AV_DICT_APPEND?

> +        ret = ffurl_open(&s->hd, lower_url, AVIO_FLAG_READ_WRITE,
> +                   &h->interrupt_callback, options);

I suspect you need a full-duplex connection for any case. Also, please align
the second line with the parenthesis.

> +        if (ret < 0) {
> +            return ret;
> +        }
> +
> +        return ret;

The test on ret seems a bit redundant since the same thing is done anyway
just after. Also, we usually do not put braces on single-line if blocs.

> +    }
>      ret = http_open_cnx(h, options);
>      if (ret < 0)
>          av_dict_free(&s->chained_options);
> @@ -1100,10 +1119,22 @@ static int http_read(URLContext *h, uint8_t *buf, int size)
>  static int http_write(URLContext *h, const uint8_t *buf, int size)
>  {
>      char temp[11] = "";  /* 32-bit hex + CRLF + nul */
> +    char header[] = "HTTP 200 OK\r\nContent-Type: application/octet-stream\r\n\r\n";
>      int ret;
>      char crlf[] = "\r\n";
>      HTTPContext *s = h->priv_data;
> +    if (s->listen) {

> +        if (!s->header_sent) {
> +            ret = ffurl_write(s->hd, header, strlen(header));

The logic here seems strange. Is there a reason you do not send the headers
immediately after establishing the connection?

Also, in this particular case, you seem to be completely ignoring the
request. Well, if the request is small enough it should work at least with
some clients, but it will only work for sending content, not receiving, so
you probably should test that the URL context is not used for input.

> +            if (ret < 0) {
> +                return ff_neterrno();
> +            }

ret is already the correct error code, ff_neterrno() is only for when
calling the network code directly.

> +            s->header_sent = 1;
> +        }
>  

> +        ret = ffurl_write(s->hd, buf, size);
> +        return ret < 0 ? ff_neterrno() : ret;
> +    }
>      if (!s->chunked_post) {
>          /* non-chunked data is sent without any special encoding */
>          return ffurl_write(s->hd, buf, size);

I believe the ffurl_write() you just added and the ffurl_write just below do
exactly the same thing, you can probably just leave the code reach here.

In fact, I suspect that if you move the headers sending in the http_open()
function, you can leave http_write() completely unchanged.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150326/bd445c7f/attachment.asc>