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

[Reimar Döffinger]

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.