2017-06-04 12:25 GMT+02:00 Nicolas George <geo...@nsup.org>:
> Le sextidi 16 prairial, an CCXXV, Daniel Kucera a écrit :
>> Signed-off-by: Daniel Kucera <daniel.kuc...@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.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
