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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel