[FFmpeg-devel] http: Return AVERROR_EOF instead of 0 in some EOF conditions

IO functions are expected to return AVERROR_EOF instead of 0
nowadays. This is also expected by other higher level layers
within the http protocol itself (e.g. the reconnect mechanism).
---
 libavformat/http.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

On Fri, Nov 13, 2020 at 12:32 AM Martin Storsjö <martin@martin.st> wrote:
>
> IO functions are expected to return AVERROR_EOF instead of 0
> nowadays. This is also expected by other higher level layers
> within the http protocol itself (e.g. the reconnect mechanism).
> ---
>  libavformat/http.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libavformat/http.c b/libavformat/http.c
> index 3d25d652d3..528ea046ef 100644
> --- a/libavformat/http.c
> +++ b/libavformat/http.c
> @@ -1408,12 +1408,12 @@ static int http_buf_read(URLContext *h, uint8_t *buf, int size)
>              if (!s->chunksize && s->multiple_requests) {
>                  http_get_line(s, line, sizeof(line)); // read empty chunk
>                  s->chunkend = 1;
> -                return 0;
> +                return AVERROR_EOF;
>              }

Trying to read into the context of this change, it seems like this
case is supposed to actually cause a retry, and the earlier if
(s->chunkend) check returns EOF on the following call (
http://git.videolan.org/?p=ffmpeg.git;a=commit;h=91a565e20f0c220a6bc37e58c11cea4a3590149c
).

Quoting the commit: <<EOQ
After this patch, EOF is emulated once a 0-byte final chunk is
received by setting a new flag. This flag is reset in ff_http_do_new_request(),
which is used to make additional requests on the open socket.
EOQ

Of course, if this specific case does not actually require this sort
of loop-de-loop any more then this code can be somewhat simplified,
but otherwise it seems like this return 0 is here for a reason (?).

>              else if (!s->chunksize) {
>                  av_log(h, AV_LOG_DEBUG, "Last chunk received, closing conn\n");
>                  ffurl_closep(&s->hd);
> -                return 0;
> +                return AVERROR_EOF;
>              }

This one seems more correct for an EOF.

Although not knowing the implementations of other protocols the choice
of closing the underlying TLS/TCP connection ASAP is an interesting
one (as it's also done in http_close). But that is unrelated to this
change.

Jan

On Fri, 20 Nov 2020, Jan Ekström wrote:

> On Fri, Nov 13, 2020 at 12:32 AM Martin Storsjö <martin@martin.st> wrote:
>>
>> IO functions are expected to return AVERROR_EOF instead of 0
>> nowadays. This is also expected by other higher level layers
>> within the http protocol itself (e.g. the reconnect mechanism).
>> ---
>>  libavformat/http.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavformat/http.c b/libavformat/http.c
>> index 3d25d652d3..528ea046ef 100644
>> --- a/libavformat/http.c
>> +++ b/libavformat/http.c
>> @@ -1408,12 +1408,12 @@ static int http_buf_read(URLContext *h, uint8_t *buf, int size)
>>              if (!s->chunksize && s->multiple_requests) {
>>                  http_get_line(s, line, sizeof(line)); // read empty chunk
>>                  s->chunkend = 1;
>> -                return 0;
>> +                return AVERROR_EOF;
>>              }
>
> Trying to read into the context of this change, it seems like this
> case is supposed to actually cause a retry, and the earlier if
> (s->chunkend) check returns EOF on the following call (
> http://git.videolan.org/?p=ffmpeg.git;a=commit;h=91a565e20f0c220a6bc37e58c11cea4a3590149c
> ).
>
> Quoting the commit: <<EOQ
> After this patch, EOF is emulated once a 0-byte final chunk is
> received by setting a new flag. This flag is reset in ff_http_do_new_request(),
> which is used to make additional requests on the open socket.
> EOQ
>
> Of course, if this specific case does not actually require this sort
> of loop-de-loop any more then this code can be somewhat simplified,
> but otherwise it seems like this return 0 is here for a reason (?).

Good point, and this change seems to have been made after the move towards 
AVERROR_EOF in the IO layer, so I presume it's intentional then.

>>              else if (!s->chunksize) {
>>                  av_log(h, AV_LOG_DEBUG, "Last chunk received, closing conn\n");
>>                  ffurl_closep(&s->hd);
>> -                return 0;
>> +                return AVERROR_EOF;
>>              }
>
> This one seems more correct for an EOF.

I think I'll defer this patch in any case; I hadn't investigated this one 
much, I just made it as a drive-by change after looking into the missing 
premature end error.

// Martin