[FFmpeg-devel] [PATCH 6/8] lavf/http: Implement server side network code.

[Nicolas George]

Le decadi 10 thermidor, an CCXXIII, Stephan Holljes a écrit :
> I thought it was necessary for the application to indicate it wants to
> finish the handshake. I think with the FINAL state this is
> superfluous.

Eventually, applications must be able to choose, but the default can be
anything, and should be the most common case.

Moreover, you need to prioritize: the API has to be ready for it (more or
less: we can consider it unstable and accept incompatible changes for some
time), but you do not have to start coding all the future ideas immediately.
Less code to debug at once for you and to review for me.

For example, eventually, the application needs to be able to decide the
reply based on the Accept-Language header, but you must not start writing
code about it now, you must just think "the application takes control
between reading the headers and sending the reply, there is a context that
can store any information, the Accept-Language and other relevant headers
can go there" and implement it, with all the pitfalls (especially guarding
against OoM DoS), later.

> Okay, since http_write_header() uses a buffer with the same size and
> its headers are a lot longer, I am pretty sure the headers will fit.

I had no doubt about it. But checking it does not cost much: since it only
uses data from internal origin, an assert is acceptable:

    len = snprintf(...);
    av_assert0(len < sizeof(buf));

> I think at first I misunderstood what you meant with the decreasing
> return values. I hope I understood it correctly this time.

The decreasing bit is very minor, but it seems convenient for debugging to
have different values with a simple scheme. Looks ok at first glance,
although not necessarily the simplest way; cf. the patch reviews themselves.

> After some research and looking at av_opt_get(), if I understand it
> correctly, it may return empty string if the option to retrieve is
> NULL. Therefore I think it is necessary to check for strlen(resource).

Urgh. I was not aware of that, but you are perfectly right. And even if it
is really set, there is a strdup(). That is rather inefficient.

I suppose we could use av_opt_ptr() to access the field directly, but that
is a bit low-level. A new av_opt_get_pointer() to get the real pointer from
pointer-like (strings and binary) options would probably be useful.

I will get to the patch review. Note that in the meantime, you can use the
branching capabilities of Git to continue, for example get to FATE
integration. I think the patches are stable enough to build on them, but
learning how FATE works will take a little time anyway.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150728/b3cd4747/attachment.sig>