[FFmpeg-devel] [PATCH 03/14] pthread_frame: merge the functionality for normal decoder init and init_thread_copy

[Anton Khirnov]

Quoting David Bryant (2020-04-01 23:35:13)
> On 3/31/20 2:47 AM, Anton Khirnov wrote:
> >>> Looking at wavpack, the code looks suspicious to me. You allocate one
> >>> dsdctx per channel at init, but AFAIU the number of channels can change
> >>> at each frame.
> >>>
> >> Multichannel WavPack consists of sequences of stereo or mono WavPack blocks that include flags indicating whether they are the
> >> beginning or end of a "frame" or "packet". This allows the number of channels available to be essentially unlimited, however the
> >> total number of channels in a stream may not change. When decoding, if a sequence of blocks generates more or less than the
> >> correct number of channels, this is flagged as an error and overrun is prevented (see libavcodec/wavpack.c, line 1499 and line
> >> 1621).
> > If my reading of the code is correct, line 1499 only checks for overflow
> > of the channel count read for the current sequence. That channel count
> > is exported to AVCodeContext in the block starting on line 1464 and
> > I don't see any checks for whether it is equal to the initial one.
> >
> Full disclosure: I did not write the WavPack decoder. I understand _how_ it works, but I don't necessarily understand _why_ it
> was written the way it was due to gaps in my knowledge of the FFmpeg internals, and there are certainly things in there that
> don't look quite right to me.
> 
> That said, I have tested it thoroughly and it handles everything I throw at it, and I have extended it to handle the DSD mode
> without anything unexpected happening. And of course it seems to handle extensive fuzzing, and the recent error was related to
> my DSD change, suggesting that the core it pretty robust. My inclination has been to not "fix" anything that wasn't broken.
> 
> Yes, that channel count stuff is a little difficult to follow. I could experiment with streams that abruptly change channel
> configuration, but again I'm hesitant to change code that's worked so well so long.

I can easily get the decoder to crash even in single-threaded mode. Just
make the demuxer always say the stream is mono, then try decoding your
DSD sample.

Also, I looked at the specification PDF you linked in the previous email
and I see nothing in it that explicitly forbids the channel count from
changing from one "superblock" to the next. It seems to me like you
could just concatenate wavpack files with different channel counts and
get a valid wavpack file.

-- 
Anton Khirnov