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

[Alban Bedel]

On Fri, 17 Mar 2006 12:25:04 +0200
"Ivan Kalvachev" <ikalvachev at gmail.com> wrote:

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

Yep unix file descriptor can be made non blocking, and ? It have nothing
to do with the pb at hand.

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

I know, it still doesn't make it true.

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

No, data is "ignored" only if the skipped data is already in the buffer.
Otherwise a seek is performed.

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

Not really because in practice you get at least one ip packet, so at
least a few hundred bytes.

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

I didn't ignored the discussion, otherwise i wouldn't even bother
posting that. I did check the code before posting my first reply,
and came to the conclusion that the patch was indeed the best solution,
so i applied it. Sure i could have given a longer list of reason why i
came to this conclusion, or why this bug have more chance to be seen
with ftp. But i'm not here to explain you how unix socket and the stream
API work.

> Please revert and fix your bug in ftp.

Again their no bug there.

> you can put assert that checks for len<BUFF && eof=0 in seek code.

If you do that most (if not all) network protocols will randomly break
on seek.

	Albeu