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

[Ivan Kalvachev]

2006/3/16, Alban Bedel <albeu at free.fr>:
> 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 :)

Well, in http://m-w.com the first definition for the 'fill' verb is:
1 a : to put into as much as can be held or conveniently contained
<fill a cup with water> b : to supply with a full complement <the
class is already filled>

> > Wont it be better to fix it inside stream_fill_buffer(), compared to
> > make same fix on 2 (or more) places?
> >
> > 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.

Reading from pipe would block if there is not enough data. However
there is non-blocking file mode that would cause the read to return
only the available at the moment data. This mode is not set by default
but by fcntl(fd,F_SETFD,O_NONBLOCK).  The same is true for all/most
unix files.
The situation with network is a little different.

I Have stated that THIS bug is in your ftp code and must be fixed there.
Something like this may help (hand made patch!) :

#stream_ftp.c
@@292
+ r = recv(s->fd,buffer,max_len,MSG_WAITALL);

(haven't tested it, could have side effects, like discarding the rest
of the final packet.... but it worthies a try ;)

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

Well, I don't see where this confidence comes to you. But I don't see
sense in this reasoning. The demuxers read all the data. They read it
even when skipping. In case of network this data is coming and you can
read it or ignore it. Seeking is different matter.

But let's assume that you are right and examine the worst case
scenario: stream_fill_buffer() fills single byte when it is called
(this after all depends on outside factors). Would this affect how
fast demuxers are working? (not only in the seek code but in regular
readings)

The block reading is done as intent to optimize the reading, buffer
size of 2kb is small enough to don't cause long stalls, big enough to
don't cause constant refilling, and is also the size of cdrom sector
(it is changed for  vcd).


P.S.
I'm outrageous about your commit while we were in discussion.
Why the hell are you posting this on ml and then ignore the discussion!!!!!!!

Please revert and fix your bug in ftp.
you can put assert that checks for len<BUFF && eof=0 in seek code.