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

[Aurelien Jacobs]

Michael Niedermayer wrote:

> On Fri, Dec 21, 2007 at 11:32:35PM +0100, Aurelien Jacobs wrote:
> > 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 */ ...
> 
> Yes, though i wouldnt use the word "reserved" it sounds strange.
> Maybe "internal field, do not access from outside avibuf.c/h"

Ok, I will propose a patch (but not today... I'm too tired).

> > > 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.
> 
> Well ...
> I dont like changes which would need to be reverted in the future ...

I don't like refusing changes only because it might eventually
require to be changed again in a distant future.

> > > 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())
> 
> url_is_streamed()
> url_feof()
> url_ferror()
> url_fileno()
> url_fget_max_packet_size()

Just to be clear, do you propose to remove them ? (at least with
next major version bump)
I'm not against it. It would at least be consistent. (either have
get/set functions for every "user" fields or for none of them,
but not for a random selection of fields)

Aurel