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

[Alban Bedel]

On Wed, 15 Mar 2006 23:30:56 +0100
Alban Bedel <albeu at free.fr> wrote:

> 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.


Commited.

	Albeu