[FFmpeg-devel] [PATCH] wavdec: RIFX file format support

[Benoit Fouet]

Hi,

Le 17/12/2014 22:15, Reimar Döffinger a écrit :
> On Wed, Dec 17, 2014 at 11:55:17AM +0100, Thomas Volkert wrote:
>> On 12/16/2014 08:36 AM, Reimar Döffinger wrote:
>>> On Mon, Dec 15, 2014 at 10:24:55AM +0000, Carl Eugen Hoyos wrote:
>>>>>        codec->sample_rate = avio_rl32(pb);
>>>>>        codec->bit_rate    = avio_rl32(pb) * 8;
>>>>>        codec->block_align = avio_rl16(pb);
>>>>> +    if (big_endian) {
>>>>> +        id                 = ntohs(id);
>>>>> +        codec->channels    = ntohs(codec->channels);
>>>>> +        codec->sample_rate = ntohl(codec->sample_rate);
>>>>> +        codec->bit_rate    = ntohl(codec->bit_rate / 8) * 8;
>>>>> +        codec->block_align = ntohs(codec->block_align);
>>>>> +    }
>>>> Instead please do:
>>>> if (big_endian) {
>>>>    id = avio_rb32(pb);
>>>>    codec->channels = avio_rb32(pb);
>>>>   ...
>>>> } else {
>>>> id = avio_rl32(pb);
>>>> ...
>>>> }
>>> Not sure this is a good idea, as it duplicates the code.
>>> It might be better to use the if as-is, just replacing ntoh*
>>> by av_bswap*.
>> I would prefer this version:
>> if (!big_endian) {
>>   avio_rl32()
>> } else {
>>   avio_rb32()
>> }
>> In case of big endianess, your idea (and my former solution) would need two
>> instead of one updates per value
> Why would that matter? Performance, especially for big-endian type is
> hardly relevant.
> Due to fewer branches it might actually give better performance for the
> common case (but as said I don't think it matters).
> It mostly avoids duplicating some actual code like the *8.
> There are also other options to reduce the code size, though I am
> sceptical if they are a good idea.
> 1) Macros like:
> #define GET16(pb) (big_endian ? avio_rb16(pb) : avio_rl16(pb))
> 2) Functions that take an additional endian argument.

While we're at it:
3) Function pointers initialized based on the endianness

-- 
Ben