[FFmpeg-devel] [PATCH] Remove static ByteIOContexts

[Michael Niedermayer]

Hi

On Thu, Nov 15, 2007 at 11:46:51AM +0100, Bj?rn Axelsson wrote:
> On Wed, 2007-11-07 at 03:18 +0100, Michael Niedermayer wrote:
> > On Tue, Nov 06, 2007 at 03:42:22PM +0100, Bj?rn Axelsson wrote:
> > > To facilitate future extensions of ByteIOContext without breaking the
> > > ABI Michael suggested[1] changing
> > > AVFormatContext{
> > >     ByteIOContext pb;
> > > to
> > > AVFormatContext{
> > >     ByteIOContext *pb;
> > > 
> > > This is my attempt at doing that. 
> > > 
> > > Basically url_fopen(), url_fdopen(), url_open_buf and url_open_dyn_*()
> > > are all changed to take a ByteIOContext ** pointer which is used to
> > > return a newly allocated ByteIOContext in. This is consistent with how
> > > url_open() works with URLContexts.
> > 
> > > 
> > > I believe this patch will break compability with most applications,
> > 
> > you need to increase the major version part of LIBAVFORMAT_VERSION*
> 
> version bump patch now included (versionbump.diff)
> 
> > > since AVFormatContext.pb must be checked for NULL before accessed (if a
> > > format with AVFMT_NOFILE is used). Both ffmpeg and ffplay needed
> > > additional checks for this. Of course, any application using the changed
> > > url_* functions will also need some minor changes.
> > 
> > iam not overly happy about these NULL checks
> > wouldnt having url_fseek() return EINVAL or something like that for NULL
> > contextes instead of crashing avoid most of these NULL checks ?
> 
> I don't feel that either solution is perfect, but in this patch series
> most null checks have been moved to aviobuf instead. url_fseek() and
> url_fsize() now return EINVAL, while  url_feof() and url_ferror() both
> return 0 when called with a NULL parameter. 
> That is the minimal number of patched functions to make ffmpeg and
> ffplay work.
> 
> > [...]
> 
> > > Index: libavformat/aviobuf.c
> > > ===================================================================
> > > --- libavformat/aviobuf.c.orig	2007-11-06 13:20:58.466007062 +0100
> > > +++ libavformat/aviobuf.c	2007-11-06 13:28:05.390808599 +0100
> > > @@ -508,12 +508,11 @@
> > >      //return 0;
> > >  }
> > >  
> > > -int url_fdopen(ByteIOContext *s, URLContext *h)
> > > +int url_fdopen(ByteIOContext **s, URLContext *h)
> > >  {
> > 
> > >      uint8_t *buffer;
> > >      int buffer_size, max_packet_size;
> > >  
> > > -
> > >      max_packet_size = url_get_max_packet_size(h);
> > 
> > cosmetic
> 
> I must have thought that the double empty line was my fault and removed
> it. Patch fixed.
> 
> > [...]
> > 
> > > Index: ffmpeg_avformatcontext/libavformat/utils.c
> > > ===================================================================
> > > --- ffmpeg_avformatcontext.orig/libavformat/utils.c	2007-11-06 14:32:23.144460497 +0100
> > > +++ ffmpeg_avformatcontext/libavformat/utils.c	2007-11-06 15:23:54.708850579 +0100
> > > @@ -1079,6 +1079,9 @@
> > >      int index;
> > >      AVStream *st;
> > >  
> > > +    if(s->pb == NULL)
> > > +        return -1;
> > > +
> > >      if (stream_index < 0)
> > >          return -1;
> > >  
> > > @@ -1141,6 +1144,9 @@
> > >      int64_t start_pos, filesize;
> > >      int no_change;
> > >  
> > > +    if(! s->pb)
> > > +        return -1;
> > 
> > inconsistant, you are mixing ! and ==NULL, i prefer !
> 
> All NULL tests changed to avoid == NULL or != NULL.
> 
> Patches synced with current SVN, and the full series still passes
> regression tests.
> 
> Slightly revised patch series and suggested commit messages:

[...]

>  2. bioc_formats_1.diff
>     Update libavformats for dynamic ByteIOContexts

ok (though doesnt apply anymore, not suprissing considering its size)
this one could get tricky timing wise as it likely will break again quickly
if reposted 
also when reposting please indicate which patches have changed so i dont
need to review all of them again and my patch application monkeys just
need to run md5sum over them to verify


[...]

>  6. bioc_ffplay_reindent.diff
>     Reindent from previous patch

unneeded (see below)


other patches ok

[...]

> Index: ffplay.c
> ===================================================================
> --- ffplay.c.orig	2007-11-15 10:39:38.556821825 +0100
> +++ ffplay.c	2007-11-15 11:16:12.328122250 +0100
> @@ -1912,7 +1912,8 @@
>          ret = -1;
>          goto fail;
>      }
> -    ic->pb.eof_reached= 0; //FIXME hack, ffplay maybe should not use url_feof() to test for the end
> +    if(ic->pb)
> +    ic->pb->eof_reached= 0; //FIXME hack, ffplay maybe should not use url_feof() to test for the end

as this is changed anyway you can indent it properly, this also makes the
reindent patch unneeded

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

It is not what we do, but why we do it that matters.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20071116/d1d13ffc/attachment.pgp>