[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