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

[Michael Niedermayer]

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 and clearly documented
that it is misuse.

Just look at:
/* XXX: put an inline version */
int get_byte(ByteIOContext *s)

If that XXX is done, direct access to buffer and co becomes needed ...
So ByteIOContext will have to be in some header and a person wanting to
misuse it can, by just include this header.

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


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

see above ...


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

r11273 simplified very recently added public things, yes i broke the
ABI strictly speaking but as noone uses that part of the ABI yet it
isnt really breaking anything. Also ive changed public fuctions in it.
Set/get functions would not have prevented this ABI "breakage".

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

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras
-------------- 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/20071221/161f61ef/attachment.pgp>