| Message ID | 20170604060016.11482-1-daniel.kucera@gmail.com |
|---|---|
| State | New |
| Headers | show |
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,
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.
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,
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) {
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(-)