[FFmpeg-devel] [FFmpeg-commits] lavf: replace all uses of url_fskip with avio_seek

[Reimar Döffinger]

On Wed, Mar 02, 2011 at 05:11:43PM +0100, Anton Khirnov wrote:
> On Thu, Mar 03, 2011 at 02:08:19AM +1100, Peter Ross wrote:
> > On Tue, Mar 01, 2011 at 06:32:10PM +0100, Anton Khirnov  wrote:
> > > Module: ffmpeg
> > > Branch: master
> > > Commit: e356fc57a2e9887370caec58d8aafeafd1f336dc
> > > 
> > > Author:    Anton Khirnov <anton at khirnov.net>
> > > Committer: Ronald S. Bultje <rsbultje at gmail.com>
> > > Date:      Mon Feb 28 14:57:55 2011 +0100
> > > 
> > > lavf: replace all uses of url_fskip with avio_seek
> > > 
> > > Signed-off-by: Ronald S. Bultje <rsbultje at gmail.com>
> > > 
> > > ---
> > 
> > [..]
> > 
> > >  libavformat/vocdec.c         |    8 +++---
> > >  libavformat/vqf.c            |    6 ++--
> > >  libavformat/wav.c            |    6 ++--
> > >  libavformat/wtv.c            |   58 +++++++++++++++++++++---------------------
> > 
> > Guys, the patch subject indicates what was done, but not why.
> > Can somebody explain WHY the fskip convention has been replaced
> > with seek. Im ignoring the original patch thread because that
> > has gone political.
> > 
> > This function has existed since I started using FFmpeg a decade ago.
> > In all my demuxers url_fskip has been used to skip a block of unknown
> > or irrelevant data. I don't 'seek' to another location in the file,
> > I 'skip' over the data. The intent of the code becomes less obvious
> > when fseek is used in both instances.
> > 
> > Fskip has never been a problem. What possible benefit does this
> > change give us? Objectively please!
> > 
> > By logical extension, the next patch in this series will be to
> > universally replace get_bits1(gb) with get_bits(gb, 1). Seriously,
> > why not?
> > 
> > I am not one normally to be critical of API and broader framework
> > changes, and dont have the time to read all mails, but this change,
> > and the get/put_tag renaming insanity really pinches a nerve. This
> > is a backward step on the master public API.
> > 
> > Its really late here -- expect an avio_skip() patch from me tomorrow.
> > 
> 
> The old code was very inconsistent in this regard -- about half of it
> used skip(), while the other half used seek(SEEK_CUR). It seemed easier
> and better to just eliminate the skip().

I am quite certain that the inconsistency is perceived mostly through ignorance.
Most of the code is essentially unchanged from a time when choosing the wrong
one would break things.
fskip will have been used for short seeks that would not cause issues with
non-seekable or badly seekable streams like stdin and http, fseek for
cases where you have given up on that.
As such the distinction served as a documentation at least.
I would have preferred to actually enforce that difference by making fskip
print a message when it is used with large values to get more thorough
testing for the stdin/http use cases even by "ordinary" users, however
I did not see any majority for it at that time since most considered it
more important to avoid the fseek(SEEK_CUR) code bloat.