[FFmpeg-devel] [PATCH] don't set is_streamed when it's not
Fri Dec 21 01:19:11 CET 2007
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:
>>>>>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 = data_pos >> 24;
> data_offset_ptr = data_pos >> 16;
> data_offset_ptr = data_pos >> 8;
> data_offset_ptr = data_pos;
> There is no guarantee this will work, depending on the ByteIOContext
> 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)
> That's why I wanted to move the ByteIOContext definition inside
> aviobuf.c (which is the only file which really need it).
> 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).
My question is why do we want to hide that much ByteIOContext, to me
this structure and all helpers functions like get_buffer, get_be*/le*,
put_buffer, put_be*/le* are pretty useful and should be completely
exported, in a seperate library like libavio would be even better.
Baptiste COUDURIER GnuPG Key Id: 0x5C1ABAAA
SMARTJOG S.A. http://www.smartjog.com
Key fingerprint 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
Phone: +33 1 49966312
More information about the ffmpeg-devel