[FFmpeg-devel] [PATCH] libavformat/aviobuf.c: don't treat 0 from read_packet as EOF

Nicolas George george at nsup.org
Sun Jun 4 13:25:06 EEST 2017


Le sextidi 16 prairial, an CCXXV, Daniel Kucera a écrit :
> Signed-off-by: Daniel Kucera <daniel.kucera at gmail.com>
> ---
>  libavformat/avio.c    |  2 +-
>  libavformat/aviobuf.c | 20 ++++++++++++--------
>  libavformat/cache.c   |  4 ++--
>  libavformat/file.c    |  2 ++
>  libavformat/subfile.c |  2 +-
>  libavformat/wtvdec.c  |  4 ++--
>  6 files changed, 20 insertions(+), 14 deletions(-)

I have several qualms about this patch.

Firs, there are several direct calls to read_packet, I am not sure it
addresses all of them. I think it would be better to have a single
low-level wrapper for read_packet and use only it.

Second, these changes do not handle packet protocols. For packet
protocols, read returning 0 means an empty packet, and that must be
returned to the application. It does not mean EOF.

I suggest the following check around read_packet :

	av_assert2(ret || url->max_packet_size);
	if (!ret && !url->max_packet_size)
	    ret = AVERROR_EOF;

The av_assert2() allows debug builds to crash whenever an invalid return
value happens, making it easier to detect them. The second check allows
normal builds to run, same as before. And of course, you should be
working and running FATE at assert level 2.

It leaves a small problem for stream protocols that are thin wrappers
around another protocol that may or may not be packet : they must all
check 0 and ignore it. Fortunately, we have only a handful of thin
wrappers.

See comments below for a few details.

> 
> diff --git a/libavformat/avio.c b/libavformat/avio.c
> index 1e79c9dd5c..bf803016b7 100644
> --- a/libavformat/avio.c
> +++ b/libavformat/avio.c
> @@ -394,7 +394,7 @@ static inline int retry_transfer_wrapper(URLContext *h, uint8_t *buf,
>                  av_usleep(1000);
>              }
>          } else if (ret < 1)
> -            return (ret < 0 && ret != AVERROR_EOF) ? ret : len;
> +            return (ret < 0) ? ret : len;
>          if (ret) {
>              fast_retries = FFMAX(fast_retries, 2);
>              wait_since = 0;
> diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
> index 1667e9f08b..3705e406d9 100644
> --- a/libavformat/aviobuf.c
> +++ b/libavformat/aviobuf.c
> @@ -556,13 +556,14 @@ static void fill_buffer(AVIOContext *s)
>      if (s->read_packet)
>          len = s->read_packet(s->opaque, dst, len);
>      else
> -        len = 0;
> -    if (len <= 0) {
> +        len = AVERROR_EOF;
> +    if (len == AVERROR_EOF) {
>          /* do not modify buffer if EOF reached so that a seek back can
>             be done without rereading data */
>          s->eof_reached = 1;
> -        if (len < 0)
> -            s->error = len;
> +    } else if (len < 0) {
> +        s->eof_reached = 1;
> +        s->error= len;
>      } else {
>          s->pos += len;
>          s->buf_ptr = dst;
> @@ -630,13 +631,16 @@ int avio_read(AVIOContext *s, unsigned char *buf, int size)
>                  // bypass the buffer and read data directly into buf
>                  if(s->read_packet)
>                      len = s->read_packet(s->opaque, buf, size);
> -
> -                if (len <= 0) {
> +		else
> +                    len = AVERROR_EOF;
> +                if (len == AVERROR_EOF) {
>                      /* do not modify buffer if EOF reached so that a seek back can
>                      be done without rereading data */
>                      s->eof_reached = 1;
> -                    if(len<0)
> -                        s->error= len;
> +                    break;
> +                } else if (len < 0) {
> +                    s->eof_reached = 1;
> +                    s->error= len;
>                      break;
>                  } else {
>                      s->pos += len;
> diff --git a/libavformat/cache.c b/libavformat/cache.c
> index 6aabca2e78..66bbbf54c9 100644
> --- a/libavformat/cache.c
> +++ b/libavformat/cache.c
> @@ -201,7 +201,7 @@ static int cache_read(URLContext *h, unsigned char *buf, int size)
>      }
>  
>      r = ffurl_read(c->inner, buf, size);
> -    if (r == 0 && size>0) {
> +    if (r == AVERROR_EOF && size>0) {
>          c->is_true_eof = 1;
>          av_assert0(c->end >= c->logical_pos);
>      }
> @@ -263,7 +263,7 @@ resolve_eof:
>                  if (whence == SEEK_SET)
>                      size = FFMIN(sizeof(tmp), pos - c->logical_pos);
>                  ret = cache_read(h, tmp, size);
> -                if (ret == 0 && whence == SEEK_END) {
> +                if (ret == AVERROR_EOF && whence == SEEK_END) {
>                      av_assert0(c->is_true_eof);
>                      goto resolve_eof;
>                  }
> diff --git a/libavformat/file.c b/libavformat/file.c
> index 264542a36a..1fb83851c0 100644
> --- a/libavformat/file.c
> +++ b/libavformat/file.c
> @@ -112,6 +112,8 @@ static int file_read(URLContext *h, unsigned char *buf, int size)
>      ret = read(c->fd, buf, size);
>      if (ret == 0 && c->follow)
>          return AVERROR(EAGAIN);
> +    if (ret == 0)
> +        return AVERROR_EOF; 
>      return (ret == -1) ? AVERROR(errno) : ret;
>  }
>  

> diff --git a/libavformat/subfile.c b/libavformat/subfile.c
> index fa971e1902..497cf85211 100644
> --- a/libavformat/subfile.c
> +++ b/libavformat/subfile.c
> @@ -102,7 +102,7 @@ static int subfile_read(URLContext *h, unsigned char *buf, int size)
>      int ret;
>  

>      if (rest <= 0)
> -        return 0;
> +        return AVERROR_EOF;
>      size = FFMIN(size, rest);
>      ret = ffurl_read(c->h, buf, size);
>      if (ret >= 0)

This part LGTM. If submitted as a separate patch, it can go in
immediately.

The same goes probably for all the individual fixes in protocols, but I
only maintain this one. I suggest you submit all of them as individual
patches and get them applied while you polish the framework patch.

> diff --git a/libavformat/wtvdec.c b/libavformat/wtvdec.c
> index 3ac4501306..ee19fd84da 100644
> --- a/libavformat/wtvdec.c
> +++ b/libavformat/wtvdec.c
> @@ -65,7 +65,7 @@ static int64_t seek_by_sector(AVIOContext *pb, int64_t sector, int64_t offset)
>  }
>  
>  /**
> - * @return bytes read, 0 on end of file, or <0 on error
> + * @return bytes read, AVERROR_EOF on end of file, or <0 on error
>   */
>  static int wtvfile_read_packet(void *opaque, uint8_t *buf, int buf_size)
>  {
> @@ -76,7 +76,7 @@ static int wtvfile_read_packet(void *opaque, uint8_t *buf, int buf_size)
>      if (wf->error || pb->error)
>          return -1;
>      if (wf->position >= wf->length || avio_feof(pb))
> -        return 0;
> +        return AVERROR_EOF;
>  
>      buf_size = FFMIN(buf_size, wf->length - wf->position);
>      while(nread < buf_size) {

Regards,

-- 
  Nicolas George


More information about the ffmpeg-devel mailing list