[FFmpeg-devel] [PATCH] don't set is_streamed when it's not

Aurelien Jacobs aurel
Fri Dec 21 00:11:17 CET 2007


Michael Niedermayer wrote:

> On Thu, Dec 20, 2007 at 01:38:10AM +0100, Aurelien Jacobs wrote:
> > Michael Niedermayer wrote:
> > 
> > > On Thu, Dec 20, 2007 at 12:38:14AM +0100, Aurelien Jacobs wrote:
> > > > Hi,
> > > > 
> > > > Currently ffserver uses url_open_buf() and url_open_dyn_buf()
> > > > and then set is_streamed in the resulting ByteIOContext.
> > > > This seems plain wrong. Buffers are really not streamed.
> > > > Attached patch avoid this. OK ?
> > > 
> > > hmm, i dont think so
> > > Its surely true that the buffer in which the header is written is
> > > seekable. But the packets following are not that is we cant seek
> > > back and update the header, this would cause problems as muxers
> > > would think during header writing that they could seek back later
> > > and update things ...
> > 
> > OK. I now understand why is_streamed is needed here.
> > So I guess the right way to avoid direct access to ByteIOContext
> > internals here is to add a url_set_streamed() function to the API ?
> > Is attached patch OK ?
> 
> What is the advantage of having one get and one set function for each
> field in ByteIOContext compared to direct access to ByteIOContext?

None. But that's not my goal ! I won't add a get function for each
field. Only for the one that outside code is allowed to touch.
The advantage is pretty evident. It would prevent misuse of
ByteIOContext. See the recent patch I proposed for rmenc.
Here is what rmdec currently does:

  ByteIOContext *s = ctx->pb;
    [...]
  data_offset_ptr = s->buf_ptr;
    [put some values in the ByteIOContext with put_*(s, *)]
  data_offset_ptr[0] = data_pos >> 24;
  data_offset_ptr[1] = data_pos >> 16;
  data_offset_ptr[2] = data_pos >> 8;
  data_offset_ptr[3] = data_pos;

There is no guarantee this will work, depending on the ByteIOContext
used.

Such misuse of ByteIOContext will happen as long as the structure
definition is part of the API. And even inside FFmpeg itself.
(rmenc is not the only example... see ape.c for another one)

That's why I wanted to move the ByteIOContext definition inside
aviobuf.c (which is the only file which really need it).
I'm not proposing to add one get/set function for every field,
just one set function corresponding to the already existing
url_is_streamed() (and probably also one for read_pause).

Another advantage is that this would allow to modify ByteIOContext
as much as you want without breaking ABI (just in case you didn't
noticed, you broke ABI in r11273 without bumping major version).

Just in case, I attached the diff of my current tree. Don't take it
too seriously and don't bother reviewing it. It's still WIP. But
it compiles fine and shows that this move of ByteIOContext definition
don't require that much changes (and most of them are in fact cleanup).

Aurel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bio.diff
Type: text/x-patch
Size: 15441 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20071221/24685f2c/attachment.bin>



More information about the ffmpeg-devel mailing list