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

[Thomas Volkert]

On 12/15/2014 11:24 AM, Carl Eugen Hoyos wrote:
> Thomas Volkert <silvo <at> gmx.net> writes:
>
>> +#include <netinet/in.h>
> This will hopefully be unneeded.
>
Okay, this will simplify the patch.


>>        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?

I do not have an example file in RIFX format with WAVEFORMATEX. So, I 
concentrated on the usual file format.
The sent patch does not influence the usual RIFF decoder but it extends 
it by a first support for big endian values.
Maybe someone else can extend my patch with support for WAVEFORMATEX in 
the future.


>> -        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.

The compiler would not accept the following construction:
"if (!memcmp(p->buf, "RIFF", 4)) || (!memcmp(p->buf, "RIFX", 4))"

>
>> -    int rf64;
>> +    int rf64 = 0;
> Should be unneeded.

In the previous version of the code, the variable rf64 was initialized 
in every case by the following:
"rf64 = tag == MKTAG('R', 'F', '6', '4');"
I simplified the code and replaced this with a default value, which gets 
overwritten, if the file header has RF64 format


>
>> -    /* 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)

The variable "tag" is initialized by "size = next_tag(pb, &tag, 
wav->rifx);" before the following switch-case.

>
>> +    wav->rifx = 0;
> Should be unneeded.
>

Okay, we can rely on the zero-initialization of the WAVDemuxContext 
structure.

Best regards,
Thomas.