[FFmpeg-devel] [PATCH] Remove static ByteIOContexts

[Michael Niedermayer]

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*


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

it surely may be better in some cases to avoid the calls in the first place
but thats a seperate issue, and surely one iam interrested to see corrected
but after this patch has been applied


> 
> I'm not 100% sure about changing the API for the dynbuffer stuff, but I
> included it anyway for consistency and to ensure future binary
> compability. Most formats using those functions never checked for
> failure which is possible in low mem situations. I added some failure
> checks to the affected formats, but that is still somewhat incomplete. I
> can polish that a bit further if this change is blessed by the
> maintainers.
> 
> The complete patch series passes "make test". It doesn't pass "make
> test-server", but neither does a clean svn build for me. 
> 
> Patch order, and suggested commit messages:
> 
>  1. bioc_api_1.diff
>     Dynamically allocate AVFormatContext::ByteIOContext
>  2. bioc_formats_1.diff
>     Update libavformats for dynamic ByteIOContexts
>  3. bioc_utils.diff
>     Update utils.c for dynamic ByteIOContexts
>  4. bioc_utils_guards.diff
>     Handle NULL ByteIOContext pointers.
>  5. bioc_utils_guards_reindent.diff
>     Reindent from previous patch
>  6. bioc_ffplay.diff
>     Update ffplay for dynamic ByteIOContexts
>  7. bioc_ffplay_reindent.diff
>     Reindent from previous patch
>  8. bioc_ffmpeg.diff
>     Update ffmpeg for dynamic ByteIOContexts
>  9. bioc_ffmpeg_reindent.diff
>     Reindent from previous patch
> 10. bioc_ffserver.diff
>     Update ffserver for dynamic ByteIOContexts
> 11. bioc_dynbuf_api.diff
>     Let url_open_buf() and url_open_dyn_*() allocate ByteIOContexts
> 12. bioc_dynbuf_formats.diff
>     Update libavformats for url_open_buf() and url_open_dyn_*() changes
> 13. bioc_dynbuf_ffserver.diff
>     Update ffserver for url_open_buf() and url_open_dyn_*() changes
> 
> [1]:
> http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/2007-October/037140.html
[...]
> 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


[...]

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

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

Many that live deserve death. And some that die deserve life. Can you give
it to them? Then do not be too eager to deal out death in judgement. For
even the very wise cannot see all ends. -- Gandalf
-------------- 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/20071107/c2975b6c/attachment.pgp>