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

Michael Niedermayer michaelni
Sat Dec 22 00:03:20 CET 2007


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"


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


> 
> > 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()


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

in just 30sec i found 5 functions which we alraedy have and which arent
needed
you would add at least one more but that wont be enough iam sure ...

also url_fileno() is really really ugly

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

Asymptotically faster algorithms should always be preferred if you have
asymptotical amounts of data
-------------- 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/20071222/3508edff/attachment.pgp>



More information about the ffmpeg-devel mailing list