[Ffmpeg-devel] Re: [PATCH] file length handling

[Michael Niedermayer]

Hi

On Mon, Dec 18, 2006 at 08:55:43PM +0100, Ronald S. Bultje wrote:
> Hi Michael,
> 
> On Mon, 18 Dec 2006, Michael Niedermayer wrote:
> > On Mon, Dec 18, 2006 at 09:20:49AM -0500, Ronald S. Bultje wrote:
> > > -#define LIBAVFORMAT_VERSION_INT ((51<<16)+(6<<8)+0)
> > > -#define LIBAVFORMAT_VERSION     51.6.0
> > > +#define LIBAVFORMAT_VERSION_INT ((52<<16)+(0<<8)+0)
> > > +#define LIBAVFORMAT_VERSION     52.0.0
> > >  #define LIBAVFORMAT_BUILD       LIBAVFORMAT_VERSION_INT
> >
> > how is compatibility broken? if it isnt then theres no need to bump
> > the version number up
> 
> I'm not sure if URLContext is external API or not, similar for
> ByteIOContext. If not, then I'll remove this.

external or not how does adding a field to the end break anyting?
no external code should depend on any struct size if it does its
broken IMHO


> 
> > >      s->seek(s->opaque, s->pos, SEEK_SET);
> > >      return size;
> > >  }
> > > @@ -516,6 +519,7 @@
> > >      }
> > >      s->is_streamed = h->is_streamed;
> > >      s->max_packet_size = max_packet_size;
> > > +    s->file_size = h->file_size;
> >
> > the copying of possible non constant variables is bad, iam not sure how to
> > best solve it though, maybe function ptrs would after all be better then a
> > variable, dunno
> [..]
> > > Index: ffmpeg-mpe/libavformat/file.c
> > > ===================================================================
> > > --- ffmpeg-mpe.orig/libavformat/file.c	2006-12-10 12:54:52.000000000 -0500
> > > +++ ffmpeg-mpe/libavformat/file.c	2006-12-18 09:02:53.000000000 -0500
> > > @@ -110,6 +110,7 @@
> > >  #endif
> > >      h->priv_data = (void *)(size_t)fd;
> > >      h->is_streamed = 1;
> > > +    h->file_size = -1;
> >
> > checking against 0 and setting to -1 wont work
> >
> > and the default value should be set in url_open() or another common place
> > (of course only if its nor zeroed anyway...)
> 
> The default is set to 0. file.c's file:// handler keeps this. The stream
> handlers (tcp://, udp://, rtp:// and pipe://) set it to -1, because you
> can't retrieve the length (i.e. -1 means error or no length available or
> something). So this is intentional. 

ok, but please document that with a doxygen comment for file_size


> The value of file_size is supposed to
> be constant (as is the case for streams and also for http, since you only
> get Content-Length once), so I think it should be safe to copy it into
> ByteIOContext.

hmm, size can change for files on your harddisk though they dont use file_size
currently so i dunno, maybe file_size in both is ok ...

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you really think that XML is the answer, then you definitly missunderstood
the question -- Attila Kinali