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

[Ronald S. Bultje]

Hi,

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.

Ronald