| Message ID | 20200917103106.29078-1-onemda@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avformat/aviobuf: fix broken logic in ffio_ensure_seekback() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On Thu, 17 Sep 2020, Paul B Mahol wrote: > This removes big CPU overhead for demuxing chained ogg streams. > > Signed-off-by: Paul B Mahol <onemda@gmail.com> > --- > libavformat/aviobuf.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c > index a77517d712..ce9b7d59c9 100644 > --- a/libavformat/aviobuf.c > +++ b/libavformat/aviobuf.c > @@ -996,20 +996,20 @@ int ffio_ensure_seekback(AVIOContext *s, int64_t buf_size) > uint8_t *buffer; > int max_buffer_size = s->max_packet_size ? > s->max_packet_size : IO_BUFFER_SIZE; > - int filled = s->buf_end - s->buffer; > ptrdiff_t checksum_ptr_offset = s->checksum_ptr ? s->checksum_ptr - s->buffer : -1; > > - buf_size += s->buf_ptr - s->buffer + max_buffer_size; > - > - if (buf_size < filled || s->seekable || !s->read_packet) > + if (buf_size <= s->buffer_size || s->seekable || !s->read_packet) > return 0; This is still not correct. You can only return if buf_size <= s->buf_end - s->buf_ptr OR buf_size <= s->buffer + s->buffer_size - s->buf_ptr - max_buffer_size + 1 And the new minimum buffer size is what is calculated now, so that should not be changed. I am not sure if this fixes you original issue. It might make sense to allocate max_buffer_size*2 length buffers by default, but the buffer size revert logic must be fixed to support that... Regards, Marton > + buf_size += s->buffer_size; > + buf_size = FFMAX(buf_size, max_buffer_size); > + > av_assert0(!s->write_flag); > > buffer = av_malloc(buf_size); > if (!buffer) > return AVERROR(ENOMEM); > > - memcpy(buffer, s->buffer, filled); > + memcpy(buffer, s->buffer, s->buffer_size); > av_free(s->buffer); > s->buf_ptr = buffer + (s->buf_ptr - s->buffer); > s->buf_end = buffer + (s->buf_end - s->buffer); > -- > 2.17.1 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Thu, Sep 17, 2020 at 09:44:52PM +0200, Marton Balint wrote: > > > On Thu, 17 Sep 2020, Paul B Mahol wrote: > > > This removes big CPU overhead for demuxing chained ogg streams. > > > > Signed-off-by: Paul B Mahol <onemda@gmail.com> > > --- > > libavformat/aviobuf.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c > > index a77517d712..ce9b7d59c9 100644 > > --- a/libavformat/aviobuf.c > > +++ b/libavformat/aviobuf.c > > @@ -996,20 +996,20 @@ int ffio_ensure_seekback(AVIOContext *s, int64_t buf_size) > > uint8_t *buffer; > > int max_buffer_size = s->max_packet_size ? > > s->max_packet_size : IO_BUFFER_SIZE; > > - int filled = s->buf_end - s->buffer; > > ptrdiff_t checksum_ptr_offset = s->checksum_ptr ? s->checksum_ptr - s->buffer : -1; > > > > - buf_size += s->buf_ptr - s->buffer + max_buffer_size; > > - > > - if (buf_size < filled || s->seekable || !s->read_packet) > > + if (buf_size <= s->buffer_size || s->seekable || !s->read_packet) > > return 0; > > This is still not correct. You can only return if > buf_size <= s->buf_end - s->buf_ptr That one gives you original issue. > OR > buf_size <= s->buffer + s->buffer_size - s->buf_ptr - max_buffer_size + 1 > That one gives you also original issue but at slower peace. > And the new minimum buffer size is what is calculated now, so that should > not be changed. > > I am not sure if this fixes you original issue. It might make sense to > allocate max_buffer_size*2 length buffers by default, but the buffer size > revert logic must be fixed to support that... > > Regards, > Marton > > > > > + buf_size += s->buffer_size; > > + buf_size = FFMAX(buf_size, max_buffer_size); > > + > > av_assert0(!s->write_flag); > > > > buffer = av_malloc(buf_size); > > if (!buffer) > > return AVERROR(ENOMEM); > > > > - memcpy(buffer, s->buffer, filled); > > + memcpy(buffer, s->buffer, s->buffer_size); > > av_free(s->buffer); > > s->buf_ptr = buffer + (s->buf_ptr - s->buffer); > > s->buf_end = buffer + (s->buf_end - s->buffer); > > -- > > 2.17.1 > > > > _______________________________________________ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > To unsubscribe, visit link above, or email > > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Thu, Sep 17, 2020 at 12:31:06PM +0200, Paul B Mahol wrote: > This removes big CPU overhead for demuxing chained ogg streams. > > Signed-off-by: Paul B Mahol <onemda@gmail.com> > --- > libavformat/aviobuf.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > will apply soon.
On Sat, 19 Sep 2020, Paul B Mahol wrote: > On Thu, Sep 17, 2020 at 12:31:06PM +0200, Paul B Mahol wrote: >> This removes big CPU overhead for demuxing chained ogg streams. >> >> Signed-off-by: Paul B Mahol <onemda@gmail.com> >> --- >> libavformat/aviobuf.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> > > will apply soon. Don't apply it, as I explained, it is not correct. Regards, Marton
On Sat, Sep 19, 2020 at 11:46:50PM +0200, Marton Balint wrote: > > > On Sat, 19 Sep 2020, Paul B Mahol wrote: > > > On Thu, Sep 17, 2020 at 12:31:06PM +0200, Paul B Mahol wrote: > > > This removes big CPU overhead for demuxing chained ogg streams. > > > > > > Signed-off-by: Paul B Mahol <onemda@gmail.com> > > > --- > > > libavformat/aviobuf.c | 10 +++++----- > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > > will apply soon. > > Don't apply it, as I explained, it is not correct. I carefully examined your explanation and I already reported here on mailing list for every one to see that it does not work. So please reconsider your blocking statement and provide correct solution to problem. I can even upload relevant sample somewhere privately for you so you can freely produce correct solution and compare it with my solution for everyones benefit.
On Sun, 20 Sep 2020, Paul B Mahol wrote: > On Sat, Sep 19, 2020 at 11:46:50PM +0200, Marton Balint wrote: >> >> >> On Sat, 19 Sep 2020, Paul B Mahol wrote: >> >> > On Thu, Sep 17, 2020 at 12:31:06PM +0200, Paul B Mahol wrote: >> > > This removes big CPU overhead for demuxing chained ogg streams. >> > > >> > > Signed-off-by: Paul B Mahol <onemda@gmail.com> >> > > --- >> > > libavformat/aviobuf.c | 10 +++++----- >> > > 1 file changed, 5 insertions(+), 5 deletions(-) >> > > >> > >> > will apply soon. >> >> Don't apply it, as I explained, it is not correct. > > I carefully examined your explanation and I already reported > here on mailing list for every one to see that it does not work. > > So please reconsider your blocking statement and provide > correct solution to problem. Your solution will break the fundamentals of ffio_ensure_seekback, so it is not OK. Checking if (buf_size <= s->buffer_size) will not guarantee that the buffer will not be flushed when reading the next buf_size bytes because buf_ptr can be anywhere in the buffer. See how fill_buffer() works. It not only makes sure the buffer has space, it makes sure that the buffer has max_buffer_size space available. If not, then it gets flushed. There are some issues with current code, I will send a patch to fix them. Unfortunately it will NOT fix your problem entirely. As I suggested, if you use 2*max_buffer_size buffers by default, that might help you, but the fundamental problem is that ffio_ensure_seekback simply cannot be implemented efficiently with the current design. The guarantees should be reduced (e.g. ffio_ensure_seekback can flush the buffer and invalidate previous ffio_ensure_seekback request) Regards, Marton
On Sun, Sep 20, 2020 at 10:49:12AM +0200, Marton Balint wrote: [...] > fundamental problem is that ffio_ensure_seekback simply cannot be > implemented efficiently with the current design. The guarantees should be > reduced (e.g. ffio_ensure_seekback can flush the buffer and invalidate > previous ffio_ensure_seekback request) this probably makes sense, would this cause a problem to any code using it ? thx [...]
On Thu, Sep 17, 2020 at 12:31:06PM +0200, Paul B Mahol wrote: > This removes big CPU overhead for demuxing chained ogg streams. > > Signed-off-by: Paul B Mahol <onemda@gmail.com> > --- > libavformat/aviobuf.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) I think we need some fate test for ffio_ensure_seekback() as our tests are ATM we generally have fully seekable inputs so failure of seekback would not be noticed. This is a problem because the buffer IO stuff is not easy to understand and so it is time consuming and error prone to review these patches just by eye. And as we see developers disagree on what is correct Such fate test should at least test various sequentially occuring ffio_ensure_seekback(), a possibly larger initial buffer from probing as well as its later reduction. And also reading requests failing, early EOF seeking, and IO errors thanks [...]
On Sat, Sep 26, 2020 at 12:50:34PM +0200, Michael Niedermayer wrote: > On Thu, Sep 17, 2020 at 12:31:06PM +0200, Paul B Mahol wrote: > > This removes big CPU overhead for demuxing chained ogg streams. > > > > Signed-off-by: Paul B Mahol <onemda@gmail.com> > > --- > > libavformat/aviobuf.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > I think we need some fate test for ffio_ensure_seekback() > as our tests are ATM we generally have fully seekable inputs so > failure of seekback would not be noticed. > > This is a problem because the buffer IO stuff is not easy to > understand and so it is time consuming and error prone to review these patches > just by eye. And as we see developers disagree on what is correct > > Such fate test should at least test various sequentially occuring > ffio_ensure_seekback(), a possibly larger initial buffer from probing > as well as its later reduction. And also reading requests failing, early EOF > seeking, and IO errors Well, if its documented that ffio_ensure_seekback() can not flush buffer than there should be added code/function to do allow flushing. Because oggdec works fine with assumed flushing and my patch. > > thanks > > [...] > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > The day soldiers stop bringing you their problems is the day you have stopped > leading them. They have either lost confidence that you can help or concluded > you do not care. Either case is a failure of leadership. - Colin Powell > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c index a77517d712..ce9b7d59c9 100644 --- a/libavformat/aviobuf.c +++ b/libavformat/aviobuf.c @@ -996,20 +996,20 @@ int ffio_ensure_seekback(AVIOContext *s, int64_t buf_size) uint8_t *buffer; int max_buffer_size = s->max_packet_size ? s->max_packet_size : IO_BUFFER_SIZE; - int filled = s->buf_end - s->buffer; ptrdiff_t checksum_ptr_offset = s->checksum_ptr ? s->checksum_ptr - s->buffer : -1; - buf_size += s->buf_ptr - s->buffer + max_buffer_size; - - if (buf_size < filled || s->seekable || !s->read_packet) + if (buf_size <= s->buffer_size || s->seekable || !s->read_packet) return 0; + buf_size += s->buffer_size; + buf_size = FFMAX(buf_size, max_buffer_size); + av_assert0(!s->write_flag); buffer = av_malloc(buf_size); if (!buffer) return AVERROR(ENOMEM); - memcpy(buffer, s->buffer, filled); + memcpy(buffer, s->buffer, s->buffer_size); av_free(s->buffer); s->buf_ptr = buffer + (s->buf_ptr - s->buffer); s->buf_end = buffer + (s->buf_end - s->buffer);
This removes big CPU overhead for demuxing chained ogg streams. Signed-off-by: Paul B Mahol <onemda@gmail.com> --- libavformat/aviobuf.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)