| Message ID | 20170518201249.87904-1-robertmeyers@google.com |
|---|---|
| State | New |
| Headers | show |
On Thu, May 18, 2017 at 01:12:49PM -0700, Rob Meyers wrote: > avio_read may make multiple calls to fill_buffer to accumulate bytes > to fulfill a request. fill_buffer will overwrite previously read data > if it is not prepped. Added a call to 'ffio_ensure_seekback' to adjust > the s->buffer to fill_buffer's liking. This isn't necessary for the > very first call to fill_buffer, thus the "size1 != size" check. > --- > libavformat/aviobuf.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c > index 0a7c39eacd..8e9594cab8 100644 > --- a/libavformat/aviobuf.c > +++ b/libavformat/aviobuf.c > @@ -648,6 +648,11 @@ int avio_read(AVIOContext *s, unsigned char *buf, int size) > s->buf_end = s->buffer/* + len*/; > } > } else { > + if (size1 != size) > + /* this call will ensure fill_buffer will not overwrite previously > + read data. */ > + ffio_ensure_seekback(s, size1); ffio_ensure_seekback() should be called when theres a potential seek back in the following code. There is no seekback in avio_read(), avio_read only moves forward. Some callers may do a avio_read() and or other function calls and then seek back. A caller might do 3 avio_read() and seek back over all ffio_ensure_seekback() generally should be called by the code that later initiates teh seek back ffio_ensure_seekback() is not free it should only be used when it is needed. [...]
avio_read does multiple calls to fill_buffer(). Adjusting things in fill_buffer was frowned upon; how would you suggest I approach this? The id3v2 seek doesn't come into play; the return from avio_read is the "full" read. The call I added in avio_read is only done when there was accumulated data, so avoid reallocating the buffer the first time through. On Thu, May 18, 2017 at 3:53 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > On Thu, May 18, 2017 at 01:12:49PM -0700, Rob Meyers wrote: > > avio_read may make multiple calls to fill_buffer to accumulate bytes > > to fulfill a request. fill_buffer will overwrite previously read data > > if it is not prepped. Added a call to 'ffio_ensure_seekback' to adjust > > the s->buffer to fill_buffer's liking. This isn't necessary for the > > very first call to fill_buffer, thus the "size1 != size" check. > > --- > > libavformat/aviobuf.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c > > index 0a7c39eacd..8e9594cab8 100644 > > --- a/libavformat/aviobuf.c > > +++ b/libavformat/aviobuf.c > > @@ -648,6 +648,11 @@ int avio_read(AVIOContext *s, unsigned char *buf, > int size) > > s->buf_end = s->buffer/* + len*/; > > } > > } else { > > + if (size1 != size) > > + /* this call will ensure fill_buffer will not > overwrite previously > > + read data. */ > > + ffio_ensure_seekback(s, size1); > > ffio_ensure_seekback() should be called when theres a potential > seek back in the following code. > > There is no seekback in avio_read(), avio_read only moves forward. > Some callers may do a avio_read() and or other function calls and > then seek back. A caller might do 3 avio_read() and seek back over all > > ffio_ensure_seekback() generally should be called by the code that > later initiates teh seek back > > ffio_ensure_seekback() is not free it should only be used when it is > needed. > > > > [...] > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Those who are too smart to engage in politics are punished by being > governed by those who are dumber. -- Plato > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
hi On Thu, May 18, 2017 at 10:58:39PM +0000, Rob Meyers wrote: > avio_read does multiple calls to fill_buffer(). Adjusting things in > fill_buffer was frowned upon; how would you suggest I approach this? Its not frowned upon to fix a bug in fill_buffer(), its frowned upon to make a random change that fixes a testcase without fixing the underlaing bug. (there is a bug somewhere) > The > id3v2 seek doesn't come into play; the return from avio_read is the "full" > read. Just tried, if i comment ff_id3v2_read_dict(s->pb, &s->internal->id3v2_meta, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta); out from avformat_open_input() your testcase works > > The call I added in avio_read is only done when there was accumulated data, > so avoid reallocating the buffer the first time through. > > On Thu, May 18, 2017 at 3:53 PM Michael Niedermayer <michael@niedermayer.cc> > wrote: > > > On Thu, May 18, 2017 at 01:12:49PM -0700, Rob Meyers wrote: > > > avio_read may make multiple calls to fill_buffer to accumulate bytes > > > to fulfill a request. fill_buffer will overwrite previously read data > > > if it is not prepped. Added a call to 'ffio_ensure_seekback' to adjust > > > the s->buffer to fill_buffer's liking. This isn't necessary for the > > > very first call to fill_buffer, thus the "size1 != size" check. > > > --- > > > libavformat/aviobuf.c | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c > > > index 0a7c39eacd..8e9594cab8 100644 > > > --- a/libavformat/aviobuf.c > > > +++ b/libavformat/aviobuf.c > > > @@ -648,6 +648,11 @@ int avio_read(AVIOContext *s, unsigned char *buf, > > int size) > > > s->buf_end = s->buffer/* + len*/; > > > } > > > } else { > > > + if (size1 != size) > > > + /* this call will ensure fill_buffer will not > > overwrite previously > > > + read data. */ > > > + ffio_ensure_seekback(s, size1); > > > > ffio_ensure_seekback() should be called when theres a potential > > seek back in the following code. > > > > There is no seekback in avio_read(), avio_read only moves forward. > > Some callers may do a avio_read() and or other function calls and > > then seek back. A caller might do 3 avio_read() and seek back over all > > > > ffio_ensure_seekback() generally should be called by the code that > > later initiates teh seek back > > > > ffio_ensure_seekback() is not free it should only be used when it is > > needed. > > > > > > > > [...] > > -- > > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > > > Those who are too smart to engage in politics are punished by being > > governed by those who are dumber. -- Plato > > _______________________________________________ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Fri, May 19, 2017 at 08:28:23PM +0200, Michael Niedermayer wrote: > hi > > On Thu, May 18, 2017 at 10:58:39PM +0000, Rob Meyers wrote: > > avio_read does multiple calls to fill_buffer(). Adjusting things in > > fill_buffer was frowned upon; how would you suggest I approach this? > > Its not frowned upon to fix a bug in fill_buffer(), its frowned > upon to make a random change that fixes a testcase without fixing > the underlaing bug. > (there is a bug somewhere) should be fixed [...]
I can confirm the current source tree has resolved our issue. On Fri, May 19, 2017 at 12:16 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > On Fri, May 19, 2017 at 08:28:23PM +0200, Michael Niedermayer wrote: > > hi > > > > On Thu, May 18, 2017 at 10:58:39PM +0000, Rob Meyers wrote: > > > avio_read does multiple calls to fill_buffer(). Adjusting things in > > > fill_buffer was frowned upon; how would you suggest I approach this? > > > > Its not frowned upon to fix a bug in fill_buffer(), its frowned > > upon to make a random change that fixes a testcase without fixing > > the underlaing bug. > > > (there is a bug somewhere) > > should be fixed > > [...] > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > The real ebay dictionary, page 1 > "Used only once" - "Some unspecified defect prevented a second use" > "In good condition" - "Can be repaird by experienced expert" > "As is" - "You wouldnt want it even if you were payed for it, if you knew > ..." > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c index 0a7c39eacd..8e9594cab8 100644 --- a/libavformat/aviobuf.c +++ b/libavformat/aviobuf.c @@ -648,6 +648,11 @@ int avio_read(AVIOContext *s, unsigned char *buf, int size) s->buf_end = s->buffer/* + len*/; } } else { + if (size1 != size) + /* this call will ensure fill_buffer will not overwrite previously + read data. */ + ffio_ensure_seekback(s, size1); + fill_buffer(s); len = s->buf_end - s->buf_ptr; if (len == 0)