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

Submitted by Daniel Kucera on June 4, 2017, 6 a.m.

Details

Message ID 20170604060016.11482-1-daniel.kucera@gmail.com
State New
Headers show

Commit Message

Daniel Kucera June 4, 2017, 6 a.m.
Signed-off-by: Daniel Kucera <daniel.kucera@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(-)

Comments

Nicolas George June 4, 2017, 10:25 a.m.
Le sextidi 16 prairial, an CCXXV, Daniel Kucera a écrit :
> Signed-off-by: Daniel Kucera <daniel.kucera@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,
Daniel Kucera June 4, 2017, 3:05 p.m.
2017-06-04 12:25 GMT+02:00 Nicolas George <george@nsup.org>:
> Le sextidi 16 prairial, an CCXXV, Daniel Kucera a écrit :
>> Signed-off-by: Daniel Kucera <daniel.kucera@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

Ok, I see only suggestions and ideas here. If you have any exact
request for change in my patch, let me know.

Btw, I suggest to include all these changes in one patch so there
won't be any single commit, which would be broken.
Nicolas George June 5, 2017, 5:02 p.m.
Le sextidi 16 prairial, an CCXXV, Daniel Kučera a écrit :
> Ok, I see only suggestions and ideas here. If you have any exact
> request for change in my patch, let me know.

I am not your foreman or anything like that, it is not my place to order
you around.

What I can tell you is that patches need to be good to be applied, and a
patch that makes the code more complex when it could make it simpler
cannot be considered good. Of course "when it could make it simpler" is
only speculation, but if somebody objects based on it, you will need to
convince them otherwise, and for that you will probably need to have
tried doing it that way.

Furthermore, patches that make the code simpler are more likely to get
quick reviews and eventually approval, because simpler code is also
easier to review.

And finally, I can tell you that a patch that breaks receiving empty
packets for packet protocols cannot be accepted.

> Btw, I suggest to include all these changes in one patch so there
> won't be any single commit, which would be broken.

And I strongly advise otherwise. The changes to make protocols return
AVERROR_EOF instead of 0 are good and necessary all by themselves,
individually and without the framework changes to catch the 0 return
values. I am pretty confident that they do not break anything.

Regards,

Patch hide | download patch | download mbox

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)
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) {