[FFmpeg-devel] [PATCH] clear s->error in avio_read

[wm4]

On Fri, 24 Mar 2017 10:47:53 -0700
Fredrik Hubinette <hubbe-at-google.com at ffmpeg.org> wrote:

> On Fri, Mar 24, 2017 at 12:47 AM, wm4 <nfxjfg at googlemail.com> wrote:
> 
> > On Mon, 20 Mar 2017 14:39:07 -0700
> > Fredrik Hubinette <hubbe-at-google.com at ffmpeg.org> wrote:
> >  
> > > It looks like the value in s->error also comes from an earlier call to
> > > avio_read().
> > > ogg_read_page tries to read 4439 bytes from the file. It has 524 bytes
> > > already buffered.
> > > fill_buffer() fails with an EIO, but because of the buffered bytes,
> > > avio_read()
> > > returns 524 and leaves the EIO in s->error.  
> >
> > Trying to understand this. Is this an EOF situation? As in, it tries to
> > read more bytes than there are left in the stream. In that case, there
> > should probably be no EIO, but AVERROR_EOF (and should possibly handled
> > specially if "short reads" are allowed in this case).
> >
> > It might also be that I'm missing something completely.
> >
> >  
> I think it is. We have a custom IO layer in chrome which currently always
> returns EIO for all unexpected errors. (Including EOF I think).
> I can try changing our code to return AVERROR_EOF when no bytes are read.
> That will cause s->error to be AVERROR_EOF and the
> avio_read() code will end up returning AVERROR_EOF in the second case.
> However, it is my opinion that it would return the correct
> value by accident.

I think the read_packet callback in the AVIO API is supposed to return
the number of bytes that could still be read if a read hits EOF, rather
than an error.

> 
> > error should not be cleared by read so that the "user" can check for
> > errors after a sequence of reads without needing to check after
> > each individual read. Some demuxers do this that would break if
> > error is reset here  
> 
> Ok, I couldn't find a description of how s->error is supposed to work.
> I have two questions:
> 
> Is it reasonable for avio_read to return s->error instead of AVERROR_EOF
> because s->error was already set when avio_read was called? (If not, I can
> patch this by adding return values from fill_buffer.)

Wouldn't that mean the API gets into an "error" state on the first
error, and the error would have to be cleared explicitly? I don't think
that's how the API works.

> Is it reasonable that a *successful* call to the ogg/webm demuxer sets
> s->error?  (If not, maybe s->error should be cleared there?)

Probably no to the former. Unsure about the latter.

> My interpretation of s->error is that it works like errno(3), and should be
> ignored unless a return value indicate an error.
> That seems to be what a lot of the code does, but it is not what
> avio_read() does.

I don't know either. As an API user, I'm using this field only in the
write path (not when reading).

These are all good questions, and it might make sense to bring some
consistency into this. Some documentation on the API might also not
hurt.