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

[Thomas Volkert]

On 12/17/2014 10:57 PM, Benoit Fouet wrote:
> 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
>

For this RIFX patch, I would rather use the current version I mailed before.
Afterwards, I suggest to do refactor some functions:
* ff_get_wav_header() in riffdec.c: replace the "avio_rl16 / avio_rb16" 
calls by some better approach
* wav_read_header() in wavdec.c: use the switch-case construct which was 
included in the first version of the RIFX patch

Best regards,
Thomas.