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

[Carl Eugen Hoyos]

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

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

What I meant was:
If this message is printed, shouldn't you return 
AVERROR_INVALIDDATA or AVERROR_PATCHWELCOME?

> >> +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))"

What about the following?
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

My comment was wrong.

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

That's not how I read the code in question but I 
may miss something.

> >> +    wav->rifx = 0;
> > Should be unneeded.
> 
> Okay, we can rely on the zero-initialization of 
> the WAVDemuxContext structure.

Yes, please do rely on the initialization.

Carl Eugen