[MPlayer-dev-eng] Re: [PATCH] Fix some seek failure

[Alban Bedel]

On Wed, 15 Mar 2006 22:52:15 +0200
"Ivan Kalvachev" <ikalvachev at gmail.com> wrote:

> 2006/3/15, Alban Bedel <albeu at free.fr>:
> > On Tue, 14 Mar 2006 10:22:17 +0200
> > "Ivan Kalvachev" <ikalvachev at gmail.com> wrote:
> >
> > > 2006/3/13, Alban Bedel <albeu at free.fr>:
> > > >
> > > > Hi,
> > > >
> > > > their is a little bug in stream_seek() which can lead to some
> > > > strange seeking failure, in particular with ftp.
> > > > Seeks are always aligned on the buffer size, then a single
> > > > stream_fill_buffer is made and failure is returned if the wanted
> > > > position is not in the buffer. So that obviously can fail if the
> > > > call to stream_fill_buffer() doesn't fill the whole buffer (it
> > > > doesn't have to).
> > > [..]
> > >
> > > Are you sure it doesn't have to?
> > >
> > > I think stream_fill_buffer is always supposed to return full
> > > buffer, unless EOF is reached (then it returns only the available
> > > data). It is used this way on few other places (e.g. cache.c) and
> > > they would need fixing. It would be better to fix it in
> > > stream_fill_buffer or t
> >
> > I double checked and that should be ok. stream_fill_buffer() is an
> > "internal" function. It is only called from stream_seek_long() and
> > cache_stream_fill_buffer(). The cache use stream_read() to fill his
> > buffer, so no pb there.
> 
> The function name suggest that it fills the buffer, it is not named
> "fill_some_data_if_you_have_pleace" :)

well fill doesn't always imply fully :)
 
> Wont it be better to fix it inside stream_fill_buffer(), compared to
> make same fix on 2 (or more) places?
>
> Hum... let me count the occurances (ignoring the calling context)
> cache.c:324

That's the main warper used to implement the stream_read function
no pb there.

> stream.c:308

Some broken code that was probably never compiled by anybody. I
doubt it ever worked as intended. Still need fixing though.

> stream.c:327

The place that is fixed by this patch.

> stream.h:111 (that binds it to cache_stream_fill_buffer so lines
> bellow are also added...)
> stream.h:117
> stream.h:200
> stream.h243

That's part of the stream_read API all these functions just don't
care how much data their is in the buffer as long as their is some.

> Anyway I think that this fix is actually workaround of bug that is
> somewhere else. For example it could be that ftp streaming returns
> early after reading of single packet without filling the requested
> data. MPlayer will hang eiher in ftp code or in your fix. That's why
> stream reading is done in cache fork.

This has nothing to do with the cache or ftp in particular. When
reading from a network socket it can happend quiet often that read
return less data than requested. Nothing special there, the same is true
when reading from pipe or any kind of unix file/device for that matter.

I have to admit i wasn't too sure at the beginning but now i have came
to the conclusion that having fill_buffer try as much as it can to fill
the buffer is a bad thing. At the fill_buffer level it's not possible to
know how much data the user of the stream want to read. So there is no
point in reading a given number of bytes, for all we know we might
already have more than enouth.

	Albeu