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

[Aurelien Jacobs]

Michael Niedermayer wrote:

> On Fri, Dec 21, 2007 at 12:11:17AM +0100, Aurelien Jacobs wrote:
> > 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)
> 
> You cannot really prevent misuse, just make it harder

Right. And I think my proposition was pretty good at making it harder.

> and clearly documented that it is misuse.

Documentation would be better than nothing.
Maybe something like this ?

struct ByteIOContext {
    unsigned char *buffer;             /**< Don't access this field, it's reserved to aviobuf.c */
    int buffer_size;                   /**< Don't access this field, it's reserved to aviobuf.c */
    unsigned char *buf_ptr, *buf_end;  /**< Don't access this field, it's reserved to aviobuf.c */
...

> Just look at:
> /* XXX: put an inline version */
> int get_byte(ByteIOContext *s)
> 
> If that XXX is done

It is not. That's a non-issue right now.

> Iam in favor of preventing misuse were it can be done easily and
> cleanly in case of ByteIOContext though i dont think that set/get
> functions for most fields are a good idea. And its not just
> set_is_streamed() but also the ones existing already as well ...

Which existing ones ? init_put_byte() ? (Which is basically a
set_everything_but_the_new_fields_added_after_this_function())

> > That's why I wanted to move the ByteIOContext definition inside
> > aviobuf.c (which is the only file which really need it).
> 
> see above ...

Well, I won't insist about this. I just thought that this move
had almost no cost and made API somewhat stronger.

Aurel