| Message ID | 20200210172138.15596-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avformat/wtvdec: Forward errors when reading packet | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
Andreas Rheinhardt: > wtvfile_read_packet did not abide by the requirements of an > AVIOContext's read_packet-function: If it did not read anything, > it returned zero, which currently leads to a warning in read_packet_wrapper > in aviobuf.c. Said warning will be an av_assert2 as soon as > FF_API_OLD_AVIO_EOF_0 is zero (probably the next major version bump). > So instead forward the error code from the underlying protocol. > > This error/assert is triggered in the wtv-demux FATE test. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > This patch supersedes [1]. The difference to the previous version is > that I have changed the return value from "nread ? nread : n ? : > AVERROR_EOF" to "nread ? nread : n". This can only make a difference if > buf_size (the size of the data to be read) is zero and I now think that > this should simply not happen: It would be possible by calling > avio_read_partial with a desired size of 0 and if the AVIOContext's > write_flag is set, but avio_read_partial is not called with one of > wtvdec's custom AVIOContexts (not to mention that if this were a > problem, it should probably be fixed in avio_read_partial). > > [1]: https://patchwork.ffmpeg.org/project/ffmpeg/patch/20190821090438.10260-4-andreas.rheinhardt@gmail.com/ > > libavformat/wtvdec.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/libavformat/wtvdec.c b/libavformat/wtvdec.c > index 67d934f074..83f510b92f 100644 > --- a/libavformat/wtvdec.c > +++ b/libavformat/wtvdec.c > @@ -71,7 +71,7 @@ static int wtvfile_read_packet(void *opaque, uint8_t *buf, int buf_size) > { > WtvFile *wf = opaque; > AVIOContext *pb = wf->pb_filesystem; > - int nread = 0; > + int nread = 0, n = 0; > > if (wf->error || pb->error) > return -1; > @@ -80,7 +80,6 @@ static int wtvfile_read_packet(void *opaque, uint8_t *buf, int buf_size) > > buf_size = FFMIN(buf_size, wf->length - wf->position); > while(nread < buf_size) { > - int n; > int remaining_in_sector = (1 << wf->sector_bits) - (wf->position & ((1 << wf->sector_bits) - 1)); > int read_request = FFMIN(buf_size - nread, remaining_in_sector); > > @@ -100,7 +99,7 @@ static int wtvfile_read_packet(void *opaque, uint8_t *buf, int buf_size) > } > } > } > - return nread; > + return nread ? nread : n; > } > > /** > Ping. - Andreas
On Tue, Feb 18, 2020 at 12:36:00PM +0000, Andreas Rheinhardt wrote: > Andreas Rheinhardt: > > wtvfile_read_packet did not abide by the requirements of an > > AVIOContext's read_packet-function: If it did not read anything, > > it returned zero, which currently leads to a warning in read_packet_wrapper > > in aviobuf.c. Said warning will be an av_assert2 as soon as > > FF_API_OLD_AVIO_EOF_0 is zero (probably the next major version bump). > > So instead forward the error code from the underlying protocol. > > > > This error/assert is triggered in the wtv-demux FATE test. > > > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > > --- > > This patch supersedes [1]. The difference to the previous version is > > that I have changed the return value from "nread ? nread : n ? : > > AVERROR_EOF" to "nread ? nread : n". This can only make a difference if > > buf_size (the size of the data to be read) is zero and I now think that > > this should simply not happen: It would be possible by calling > > avio_read_partial with a desired size of 0 and if the AVIOContext's > > write_flag is set, but avio_read_partial is not called with one of > > wtvdec's custom AVIOContexts (not to mention that if this were a > > problem, it should probably be fixed in avio_read_partial). > > > > [1]: https://patchwork.ffmpeg.org/project/ffmpeg/patch/20190821090438.10260-4-andreas.rheinhardt@gmail.com/ > > > > libavformat/wtvdec.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/libavformat/wtvdec.c b/libavformat/wtvdec.c > > index 67d934f074..83f510b92f 100644 > > --- a/libavformat/wtvdec.c > > +++ b/libavformat/wtvdec.c > > @@ -71,7 +71,7 @@ static int wtvfile_read_packet(void *opaque, uint8_t *buf, int buf_size) > > { > > WtvFile *wf = opaque; > > AVIOContext *pb = wf->pb_filesystem; > > - int nread = 0; > > + int nread = 0, n = 0; > > > > if (wf->error || pb->error) > > return -1; > > @@ -80,7 +80,6 @@ static int wtvfile_read_packet(void *opaque, uint8_t *buf, int buf_size) > > > > buf_size = FFMIN(buf_size, wf->length - wf->position); > > while(nread < buf_size) { > > - int n; > > int remaining_in_sector = (1 << wf->sector_bits) - (wf->position & ((1 << wf->sector_bits) - 1)); > > int read_request = FFMIN(buf_size - nread, remaining_in_sector); > > > > @@ -100,7 +99,7 @@ static int wtvfile_read_packet(void *opaque, uint8_t *buf, int buf_size) > > } > > } > > } > > - return nread; > > + return nread ? nread : n; > > } > > > > /** > > > > Ping. looks good. please apply. -- Peter (A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
On Wed, Feb 19, 2020 at 08:16:03PM +1100, Peter Ross wrote: > On Tue, Feb 18, 2020 at 12:36:00PM +0000, Andreas Rheinhardt wrote: > > Andreas Rheinhardt: > > > wtvfile_read_packet did not abide by the requirements of an > > > AVIOContext's read_packet-function: If it did not read anything, > > > it returned zero, which currently leads to a warning in read_packet_wrapper > > > in aviobuf.c. Said warning will be an av_assert2 as soon as > > > FF_API_OLD_AVIO_EOF_0 is zero (probably the next major version bump). > > > So instead forward the error code from the underlying protocol. > > > > > > This error/assert is triggered in the wtv-demux FATE test. > > > > > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > > > --- > > > This patch supersedes [1]. The difference to the previous version is > > > that I have changed the return value from "nread ? nread : n ? : > > > AVERROR_EOF" to "nread ? nread : n". This can only make a difference if > > > buf_size (the size of the data to be read) is zero and I now think that > > > this should simply not happen: It would be possible by calling > > > avio_read_partial with a desired size of 0 and if the AVIOContext's > > > write_flag is set, but avio_read_partial is not called with one of > > > wtvdec's custom AVIOContexts (not to mention that if this were a > > > problem, it should probably be fixed in avio_read_partial). > > > > > > [1]: https://patchwork.ffmpeg.org/project/ffmpeg/patch/20190821090438.10260-4-andreas.rheinhardt@gmail.com/ > > > > > > libavformat/wtvdec.c | 5 ++--- > > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > > > diff --git a/libavformat/wtvdec.c b/libavformat/wtvdec.c > > > index 67d934f074..83f510b92f 100644 > > > --- a/libavformat/wtvdec.c > > > +++ b/libavformat/wtvdec.c > > > @@ -71,7 +71,7 @@ static int wtvfile_read_packet(void *opaque, uint8_t *buf, int buf_size) > > > { > > > WtvFile *wf = opaque; > > > AVIOContext *pb = wf->pb_filesystem; > > > - int nread = 0; > > > + int nread = 0, n = 0; > > > > > > if (wf->error || pb->error) > > > return -1; > > > @@ -80,7 +80,6 @@ static int wtvfile_read_packet(void *opaque, uint8_t *buf, int buf_size) > > > > > > buf_size = FFMIN(buf_size, wf->length - wf->position); > > > while(nread < buf_size) { > > > - int n; > > > int remaining_in_sector = (1 << wf->sector_bits) - (wf->position & ((1 << wf->sector_bits) - 1)); > > > int read_request = FFMIN(buf_size - nread, remaining_in_sector); > > > > > > @@ -100,7 +99,7 @@ static int wtvfile_read_packet(void *opaque, uint8_t *buf, int buf_size) > > > } > > > } > > > } > > > - return nread; > > > + return nread ? nread : n; > > > } > > > > > > /** > > > > > > > Ping. > > looks good. please apply. will apply thx [...]
diff --git a/libavformat/wtvdec.c b/libavformat/wtvdec.c index 67d934f074..83f510b92f 100644 --- a/libavformat/wtvdec.c +++ b/libavformat/wtvdec.c @@ -71,7 +71,7 @@ static int wtvfile_read_packet(void *opaque, uint8_t *buf, int buf_size) { WtvFile *wf = opaque; AVIOContext *pb = wf->pb_filesystem; - int nread = 0; + int nread = 0, n = 0; if (wf->error || pb->error) return -1; @@ -80,7 +80,6 @@ static int wtvfile_read_packet(void *opaque, uint8_t *buf, int buf_size) buf_size = FFMIN(buf_size, wf->length - wf->position); while(nread < buf_size) { - int n; int remaining_in_sector = (1 << wf->sector_bits) - (wf->position & ((1 << wf->sector_bits) - 1)); int read_request = FFMIN(buf_size - nread, remaining_in_sector); @@ -100,7 +99,7 @@ static int wtvfile_read_packet(void *opaque, uint8_t *buf, int buf_size) } } } - return nread; + return nread ? nread : n; } /**
wtvfile_read_packet did not abide by the requirements of an AVIOContext's read_packet-function: If it did not read anything, it returned zero, which currently leads to a warning in read_packet_wrapper in aviobuf.c. Said warning will be an av_assert2 as soon as FF_API_OLD_AVIO_EOF_0 is zero (probably the next major version bump). So instead forward the error code from the underlying protocol. This error/assert is triggered in the wtv-demux FATE test. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- This patch supersedes [1]. The difference to the previous version is that I have changed the return value from "nread ? nread : n ? : AVERROR_EOF" to "nread ? nread : n". This can only make a difference if buf_size (the size of the data to be read) is zero and I now think that this should simply not happen: It would be possible by calling avio_read_partial with a desired size of 0 and if the AVIOContext's write_flag is set, but avio_read_partial is not called with one of wtvdec's custom AVIOContexts (not to mention that if this were a problem, it should probably be fixed in avio_read_partial). [1]: https://patchwork.ffmpeg.org/project/ffmpeg/patch/20190821090438.10260-4-andreas.rheinhardt@gmail.com/ libavformat/wtvdec.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)