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

[Carl Eugen Hoyos]

Thomas Volkert <silvo <at> gmx.net> writes:

> +#include <netinet/in.h>

This will hopefully be unneeded.

>       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);
...
}
If you do not reindent the little endian block, the 
patch gets much easier to read.

> +        if(!big_endian)
> +            codec->bits_per_coded_sample = avio_rl16(pb);
> +        else
> +            codec->bits_per_coded_sample = ntohs(avio_rl16(pb));

avio_rb32(pb) and please add braces around if - else blocks.

>       if (size >= 18) {  /* We're obviously dealing with WAVEFORMATEX */
> +        if (big_endian)
> +            avpriv_report_missing_feature(codec,
> "WAVEFORMATEX support for RIFX files\n");

Is this sufficient, no further error handling needed?

> +    if (!big_endian)
> +        return avio_rl32(pb);
> +    else
> +        return ntohl(avio_rl32(pb));

avio_rb32() and braces.

> -        if (!memcmp(p->buf, "RIFF", 4))
> +        if ((!memcmp(p->buf, "RIFF", 4)) || (!memcmp(p->buf, "RIFX", 4)))

Maybe I misread but this looks like too many parentheses.

> -    int rf64;
> +    int rf64 = 0;

Should be unneeded.

> -    /* check RIFF header */
> -    tag = avio_rl32(pb);

nit: You could not remove the variable and do a switch (tag) 
below to make the patch smaller.
(Smaller patch == easier review, both now and in the future)

> +    wav->rifx = 0;

Should be unneeded.

One line looked to me as if it contained a tab, use 
tools/patcheck to check your patch before submitting.

Thank you!

Carl Eugen