[FFmpeg-devel] [PATCH] avio: fix fourcc if any character is >=0x80.

[Ronald S. Bultje]

Hi Baptiste,

On Fri, Mar 4, 2011 at 4:53 PM, Baptiste Coudurier
<baptiste.coudurier at gmail.com> wrote:
> On 3/4/11 1:32 PM, Ronald S. Bultje wrote:
>> On Fri, Mar 4, 2011 at 4:20 PM, Baptiste Coudurier
>> <baptiste.coudurier at gmail.com> wrote:
>>> On 3/4/11 1:19 PM, Ronald S. Bultje wrote:
>>>> On Fri, Mar 4, 2011 at 4:14 PM, Baptiste Coudurier
>>>> <baptiste.coudurier at gmail.com> wrote:
>>>>> On 3/4/11 7:50 AM, Ronald S. Bultje wrote:
>>>>>> 2011/3/4 M?ns Rullg?rd <mans at mansr.com>:
>>>>>>> "Ronald S. Bultje" <rsbultje at gmail.com> writes:
>>>>>>>
>>>>>>>> Fixes issue 2638.
>>>>>>>> ---
>>>>>>>> ?libavformat/avio_internal.h | ? ?5 ++++-
>>>>>>>> ?1 files changed, 4 insertions(+), 1 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/libavformat/avio_internal.h b/libavformat/avio_internal.h
>>>>>>>> index 3b38990..279c7f6 100644
>>>>>>>> --- a/libavformat/avio_internal.h
>>>>>>>> +++ b/libavformat/avio_internal.h
>>>>>>>> @@ -42,6 +42,9 @@ int ffio_read_partial(AVIOContext *s, unsigned char *buf, int size);
>>>>>>>>
>>>>>>>> ?void ffio_fill(AVIOContext *s, int b, int count);
>>>>>>>>
>>>>>>>> -#define ffio_wfourcc(pb, str) avio_wl32(pb, MKTAG((str)[0], (str)[1], (str)[2], (str)[3]))
>>>>>>>> +static av_always_inline void ffio_wfourcc(AVIOContext *pb, const uint8_t *s)
>>>>>>>> +{
>>>>>>>> + ? ?avio_wl32(pb, MKTAG(s[0], s[1], s[2], s[3]));
>>>>>>>> +}
>>>>>
>>>>> avio_write(pb, s, 4) is faster and better IMHO.
>>>>> Why not using that ?
>>>>
>>>> I'd like to eventually change avio_wl32() into doing that if
>>>> endianness is suitable, just didn't get to it yet.
>>>>
>>>> Also, I believe it should actually use AV_WL32() instead of a plain
>>>> avio_write() (which, on x86-32, expands to the same ASM).
>>>
>>> That won't eliminate the MKTAG which is slower, and in the context of a
>>> "fourcc"/"tag", avio_write is better in every case here.
>>
>> Oh I see, sorry, I am being an idiot. Yes, indeed, currently
>> avio_write() would be better.
>>
>> The one "problem" that avio_write() introduces is that, if feeding it
>> with strings, it will NULL-terminate each string, thus each string
>> takes 5 bytes of rodata plus a pointer to it. By using avio_w[lb]32(),
>> no rodata is used and the "fourcc" takes 4 bytes without a pointer. An
>> ideal implementation of avio_write() would not use a pointer to a
>> NULL-terminated string. My eventual idea for ffio_wfourcc() & co was
>> to basically "fix" this problem.
>>
>> Ideally, calling ffio_wfourcc() on x86-32/64 makes the compiler
>> automatically see that the MKTAG() call is on const data, therefore it
>> automatically generates code with the correct uint32_t fourcc in the
>> assembly, which it uses in a register (64)/stack (32) when calling
>> avio_wl32(), which then directly writes it out without a dereference.
>> Some macro magic could do better-than-average stuff on BE systems
>> also. This would be more optimal than a plain avio_write().
>>
>> For movenc.c, this currently doesn't happen because there's multiple
>> levels of functions. My patch tried to convert everything in "fourcc"s
>> so the dereference to rodata doesn't happen anymore, but you (validly
>> - I admit it was ugly) didn't like that, so I'm not doing that for
>> now. However, I do believe that in an ideal implementation, maybe by
>> using a bunch more macros inside movenc.c or so, it would use this
>> system rather than plain strings.
>
> I see what you want to achieve here.
> Currently, avio_wl32 uses avio_w8, so you need to change that before
> anything can happen.
> It seems to be quite some work to achieve this, and just for muxers, not
> sure it's worth it, but it you do it in a short timeframe, it's nice.

I'll see what I can get done this weekend.

Ronald