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

Daniel Kučera daniel.kucera at gmail.com
Sun Jun 4 18:05:46 EEST 2017


2017-06-04 12:25 GMT+02:00 Nicolas George <george at nsup.org>:
> 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

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.

-- 

S pozdravom / Best regards
Daniel Kucera.


More information about the ffmpeg-devel mailing list