[FFmpeg-devel] [PATCH 2/3] avformat/aviobuf: fix checks in ffio_ensure_seekback

[Michael Niedermayer]

On Sat, Sep 26, 2020 at 12:46:40PM +0200, Marton Balint wrote:
> 
> 
> On Sat, 26 Sep 2020, Michael Niedermayer wrote:
> 
> > On Sun, Sep 20, 2020 at 10:52:52AM +0200, Marton Balint wrote:
> > > Signed-off-by: Marton Balint <cus at passwd.hu>
> > > ---
> > >  libavformat/aviobuf.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > The commit message is too terse. It is not clear from it what the problem
> > is that is being fixed and how it is fixed.
> > Also this feels like it fixes multiple issues so it probably should be spit
> > unless iam missing some interdependancies
> 
> It can be seperated to two patches if that is preferred:
> 
> 1) existing code does not check if the requested seekback buffer is already
> read entirely. In this case, nothing has to be done. This is the newly added
> if() at the beginning of the patch.
> 
> 2) the new buf_size is detemined too conservatively, maybe because of the
> issue which was fixed in patch 1/3. So we can safely substract 1 more from
> the new buffer size. Comparing the new buf_size against filled does not make
> a lot of sense to me, that just seems wrong. What makes sense is that we
> want to reallocate the buffer if the new buf_size is bigger than the old,
> therefore the change in the check.

i would prefer it split yes


> 
> > 
> > ill also take a look at the "competing" patch, so i dont yet know
> > how they relate ...
> > 
> > 
> > > 
> > > diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
> > > index 9675425349..aa1d6c0830 100644
> > > --- a/libavformat/aviobuf.c
> > > +++ b/libavformat/aviobuf.c
> > > @@ -999,9 +999,12 @@ int ffio_ensure_seekback(AVIOContext *s, int64_t buf_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 <= s->buf_end - s->buf_ptr)
> > > +        return 0;
> > > +
> > > +    buf_size += s->buf_ptr - s->buffer + max_buffer_size - 1;
> > > 
> > 
> > > -    if (buf_size < filled || s->seekable || !s->read_packet)
> > > +    if (buf_size <= s->buffer_size || s->seekable || !s->read_packet)
> > >          return 0;
> > 
> > Did you check that this doesnt interfere with the s->buffer_size reduction
> > we do when it was larger from probing ?
> > Its maybe ok, just checking that this was considered
> 
> I don't see how that can be affected by this change, so I am not sure what
> you mean here. If the new buffer size is bigger than s->orig_buffer_size,
> then eventually it will be reduced, it does not seem more complicated than
> that to me.

what i meant was that if its reduced then at that point the seekback 
gurantee changes relative to it never being reduced.
I think you consider this in your code, i just wanted to double check.

thx


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Into a blind darkness they enter who follow after the Ignorance,
they as if into a greater darkness enter who devote themselves
to the Knowledge alone. -- Isha Upanishad
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200926/b84857e0/attachment.sig>