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

[Baptiste Coudurier]

Hi

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