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

[Baptiste Coudurier]

On 3/4/11 1:32 PM, Ronald S. Bultje wrote:
> 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.

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.

-- 
Baptiste COUDURIER
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org