| Message ID | CAEVbG5rZoPsK5jTA5T0MaYRWB83WqD=KRGztNt_U_BrcgCtDiA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
ping? On Mon, Mar 20, 2017 at 2:49 PM, Fredrik Hubinette <hubbe@google.com> wrote: > Hopefully valid patch attached. > > > On Mon, Mar 20, 2017 at 2:39 PM, Fredrik Hubinette <hubbe@google.com> > 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. >> >> /Fredrik "Hubbe" Hubinette >> >> >> On Mon, Mar 20, 2017 at 1:53 PM, Fredrik Hubinette <hubbe@google.com> >> wrote: >> >>> >>> On Mon, Mar 20, 2017 at 1:35 PM, Michael Niedermayer < >>> michael@niedermayer.cc> wrote: >>> >>>> On Mon, Mar 20, 2017 at 10:21:08AM -0700, Fredrik Hubinette wrote: >>>> > In some cases (when parsing OGG) non-fatal errors can happen, which >>>> > will cause s->error to be set. In most cases, this is not a problem >>>> beucase >>>> > s->error is not checked unless an actual error has occurred. However, >>>> > when avio_read() fails to read more bytes, it checks s->error to >>>> decide if >>>> > it just reached the end of the file, or an error occurred. Since >>>> s->error is >>>> > not modified if no error occurred, this is not reliable unless we >>>> first >>>> > clear >>>> > s->error before reading. >>>> > >>>> > --- >>>> > libavformat/aviobuf.c | 2 ++ >>>> > >>>> > 2 files changed, 7 insertions(+) >>>> >>>> how can the issue you describe be reproduced >>>> >>>> >>> I don't currently have a simple repro. I discovered it while trying to >>> add some buffering >>> between the demuxer and decoder in chrome. One of our tests fails, and >>> when >>> looked into it, I discovered that s->error was already 5 (EIO) when >>> entering >>> avio_read. I have not yet tracked down where the EIO came from >>> originally. >>> >>> also thats not a valid git patc >>>> >>> >>> Ops, I had a chrome patch and I removed the chrome-specific parts, but >>> I must have messed it up. I will make a better patch. >>> >>> >>>> [...] >>>> -- >>>> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB >>>> >>>> Those who are best at talking, realize last or never when they are >>>> wrong. >>>> >>>> _______________________________________________ >>>> ffmpeg-devel mailing list >>>> ffmpeg-devel@ffmpeg.org >>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >>>> >>>> >>> >> >
On Mon, Mar 20, 2017 at 02:49:18PM -0700, Fredrik Hubinette wrote: > Hopefully valid patch attached. > > > On Mon, Mar 20, 2017 at 2:39 PM, Fredrik Hubinette <hubbe@google.com> 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. > > > > /Fredrik "Hubbe" Hubinette > > > > > > On Mon, Mar 20, 2017 at 1:53 PM, Fredrik Hubinette <hubbe@google.com> > > wrote: > > > >> > >> On Mon, Mar 20, 2017 at 1:35 PM, Michael Niedermayer < > >> michael@niedermayer.cc> wrote: > >> > >>> On Mon, Mar 20, 2017 at 10:21:08AM -0700, Fredrik Hubinette wrote: > >>> > In some cases (when parsing OGG) non-fatal errors can happen, which > >>> > will cause s->error to be set. In most cases, this is not a problem > >>> beucase > >>> > s->error is not checked unless an actual error has occurred. However, > >>> > when avio_read() fails to read more bytes, it checks s->error to > >>> decide if > >>> > it just reached the end of the file, or an error occurred. Since > >>> s->error is > >>> > not modified if no error occurred, this is not reliable unless we first > >>> > clear > >>> > s->error before reading. > >>> > > >>> > --- > >>> > libavformat/aviobuf.c | 2 ++ > >>> > > >>> > 2 files changed, 7 insertions(+) > >>> > >>> how can the issue you describe be reproduced > >>> > >>> > >> I don't currently have a simple repro. I discovered it while trying to > >> add some buffering > >> between the demuxer and decoder in chrome. One of our tests fails, and > >> when > >> looked into it, I discovered that s->error was already 5 (EIO) when > >> entering > >> avio_read. I have not yet tracked down where the EIO came from > >> originally. > >> > >> also thats not a valid git patc > >>> > >> > >> Ops, I had a chrome patch and I removed the chrome-specific parts, but > >> I must have messed it up. I will make a better patch. > >> > >> > >>> [...] > >>> -- > >>> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > >>> > >>> Those who are best at talking, realize last or never when they are wrong. > >>> > >>> _______________________________________________ > >>> ffmpeg-devel mailing list > >>> ffmpeg-devel@ffmpeg.org > >>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >>> > >>> > >> > > > aviobuf.c | 2 ++ > 1 file changed, 2 insertions(+) > 3ef8ffcafab28750ede89a2b7390203a95afb59f 0001-clear-s-error-in-avio_read.patch > From a26c186d2ac6d013215e9707fa3508ba1ef8537a Mon Sep 17 00:00:00 2001 > From: Fredrik Hubinette <hubbe@google.com> > Date: Mon, 20 Mar 2017 14:47:06 -0700 > Subject: [PATCH] clear s->error in avio_read > > --- > libavformat/aviobuf.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c > index 4ade4d0d7e..0912dfdb57 100644 > --- a/libavformat/aviobuf.c > +++ b/libavformat/aviobuf.c > @@ -615,6 +615,8 @@ int avio_read(AVIOContext *s, unsigned char *buf, int size) > { > int len, size1; > > + s->error = 0; 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 [...]
From a26c186d2ac6d013215e9707fa3508ba1ef8537a Mon Sep 17 00:00:00 2001 From: Fredrik Hubinette <hubbe@google.com> Date: Mon, 20 Mar 2017 14:47:06 -0700 Subject: [PATCH] clear s->error in avio_read --- libavformat/aviobuf.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c index 4ade4d0d7e..0912dfdb57 100644 --- a/libavformat/aviobuf.c +++ b/libavformat/aviobuf.c @@ -615,6 +615,8 @@ int avio_read(AVIOContext *s, unsigned char *buf, int size) { int len, size1; + s->error = 0; + size1 = size; while (size > 0) { len = FFMIN(s->buf_end - s->buf_ptr, size); -- 2.12.0.367.g23dc2f6d3c-goog