diff mbox series

[FFmpeg-devel,1/2] avformat/wtvdec: Forward errors when reading packet

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
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Andreas Rheinhardt Feb. 10, 2020, 5:21 p.m. UTC
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(-)

Comments

Andreas Rheinhardt Feb. 18, 2020, 12:36 p.m. UTC | #1
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
Peter Ross Feb. 19, 2020, 9:16 a.m. UTC | #2
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)
Michael Niedermayer Feb. 19, 2020, 9:13 p.m. UTC | #3
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 mbox series

Patch

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;
 }
 
 /**