Re: [FFmpeg-devel] [PATCH 3/3] avcodec/wavpack: Do not allow the sample format to change between channels
On 6/3/20 4:19 PM, Michael Niedermayer wrote: > Fixes: out of array access > Fixes: > 22692/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_WAVPACK_fuzzer-5678686190960640 > > Found-by: continuous fuzzing process > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer > --- > libavcodec/wavpack.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/libavcodec/wavpack.c b/libavcodec/wavpack.c > index ead57063c8..f77548e5a5 100644 > --- a/libavcodec/wavpack.c > +++ b/libavcodec/wavpack.c > @@ -1129,6 +1129,9 @@ static int wavpack_decode_block(AVCodecContext *avctx, > int block_no, > else > sample_fmt = AV_SAMPLE_FMT_S32P; > > +if (wc->ch_offset && avctx->sample_fmt != sample_fmt) > +return AVERROR_INVALIDDATA; > + > bpp= av_get_bytes_per_sample(sample_fmt); > orig_bpp = ((s->frame_flags & 0x03) + 1) << 3; > multiblock = (s->frame_flags & WV_SINGLE_BLOCK) != WV_SINGLE_BLOCK; Looks reasonable to me and passes my local test suite. Thanks! -David ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avcodec/wavpack: Check rate_x and sample rate for overflow
On 5/2/20 2:08 PM, Michael Niedermayer wrote:
> Fixes: shift exponent 32 is too large for 32-bit type 'int'
> Fixes:
> 21647/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_WAVPACK_fuzzer-5686168323883008
>
> Found-by: continuous fuzzing process
> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer
> ---
> libavcodec/wavpack.c | 13 ++---
> 1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/libavcodec/wavpack.c b/libavcodec/wavpack.c
> index 58ab561a15..ead57063c8 100644
> --- a/libavcodec/wavpack.c
> +++ b/libavcodec/wavpack.c
> @@ -1359,7 +1359,10 @@ static int wavpack_decode_block(AVCodecContext *avctx,
> int block_no,
> bytestream2_skip(, ssize);
> continue;
> }
> -rate_x = 1 << bytestream2_get_byte();
> +rate_x = bytestream2_get_byte();
> +if (rate_x > 30)
> +return AVERROR_INVALIDDATA;
> +rate_x = 1 << rate_x;
> dsd_mode = bytestream2_get_byte();
> if (dsd_mode && dsd_mode != 1 && dsd_mode != 3) {
> av_log(avctx, AV_LOG_ERROR, "Invalid DSD encoding mode:
> %d\n",
> @@ -1498,9 +1501,13 @@ static int wavpack_decode_block(AVCodecContext *avctx,
> int block_no,
> av_log(avctx, AV_LOG_ERROR, "Custom sample rate missing.\n");
> return AVERROR_INVALIDDATA;
> }
> -new_samplerate = sample_rate * rate_x;
> +new_samplerate = sample_rate;
> } else
> -new_samplerate = wv_rates[sr] * rate_x;
> +new_samplerate = wv_rates[sr];
> +
> +if (new_samplerate * (uint64_t)rate_x > INT_MAX)
> +return AVERROR_INVALIDDATA;
> +new_samplerate *= rate_x;
>
> if (multiblock) {
> if (chan)
Looks correct to me. Thanks.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] tests/fate/wavpack: add a lossless DSD file
On 4/18/20 10:32 AM, Carl Eugen Hoyos wrote: > Am Sa., 18. Apr. 2020 um 19:31 Uhr schrieb David Bryant : >> On 4/18/20 9:49 AM, Carl Eugen Hoyos wrote: >>> Am Sa., 18. Apr. 2020 um 17:36 Uhr schrieb David Bryant : >>>> As suggested in another thread, I have created a WavPack DSD test file >>>> that exercises all three of the DSD modes (fast, high, and copy). >>>> >>>> If someone would be kind enough to add this to the fate suite (in the >>>> wavpack/lossless folder) then this patch can go in. >>>> >>>> file: wavpack.com/dsd.wv (MD5: 74b2181f3e9829d9a5b98edd037984ac) >>> I uploaded it but I wonder if 1MB wouldn't be enough? >> It probably would, but because DSD is so inefficient this file is still only >> 5 seconds long, and because I combined all three >> modes, this is really replacing three files. > Just asking... > >> If space or bandwidth is a concern, I would rather pare down a few other >> larger WavPack files. There is one 14 MB file that >> really is a waste of space (and takes a long time to decode also). > This is not possible as we also want to run fate with old versions of FFmpeg. Oh, right, I had not thought of that. I guess it could be done if you generate the same MD5 result with a different input file, but that's probably not very useful. Thanks! - David Bryant > > Carl Eugen > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] tests/fate/wavpack: add a lossless DSD file
On 4/18/20 9:49 AM, Carl Eugen Hoyos wrote: > Am Sa., 18. Apr. 2020 um 17:36 Uhr schrieb David Bryant : >> As suggested in another thread, I have created a WavPack DSD test file that >> exercises all three of the DSD modes (fast, high, and copy). >> >> If someone would be kind enough to add this to the fate suite (in the >> wavpack/lossless folder) then this patch can go in. >> >> file: wavpack.com/dsd.wv (MD5: 74b2181f3e9829d9a5b98edd037984ac) > I uploaded it but I wonder if 1MB wouldn't be enough? It probably would, but because DSD is so inefficient this file is still only 5 seconds long, and because I combined all three modes, this is really replacing three files. If space or bandwidth is a concern, I would rather pare down a few other larger WavPack files. There is one 14 MB file that really is a waste of space (and takes a long time to decode also). Kind regards, David > > Carl Eugen > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] tests/fate/wavpack: add a lossless DSD file
As suggested in another thread, I have created a WavPack DSD test file that exercises all three of the DSD modes (fast, high, and copy). If someone would be kind enough to add this to the fate suite (in the wavpack/lossless folder) then this patch can go in. file: wavpack.com/dsd.wv (MD5: 74b2181f3e9829d9a5b98edd037984ac) decoded MD5 (f32le): a3a88bba95f809025dce01ecb9064091 Thanks! -David Bryant From 8e11e2068f1e763c46903d52da6489cb360b8f8e Mon Sep 17 00:00:00 2001 From: David Bryant Date: Fri, 17 Apr 2020 15:48:05 -0700 Subject: [PATCH] tests/fate/wavpack: add a lossless DSD file Signed-off-by: David Bryant --- tests/fate/wavpack.mak | 3 +++ tests/ref/fate/wavpack-lossless-dsd | 1 + 2 files changed, 4 insertions(+) create mode 100644 tests/ref/fate/wavpack-lossless-dsd diff --git a/tests/fate/wavpack.mak b/tests/fate/wavpack.mak index e3cf4ec..65b54c2 100644 --- a/tests/fate/wavpack.mak +++ b/tests/fate/wavpack.mak @@ -20,6 +20,9 @@ fate-wavpack-lossless-32bit: CMD = md5 -i $(TARGET_SAMPLES)/wavpack/lossless/32b FATE_WAVPACK-$(call DEMDEC, WV, WAVPACK) += fate-wavpack-lossless-float fate-wavpack-lossless-float: CMD = md5 -i $(TARGET_SAMPLES)/wavpack/lossless/32bit_float-partial.wv -f f32le +FATE_WAVPACK-$(call DEMDEC, WV, WAVPACK) += fate-wavpack-lossless-dsd +fate-wavpack-lossless-dsd: CMD = md5 -i $(TARGET_SAMPLES)/wavpack/lossless/dsd.wv -f f32le + # lossy FATE_WAVPACK-$(call DEMDEC, WV, WAVPACK) += fate-wavpack-lossy-8bit diff --git a/tests/ref/fate/wavpack-lossless-dsd b/tests/ref/fate/wavpack-lossless-dsd new file mode 100644 index 000..46ffaf7 --- /dev/null +++ b/tests/ref/fate/wavpack-lossless-dsd @@ -0,0 +1 @@ +a3a88bba95f809025dce01ecb9064091 -- 1.9.1 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 4/4] lavf/wvdec: remove artificial restrictions on stream parameter changes
On 4/5/20 1:32 PM, Anton Khirnov wrote:
> They are not forbidden by the specification.
> ---
> libavformat/wvdec.c | 18 --
> 1 file changed, 18 deletions(-)
>
> diff --git a/libavformat/wvdec.c b/libavformat/wvdec.c
> index b9fc6a59f9..4159bf1253 100644
> --- a/libavformat/wvdec.c
> +++ b/libavformat/wvdec.c
> @@ -208,24 +208,6 @@ static int wv_read_block_header(AVFormatContext *ctx,
> AVIOContext *pb)
> if (!wc->rate)
> wc->rate = rate * rate_x;
>
> -if (flags && bpp != wc->bpp) {
> -av_log(ctx, AV_LOG_ERROR,
> - "Bits per sample differ, this block: %i, header block: %i\n",
> - bpp, wc->bpp);
> -return AVERROR_INVALIDDATA;
> -}
> -if (flags && !wc->multichannel && chan != wc->chan) {
> -av_log(ctx, AV_LOG_ERROR,
> - "Channels differ, this block: %i, header block: %i\n",
> - chan, wc->chan);
> -return AVERROR_INVALIDDATA;
> -}
> -if (flags && rate != -1 && !(flags & WV_DSD) && rate * rate_x !=
> wc->rate) {
> -av_log(ctx, AV_LOG_ERROR,
> - "Sampling rate differ, this block: %i, header block: %i\n",
> - rate * rate_x, wc->rate);
> -return AVERROR_INVALIDDATA;
> -}
> return 0;
> }
>
The specification does not explicitly mention that the data format, the channel
count (and layout), and the sample rate cannot
change inside of a WavPack file, but they can't. The first block of a WavPack
file that contains audio determines the format of
the entire file, and the documentation will be updated shortly to reflect that.
And obviously Konstantin Shishkov was aware of
this, even referring to the first block containing audio as the "header block".
I have no issue with the decoder handling these changes as it might be
receiving a WavPack stream from a container that allows
this, but the native WavPack container doesn't. And I actually don't have any
problem with this specific change going in because
it doesn't actually break anything. However, it should not be possible to
create a WavPack file with format changes such as these.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 03/14] pthread_frame: merge the functionality for normal decoder init and init_thread_copy
On 4/2/20 11:32 PM, Anton Khirnov wrote: > Quoting David Bryant (2020-04-03 07:14:21) >> On 4/2/20 2:13 AM, Anton Khirnov wrote: >>> 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. >> Okay, I reproduced that. However, I'm not sure it's really that serious >> because the demuxer and the decoder use basically the >> same logic to determine that number of channels, so it would be tricky to >> craft a file that caused the demuxer to calculate 1 >> channel and the decoder to reach a different result, which is probably why >> the fuzzers have not been able to do so. > I disagree - this is just as serious as any other crash on invalid data. > > The decoder must not ever crash for arbitrary input data. It cannot > assume that it's always going to be provided valid input or that it's > always used with some specific demuxer. The caller might not be using > libavformat at all, libavcodec should work with any source of data. > > Besides, I can reproduce the crash with no code modification necessary > by muxing your sample into Matroska and then hex-editing the file to say > it has only one channel. Yes, I have reproduced this crash even without Matroska. I was working on a solution but you beat me to it. Thanks! > >> That said, it would not be hard to have the decoder save away the initial >> channel count from avctx, never overwrite it, and >> verify that all superblocks match that count. I don't know off hand whether >> that would break anything though. Is it guaranteed >> that avctx->channels is correct during the call to decode_init(), even if >> it's being demuxed from Matroska, for example? If so, >> why is the WavPack decoder _ever_ writing to the avctx channel count? > The question is what you define as correct. The initial value provided > by the caller is supposed to be the caller's best guess at the correct > (initial) value. It may come from a demuxer, or from the Delphic Oracle. > The decoder doesn't know. > > For some codecs, the relevant parameters are
Re: [FFmpeg-devel] [PATCH 3/4] wavpack: fully support stream parameter changes
On 4/5/20 1:32 PM, Anton Khirnov wrote:
> Fix invalid memory access on DSD streams with changing channel count.
> ---
> libavcodec/wavpack.c | 122 +++
> 1 file changed, 90 insertions(+), 32 deletions(-)
>
> diff --git a/libavcodec/wavpack.c b/libavcodec/wavpack.c
> index b27262b94e..9cc4104dd0 100644
> --- a/libavcodec/wavpack.c
> +++ b/libavcodec/wavpack.c
> @@ -20,6 +20,7 @@
> * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
> USA
> */
>
> +#include "libavutil/buffer.h"
> #include "libavutil/channel_layout.h"
>
> #define BITSTREAM_READER_LE
> @@ -109,7 +110,10 @@ typedef struct WavpackContext {
> AVFrame *frame;
> ThreadFrame curr_frame, prev_frame;
> Modulation modulation;
> +
> +AVBufferRef *dsd_ref;
> DSDContext *dsdctx;
> +int dsd_channels;
> } WavpackContext;
>
> #define LEVEL_DECAY(a) (((a) + 0x80) >> 8)
> @@ -978,6 +982,32 @@ static av_cold int wv_alloc_frame_context(WavpackContext
> *c)
> return 0;
> }
>
> +static int wv_dsd_reset(WavpackContext *s, int channels)
> +{
> +int i;
> +
> +s->dsdctx = NULL;
> +s->dsd_channels = 0;
> +av_buffer_unref(>dsd_ref);
> +
> +if (!channels)
> +return 0;
> +
> +if (channels > INT_MAX / sizeof(*s->dsdctx))
> +return AVERROR(EINVAL);
> +
> +s->dsd_ref = av_buffer_allocz(channels * sizeof(*s->dsdctx));
> +if (!s->dsd_ref)
> +return AVERROR(ENOMEM);
> +s->dsdctx = (DSDContext*)s->dsd_ref->data;
> +s->dsd_channels = channels;
> +
> +for (i = 0; i < channels; i++)
> +memset(s->dsdctx[i].buf, 0x69, sizeof(s->dsdctx[i].buf));
> +
> +return 0;
> +}
> +
> #if HAVE_THREADS
> static int init_thread_copy(AVCodecContext *avctx)
> {
> @@ -1008,6 +1038,17 @@ static int update_thread_context(AVCodecContext *dst,
> const AVCodecContext *src)
> return ret;
> }
>
> +av_buffer_unref(>dsd_ref);
> +fdst->dsdctx = NULL;
> +fdst->dsd_channels = 0;
> +if (fsrc->dsd_ref) {
> +fdst->dsd_ref = av_buffer_ref(fsrc->dsd_ref);
> +if (!fdst->dsd_ref)
> +return AVERROR(ENOMEM);
> +fdst->dsdctx = (DSDContext*)fdst->dsd_ref->data;
> +fdst->dsd_channels = fsrc->dsd_channels;
> +}
> +
> return 0;
> }
> #endif
> @@ -1025,15 +1066,9 @@ static av_cold int wavpack_decode_init(AVCodecContext
> *avctx)
> s->curr_frame.f = av_frame_alloc();
> s->prev_frame.f = av_frame_alloc();
>
> -// the DSD to PCM context is shared (and used serially) between all
> decoding threads
> -s->dsdctx = av_calloc(avctx->channels, sizeof(DSDContext));
> -
> -if (!s->curr_frame.f || !s->prev_frame.f || !s->dsdctx)
> +if (!s->curr_frame.f || !s->prev_frame.f)
> return AVERROR(ENOMEM);
>
> -for (int i = 0; i < avctx->channels; i++)
> -memset(s->dsdctx[i].buf, 0x69, sizeof(s->dsdctx[i].buf));
> -
> ff_init_dsd_data();
>
> return 0;
> @@ -1053,8 +1088,7 @@ static av_cold int wavpack_decode_end(AVCodecContext
> *avctx)
> ff_thread_release_buffer(avctx, >prev_frame);
> av_frame_free(>prev_frame.f);
>
> -if (!avctx->internal->is_copy)
> -av_freep(>dsdctx);
> +av_buffer_unref(>dsd_ref);
>
> return 0;
> }
> @@ -1065,6 +1099,7 @@ static int wavpack_decode_block(AVCodecContext *avctx,
> int block_no,
> WavpackContext *wc = avctx->priv_data;
> WavpackFrameContext *s;
> GetByteContext gb;
> +enum AVSampleFormat sample_fmt;
> void *samples_l = NULL, *samples_r = NULL;
> int ret;
> int got_terms = 0, got_weights = 0, got_samples = 0,
> @@ -1102,7 +1137,15 @@ static int wavpack_decode_block(AVCodecContext *avctx,
> int block_no,
> return AVERROR_INVALIDDATA;
> }
> s->frame_flags = bytestream2_get_le32();
> -bpp= av_get_bytes_per_sample(avctx->sample_fmt);
> +
> +if (s->frame_flags & (WV_FLOAT_DATA | WV_DSD_DATA))
> +sample_fmt = AV_SAMPLE_FMT_FLTP;
> +else if ((s->frame_flags & 0x03) <= 1)
> +sample_fmt = AV_SAMPLE_FMT_S16P;
> +else
> +sample_fmt = AV_SAMPLE_FMT_S32P;
> +
> +bpp= av_get_bytes_per_sample(sample_fmt);
> orig_bpp = ((s->frame_flags & 0x03) + 1) << 3;
> multiblock = (s->frame_flags & WV_SINGLE_BLOCK) != WV_SINGLE_BLOCK;
>
> @@ -1436,11 +1479,11 @@ static int wavpack_decode_block(AVCodecContext
> *avctx, int block_no,
> av_log(avctx, AV_LOG_ERROR, "Hybrid config not found\n");
> return AVERROR_INVALIDDATA;
> }
> -if (!got_float && avctx->sample_fmt == AV_SAMPLE_FMT_FLTP) {
> +if (!got_float && sample_fmt == AV_SAMPLE_FMT_FLTP) {
> av_log(avctx, AV_LOG_ERROR, "Float information not found\n");
> return AVERROR_INVALIDDATA;
> }
> -if (s->got_extra_bits && avctx->sample_fmt != AV_SAMPLE_FMT_FLTP) {
Re: [FFmpeg-devel] [PATCH 03/14] pthread_frame: merge the functionality for normal decoder init and init_thread_copy
On 4/2/20 2:13 AM, Anton Khirnov wrote: > 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. Okay, I reproduced that. However, I'm not sure it's really that serious because the demuxer and the decoder use basically the same logic to determine that number of channels, so it would be tricky to craft a file that caused the demuxer to calculate 1 channel and the decoder to reach a different result, which is probably why the fuzzers have not been able to do so. That said, it would not be hard to have the decoder save away the initial channel count from avctx, never overwrite it, and verify that all superblocks match that count. I don't know off hand whether that would break anything though. Is it guaranteed that avctx->channels is correct during the call to decode_init(), even if it's being demuxed from Matroska, for example? If so, why is the WavPack decoder _ever_ writing to the avctx channel count? > > 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. > You're right, I could not find that being explicitly forbidden. Nevertheless, it _is_ forbidden for any stream parameters to change (e.g. data format, sampling rate, channel count or mapping) and both libwavpack and the FFmpeg demuxer enforce this (see libavformat/wvdec.c starting at line 211), or at least gracefully fail without crashing. The reason it's never mentioned is probably just because it never occurred to me. None of the source file formats for WavPack (e.g., wav, caf, dsdiff) allow format changes and neither do any of the other lossless compressors I am aware of. I can't imagine how that would work, and extensive fuzzing has ensured that pathological files like you suggest are unlikely to be the source of exploits. Kind regards, David ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 03/14] pthread_frame: merge the functionality for normal decoder init and init_thread_copy
On 3/31/20 2:47 AM, Anton Khirnov wrote: > Quoting David Bryant (2020-03-28 21:22:40) >> On 3/28/20 6:23 AM, Anton Khirnov wrote: >>> Quoting David Bryant (2020-03-27 23:51:19) >>>> On 3/27/20 5:57 AM, Anton Khirnov wrote: >>>>> The current design, where >>>>> - proper init is called for the first per-thread context >>>>> - first thread's private data is copied into private data for all the >>>>> other threads >>>>> - a "fixup" function is called for all the other threads to e.g. >>>>> allocate dynamically allocated data >>>>> is very fragile and hard to follow, so it is abandoned. Instead, the >>>>> same init function is used to init each per-thread context. Where >>>>> necessary, AVCodecInternal.is_copy can be used to differentiate between >>>>> the first thread and the other ones (e.g. for decoding the extradata >>>>> just once). >>>>> --- >>>> I'm not sure I fully understand this change. You mention that >>>> AVCodecInternal.is_copy can be used where different treatment >>>> might be necessary for subsequent threads, and I see that done in a couple >>>> places, but in most cases you have simply deleted the >>>> init_thread_copy() function even when it's not at all obvious (to me >>>> anyway) that that won't break the codec. >>> In most cases, just deleting init_thread_copy() is the right thing to >>> do. E.g. all decoders that do not implement update_thread_context() have >>> to be intra-only, so every frame thread is effectively a completely >>> standalone decoder. And in most of the more complex decoders (like h264) >>> the important parameters are dynamically changeable during decoding, so >>> not that much is done in decoder init beyond allocating some stuff that >>> does not depend on the bistream properties. >>> >>> My intent is for each frame-thread worker to be treated inasmuch as >>> possible as a standalone decoder, and where it has to share data with >>> other threads to make this sharing explicit (rather than implicit as is >>> the case now). >> Yes, this makes sense. The confusing part is when the decode_init() function >> looks completely different than the >> init_thread_copy() function. This is often because the decode_init() >> function is generating things (tables, etc.) from scratch >> and the init_thread_copy() is just copying the necessary things from the >> original. In cases where the original generation might >> be time consuming this might make sense, but usually it's probably just >> making the code more complex and difficult to follow >> (which I believe was your original point). > Yes, precisely. I believe for obscure codecs it's better to waste a > little memory when doing threaded decoding, in return for simpler code. > And if people really care about being as memory-efficient as possible > then doing explicit refcounting on allocated buffers is still possible. > >> One possible interim solution for complex cases that break would be to leave >> the init_thread_copy() function there, but instead >> of having it in the AVCodec struct and called from outside (which is no >> longer possible), it simply gets called first thing from >> decode_init() if AVCodecInternal.is_copy is set. That way the architecture >> is cleaned up now, and the codec won't break and can >> be cleaned up when time permits. Just a thought. > I don't really see much point in any interim solutions, since no codecs > should break. You found a problem with wavpack, that's going to be fixed > in the next iteration of this patch. > Speaking of which, ideally we should have a FATE test for DSD files so > that I would have found the failure myself when running test. I have created a WavPack DSD test file that exercises all three of the DSD modes (fast, high, and copy). I have also verified that it would have caught this change, although I had to specify fate multithreading once I discovered it wasn't the default. If someone can add this to the fate suite (in the wavpack/lossless folder) then I can supply a patch to add the test: file: http://www.wavpack.com/dsd.wv (MD5: 74b2181f3e9829d9a5b98edd037984ac) decoded MD5 (f32le): a3a88bba95f809025dce01ecb9064091 >>>> Specifically this will break WavPack because now it will be allocating a >>>> new dsdctx for every thread instead of sharing one >>>> between all threads. I am happy to fix and test this case once the patch >>>> goes in, but is the intent of this patch that the >
Re: [FFmpeg-devel] [PATCH 1/3] avformat/matroskadec: Add a workaround for missing WavPack extradata
On 4/1/20 12:53 AM, Andreas Rheinhardt wrote:
> Andreas Rheinhardt:
>> mkvmerge versions 6.2 to 40.0 had a bug that made it not propagate the
>> WavPack extradata (containing the WavPack version) during remuxing from
>> a Matroska file; currently our demuxer would treat every WavPack block
>> encountered as invalid data (unless the WavPack stream is to be
>> discarded (i.e. the streams discard is >= AVDISCARD_ALL)) and try to
>> resync to the next level 1 element.
>>
>> Luckily, the WavPack version is currently not really important; so we
>> fix this problem by assuming a version. David Bryant, the creator of
>> WavPack, recommended using version 0x410 (the most recent version) for
>> this. And this is what this commit does.
>>
>> A FATE-test for this has been added.
>>
>> Signed-off-by: Andreas Rheinhardt
>> ---
>> James has already uploaded the sample. Thanks for this.
>>
>> libavformat/matroskadec.c| 11 ++-
>> tests/fate/matroska.mak | 5 +
>> tests/ref/fate/matroska-wavpack-missing-codecprivate | 9 +
>> 3 files changed, 24 insertions(+), 1 deletion(-)
>> create mode 100644 tests/ref/fate/matroska-wavpack-missing-codecprivate
>>
>> diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
>> index 4d7fdab99f..5b55606b98 100644
>> --- a/libavformat/matroskadec.c
>> +++ b/libavformat/matroskadec.c
>> @@ -2613,6 +2613,14 @@ static int matroska_parse_tracks(AVFormatContext *s)
>> ret = matroska_parse_flac(s, track, _offset);
>> if (ret < 0)
>> return ret;
>> +} else if (codec_id == AV_CODEC_ID_WAVPACK &&
>> track->codec_priv.size < 2) {
>> +av_log(matroska->ctx, AV_LOG_INFO, "Assuming WavPack version
>> 4.10 "
>> + "in absence of valid CodecPrivate.\n");
>> +extradata_size = 2;
>> +extradata = av_mallocz(2 + AV_INPUT_BUFFER_PADDING_SIZE);
>> +if (!extradata)
>> +return AVERROR(ENOMEM);
>> +AV_WL16(extradata, 0x410);
>> } else if (codec_id == AV_CODEC_ID_PRORES && track->codec_priv.size
>> == 4) {
>> fourcc = AV_RL32(track->codec_priv.data);
>> } else if (codec_id == AV_CODEC_ID_VP9 && track->codec_priv.size) {
>> @@ -3165,9 +3173,10 @@ static int matroska_parse_wavpack(MatroskaTrack
>> *track, uint8_t *src,
>> uint16_t ver;
>> int ret, offset = 0;
>>
>> -if (srclen < 12 || track->stream->codecpar->extradata_size < 2)
>> +if (srclen < 12)
>> return AVERROR_INVALIDDATA;
>>
>> +av_assert1(track->stream->codecpar->extradata_size >= 2);
>> ver = AV_RL16(track->stream->codecpar->extradata);
>>
>> samples = AV_RL32(src);
>> diff --git a/tests/fate/matroska.mak b/tests/fate/matroska.mak
>> index b9ed7322fd..93b5bff89a 100644
>> --- a/tests/fate/matroska.mak
>> +++ b/tests/fate/matroska.mak
>> @@ -17,6 +17,11 @@ fate-matroska-remux: REF =
>> 49a60ef91cf7302ab7276f9373f8a429
>> FATE_MATROSKA-$(call ALLYES, MATROSKA_DEMUXER VORBIS_PARSER) +=
>> fate-matroska-xiph-lacing
>> fate-matroska-xiph-lacing: CMD = framecrc -i
>> $(TARGET_SAMPLES)/mkv/xiph_lacing.mka -c:a copy
>>
>> +# This tests that the Matroska demuxer correctly demuxes WavPack
>> +# without CodecPrivate; it also tests zlib compressed WavPack.
>> +FATE_MATROSKA-$(call ALLYES, MATROSKA_DEMUXER ZLIB) +=
>> fate-matroska-wavpack-missing-codecprivate
>> +fate-matroska-wavpack-missing-codecprivate: CMD = framecrc -i
>> $(TARGET_SAMPLES)/mkv/wavpack_missing_codecprivate.mka -c copy
>> +
>> # This tests that the matroska demuxer supports decompressing
>> # zlib compressed tracks (both the CodecPrivate as well as the actual
>> frames).
>> FATE_MATROSKA-$(call ALLYES, MATROSKA_DEMUXER ZLIB) +=
>> fate-matroska-zlib-decompression
>> diff --git a/tests/ref/fate/matroska-wavpack-missing-codecprivate
>> b/tests/ref/fate/matroska-wavpack-missing-codecprivate
>> new file mode 100644
>> index 00..4645a86ff6
>> --- /dev/null
>> +++ b/tests/ref/fate/matroska-wavpack-missing-codecprivate
>> @@ -0,0 +1,9 @@
>> +#extradata 0:2, 0x00240014
>> +#tb 0: 11337/5
>> +#media_type 0: audio
>> +#codec_id 0: wavpack
>> +#sample_rate 0: 44100
>> +#channel_layout 0: 3
>> +#channel_layout_name 0: stereo
>> +0, 0, 0,22051,14778, 0x02819286
>> +0, 22051, 22051,22052,14756, 0x21976243
>>
> Any comments on this patchset? If not, I'd like to apply it tomorrow.
This looks good to me. Thanks!
-David
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 03/14] pthread_frame: merge the functionality for normal decoder init and init_thread_copy
On 3/28/20 6:23 AM, Anton Khirnov wrote: > Quoting David Bryant (2020-03-27 23:51:19) >> On 3/27/20 5:57 AM, Anton Khirnov wrote: >>> The current design, where >>> - proper init is called for the first per-thread context >>> - first thread's private data is copied into private data for all the >>> other threads >>> - a "fixup" function is called for all the other threads to e.g. >>> allocate dynamically allocated data >>> is very fragile and hard to follow, so it is abandoned. Instead, the >>> same init function is used to init each per-thread context. Where >>> necessary, AVCodecInternal.is_copy can be used to differentiate between >>> the first thread and the other ones (e.g. for decoding the extradata >>> just once). >>> --- >> I'm not sure I fully understand this change. You mention that >> AVCodecInternal.is_copy can be used where different treatment >> might be necessary for subsequent threads, and I see that done in a couple >> places, but in most cases you have simply deleted the >> init_thread_copy() function even when it's not at all obvious (to me anyway) >> that that won't break the codec. > In most cases, just deleting init_thread_copy() is the right thing to > do. E.g. all decoders that do not implement update_thread_context() have > to be intra-only, so every frame thread is effectively a completely > standalone decoder. And in most of the more complex decoders (like h264) > the important parameters are dynamically changeable during decoding, so > not that much is done in decoder init beyond allocating some stuff that > does not depend on the bistream properties. > > My intent is for each frame-thread worker to be treated inasmuch as > possible as a standalone decoder, and where it has to share data with > other threads to make this sharing explicit (rather than implicit as is > the case now). Yes, this makes sense. The confusing part is when the decode_init() function looks completely different than the init_thread_copy() function. This is often because the decode_init() function is generating things (tables, etc.) from scratch and the init_thread_copy() is just copying the necessary things from the original. In cases where the original generation might be time consuming this might make sense, but usually it's probably just making the code more complex and difficult to follow (which I believe was your original point). One possible interim solution for complex cases that break would be to leave the init_thread_copy() function there, but instead of having it in the AVCodec struct and called from outside (which is no longer possible), it simply gets called first thing from decode_init() if AVCodecInternal.is_copy is set. That way the architecture is cleaned up now, and the codec won't break and can be cleaned up when time permits. Just a thought. > >> Specifically this will break WavPack because now it will be allocating a new >> dsdctx for every thread instead of sharing one >> between all threads. I am happy to fix and test this case once the patch >> goes in, but is the intent of this patch that the >> respective authors will find and fix all the breakages? Or did I just happen >> to catch the one case you missed? > I certainly intended to convert the decoders correctly myself, > apparently I didn't pay enough attention to the recent wavpack changes. > Hopefully the other conversions are correct, but I will go through the > changes again to check. > > 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 you are curious, the WavPack format is described in detail here: https://github.com/dbry/WavPack/blob/master/doc/WavPack5FileFormat.pdf Thanks! -David ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 03/14] pthread_frame: merge the functionality for normal decoder init and init_thread_copy
On 3/27/20 5:57 AM, Anton Khirnov wrote: > The current design, where > - proper init is called for the first per-thread context > - first thread's private data is copied into private data for all the > other threads > - a "fixup" function is called for all the other threads to e.g. > allocate dynamically allocated data > is very fragile and hard to follow, so it is abandoned. Instead, the > same init function is used to init each per-thread context. Where > necessary, AVCodecInternal.is_copy can be used to differentiate between > the first thread and the other ones (e.g. for decoding the extradata > just once). > --- I'm not sure I fully understand this change. You mention that AVCodecInternal.is_copy can be used where different treatment might be necessary for subsequent threads, and I see that done in a couple places, but in most cases you have simply deleted the init_thread_copy() function even when it's not at all obvious (to me anyway) that that won't break the codec. Specifically this will break WavPack because now it will be allocating a new dsdctx for every thread instead of sharing one between all threads. I am happy to fix and test this case once the patch goes in, but is the intent of this patch that the respective authors will find and fix all the breakages? Or did I just happen to catch the one case you missed? Cheers! -David ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/2] avcodec/wavpack: Prevent frame format from being wrong
On 3/23/20 9:49 AM, Anton Khirnov wrote: > Quoting Michael Niedermayer (2020-03-20 21:50:18) >> On Fri, Mar 20, 2020 at 10:18:49AM +0100, Anton Khirnov wrote: >>> Quoting Michael Niedermayer (2020-03-20 01:03:36) Fixes: out of array access Fixes: 21193/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_WAVPACK_fuzzer-5125168956702720 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer --- libavcodec/wavpack.c | 1 + 1 file changed, 1 insertion(+) diff --git a/libavcodec/wavpack.c b/libavcodec/wavpack.c index b27262b94e..e9c870e41e 100644 --- a/libavcodec/wavpack.c +++ b/libavcodec/wavpack.c @@ -1488,6 +1488,7 @@ static int wavpack_decode_block(AVCodecContext *avctx, int block_no, /* get output buffer */ wc->curr_frame.f->nb_samples = s->samples; +wc->curr_frame.f->format = avctx->sample_fmt; >>> How does this have any effect? curr_frame.f should now be clean and get >>> initialized from avctx->sample_fmt. >> IIRC >> The format changes between frames, so the struct is still set to the one >> from the previous frame and that overrides the use of the avctx value >> >> setting it to NONE (here or somewhere else) should work too. > ff_thread_release_buffer() is called on that frame immediately before, > which should reset it to defaults (setting format to FMT_NONE). > I don't think the format should change between frames, so I don't understand how the format is getting set to a wacky value. Would it be possible for me the get the triggering test case and try this myself? I searched and couldn't find it, so I assume it's not public yet. I assume that just decoding the file should trigger the assertion, right? Thanks! ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 2/2] avcodec/wavpack: Prevent frame format from being wrong
On 3/20/20 1:54 AM, Paul B Mahol wrote: > lgtm Looks good to me too, sorry about that! > > On 3/20/20, Michael Niedermayer wrote: >> Fixes: out of array access >> Fixes: >> 21193/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_WAVPACK_fuzzer-5125168956702720 >> >> Found-by: continuous fuzzing process >> https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >> Signed-off-by: Michael Niedermayer >> --- >> libavcodec/wavpack.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/libavcodec/wavpack.c b/libavcodec/wavpack.c >> index b27262b94e..e9c870e41e 100644 >> --- a/libavcodec/wavpack.c >> +++ b/libavcodec/wavpack.c >> @@ -1488,6 +1488,7 @@ static int wavpack_decode_block(AVCodecContext *avctx, >> int block_no, >> >> /* get output buffer */ >> wc->curr_frame.f->nb_samples = s->samples; >> +wc->curr_frame.f->format = avctx->sample_fmt; >> if ((ret = ff_thread_get_buffer(avctx, >curr_frame, >> AV_GET_BUFFER_FLAG_REF)) < 0) >> return ret; >> >> -- >> 2.17.1 >> >> ___ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> >> To unsubscribe, visit link above, or email >> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] avcodec/wavpack: add DSD support
Hi,
My patch from last July to add WavPack DSD support was rejected because I
created another codec ID for WavPack DSD audio and
duplicated some existing WavPack code.
This patch simply adds the DSD support to the existing wavpack.c file. As
suggested, I now use ff_thread_report/await_progress()
to keep the final dsd2pcm operation serialized between frame threads, and I
also put in slice threading for that because the
channels can be independently converted.
This has been tested extensively on both X64 and ARM and appears quite stable.
I have also tested it on various sanitizers and
valgrind. Also, essentially the same DSD decoder has been extensively fuzzed on
OSS-Fuzz because WavPack is also part of that
project now.
Kind regards,
David
From 5d033a4628bdaa13694c5df88b185e7b099bfea5 Mon Sep 17 00:00:00 2001
From: David Bryant
Date: Mon, 9 Mar 2020 15:23:53 -0700
Subject: [PATCH] avcodec/wavpack: add support for DSD files
Add support for WavPack DSD files to the existing WavPack decoder using
avcodec/dsd to perform the 8:1 decimation to 32-bit float samples. We must
serialize the dsd2pcm operation (cross-boundary filtering) but would like
to use frame-level multithreading for the CPU-intensive DSD decompression,
and this is accomplished with ff_thread_report/await_progress(). Because
the dsd2pcm operation is independent across channels we use slice-based
multithreading for that part.
Also a few things were removed from the existing WavPack decoder that
weren't being used (primarily the SavedContext stuff) and the WavPack
demuxer was enhanced to correctly determine the sampling rate of DSD
files (and of course to no longer reject them).
Signed-off-by: David Bryant
---
libavcodec/Makefile | 2 +-
libavcodec/wavpack.c | 692 ---
libavcodec/wavpack.h | 2 +
libavformat/wvdec.c | 28 ++-
4 files changed, 616 insertions(+), 108 deletions(-)
diff --git a/libavcodec/Makefile b/libavcodec/Makefile
index 0fd374f..a3326a4 100644
--- a/libavcodec/Makefile
+++ b/libavcodec/Makefile
@@ -693,7 +693,7 @@ OBJS-$(CONFIG_VP9_QSV_ENCODER) += qsvenc_vp9.o
OBJS-$(CONFIG_VPLAYER_DECODER) += textdec.o ass.o
OBJS-$(CONFIG_VP9_V4L2M2M_DECODER) += v4l2_m2m_dec.o
OBJS-$(CONFIG_VQA_DECODER) += vqavideo.o
-OBJS-$(CONFIG_WAVPACK_DECODER) += wavpack.o
+OBJS-$(CONFIG_WAVPACK_DECODER) += wavpack.o dsd.o
OBJS-$(CONFIG_WAVPACK_ENCODER) += wavpackenc.o
OBJS-$(CONFIG_WCMV_DECODER)+= wcmv.o
OBJS-$(CONFIG_WEBP_DECODER)+= webp.o
diff --git a/libavcodec/wavpack.c b/libavcodec/wavpack.c
index edc0f79..bb36f43 100644
--- a/libavcodec/wavpack.c
+++ b/libavcodec/wavpack.c
@@ -1,6 +1,7 @@
/*
* WavPack lossless audio decoder
* Copyright (c) 2006,2011 Konstantin Shishkov
+ * Copyright (c) 2020 David Bryant
*
* This file is part of FFmpeg.
*
@@ -29,18 +30,37 @@
#include "thread.h"
#include "unary.h"
#include "wavpack.h"
+#include "dsd.h"
/**
* @file
* WavPack lossless audio decoder
*/
-typedef struct SavedContext {
-int offset;
-int size;
-int bits_used;
-uint32_t crc;
-} SavedContext;
+#define DSD_BYTE_READY(low,high) (!(((low) ^ (high)) & 0xff00))
+
+#define PTABLE_BITS 8
+#define PTABLE_BINS (1<> 8)
@@ -365,12 +394,6 @@ static float wv_get_value_float(WavpackFrameContext *s,
uint32_t *crc, int S)
return value.f;
}
-static void wv_reset_saved_context(WavpackFrameContext *s)
-{
-s->pos= 0;
-s->sc.crc = s->extra_sc.crc = 0x;
-}
-
static inline int wv_check_crc(WavpackFrameContext *s, uint32_t crc,
uint32_t crc_extra_bits)
{
@@ -386,15 +409,372 @@ static inline int wv_check_crc(WavpackFrameContext *s,
uint32_t crc,
return 0;
}
+static void init_ptable(int *table, int rate_i, int rate_s)
+{
+int value = 0x808000, rate = rate_i << 8;
+
+for (int c = (rate + 128) >> 8; c--;)
+value += (DOWN - value) >> DECAY;
+
+for (int i = 0; i < PTABLE_BINS/2; i++) {
+table[i] = value;
+table[PTABLE_BINS-1-i] = 0x100 - value;
+
+if (value > 0x01) {
+rate += (rate * rate_s + 128) >> 8;
+
+for (int c = (rate + 64) >> 7; c--;)
+value += (DOWN - value) >> DECAY;
+}
+}
+}
+
+typedef struct {
+int32_t value, fltr0, fltr1, fltr2, fltr3, fltr4, fltr5, fltr6, factor;
+unsigned int byte;
+} DSDfilters;
+
+static int wv_unpack_dsd_high(WavpackFrameContext *s, uint8_t *dst_left,
uint8_t *dst_right)
+{
+uint32_t checksum = 0x;
+uint8_t *dst_l = dst_left, *dst_r = dst_right;
+int total_samples = s->samples, stereo = dst_r ? 1 : 0;
+DSDfilters filters[2], *sp = filters;
+int rate_i, rate_s;
+uint32_t low, high, value;
+
+if (bytestream2_get_bytes_left(>gb
Re: [FFmpeg-devel] [PATCH] DSD and DST speed improvements
On 7/31/19 11:49 PM, Paul B Mahol wrote: > Hi, > > patches attached. > > This time DSD decoding is approx %50 faster on old Celeron N3050 CPUs and 2 > threads. I see about 42% speedup on Core Duo E8400 (36x to 52x) for a 5-channel file. On the same file encoded in DST, the improvement almost disappears (2.47x to 2.54x) because the DST part takes about 15x more CPU than the dsd2pcm part; would be extremely valuable to multi-thread that instead and leave the dsd2pcm alone. And this breaks my WavPack DSD decoder, but I guess I can't complain about that yet. :) > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH] avcodec/dsddec: add slice threading support
On 7/30/19 1:43 AM, Carl Eugen Hoyos wrote: > Am So., 28. Juli 2019 um 23:35 Uhr schrieb Paul B Mahol : >> Hi, >> >> patch attached. > As just posted on irc: > On an 8-core Intel cpu, this makes decoding slower by a factor two, > heating the cpu > ten times as much, on a multi-core powerpc, decoding is nearly two > times slower, and > heats nearly eight times as much. I can verify the greatly increased CPU load on a dual core system also. I also verified that the outputted audio data is correct (identical to unmodified code) and that there are no more samples being processed, indicating it's probably just threading overhead. The dsd2pcm conversion is pretty optimized and I suspect that's just way too fast to be worth multi-threading. As I suggested earlier, it's the DST decompression that is worth looking into for threading. -David > > Carl Eugen > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] avcodec: add a WavPack DSD decoder
On 7/28/19 10:27 AM, Paul B Mahol wrote: > On Sun, Jul 28, 2019 at 6:02 AM David Bryant wrote: > >> On 7/24/19 12:26 AM, Paul B Mahol wrote: >>> On 7/23/19, David Bryant wrote: >>>> On 7/23/19 12:47 AM, Paul B Mahol wrote: >>>>> On 7/23/19, David Bryant wrote: >>>>>> On 7/21/19 11:23 PM, Paul B Mahol wrote: >>>>>>> On 7/22/19, David Bryant wrote: >>>>>>>> Hi, >>>>>>>> >>>>>>>> As I promised late last year, here is a patch to add a WavPack DSD >>>>>>>> decoder. >>>>>>>> >>>>>>>> Thanks! >>>>>>>> >>>>>>>> -David Bryant >>>>>>>> >>>>>>>> >>>>>>> Please correct me if I'm wrong, but why this uses new codec id? >>>>>>> Apparently is also copies some logic from other files. >>>>>> Yes, I created a new codec ID for WavPack DSD. It would be possible to >>>>>> just >>>>>> piggyback on the existing WavPack codec by silently >>>>>> decimating the DSD to PCM, but that seemed weird and wrong to me. For >> one >>>>>> thing, the user would have no idea that the file was >>>>>> actually DSD and not high sample-rate PCM. >>>>>> >>>>>> Also, since regular WavPack has threading enabled but WavPack DSD >> can't >>>>>> (because of the dsd2pcm conversion) I would have to turn >>>>>> off threading for all WavPack (unless there's some way of making that >>>>>> conditional at runtime). It would also mean that regular >>>>>> WavPack would be dependent on the dsd2pcm component even if DSD was >> not >>>>>> required (not everyone needs DSD). And of course I was >>>>>> looking closely at the only other DSD codec in FFmpeg (DST) which has >> its >>>>>> own codec ID. >>>>>> >>>>>> Because regular WavPack PCM and DSD share the same block format and >>>>>> metadata >>>>>> structure, there is a little code that is shared >>>>>> between the two codecs (although they are no longer identical because >> of >>>>>> the >>>>>> threading difference). Is this a problem? I could >>>>>> combine the two codecs into one file and sprinkle in a few >> conditionals, >>>>>> but >>>>>> I don't think it would be as clean or clear (but >>>>>> might save a little duplicate code). >>>>>> >>>>>> That's my thinking, but obviously I am not as familiar with the FFmpeg >>>>>> philosophy as you guys. >>>>> Looking carefully at dsd2pcm code in your patch, it appears it would >>>>> work only with 1 or 2 channels. >>>>> Is this correct? >>>> Individual WavPack blocks can only be 1 or 2 channels. Multichannel >> files >>>> contains sequences of blocks which ends up creating >>>> multiple frame contexts. This was originally implemented for PCM WavPack >>>> years ago and so it "just works" for DSD WavPack >>>> because I didn't touch any of that. >>>> >>>> I have tested this extensively with multichannel DSD files and it works >>>> great, including seeking. >>> Than I fail to see why it could not work for multi-threading too. >> Really? Because I just looked back at the FFmpeg devel archives (May 2016) >> and you supplied the patch for the DST codec, the >> only other DSD compression codec in existence before my recent >> development. And dstdec.c does not enable multi-threading >> because, of course, you get clicks. This seems like an unlikely oversight >> since DST is a well-documented CPU hog and would >> benefit greatly from multi-threading. >> > You are right, it can not use frame-multi-threading. But can use > slice-multi-threading as each channel is independent. > Thanks for pointing this for DST, I will implement this slice-threading > soon, unless someone beat me first. My thought was that the CPU-hungry DSD decoding would be multi-threaded, but each thread would have to wait when it got to the dsd2pcm step to make sure the previous had finished, and then it would copy the dsd2pcm-specific context from the previous thread. So there would be dsd2pcm contexts allocated for each thread, but the actual content would be copied from one to the next. Or you could just always use the dsd2pcm contexts from the "first" thread, if it was possible to access that. I would need to look at it a lot more to be sure. > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] avcodec: add a WavPack DSD decoder
On 7/24/19 12:26 AM, Paul B Mahol wrote: > On 7/23/19, David Bryant wrote: >> On 7/23/19 12:47 AM, Paul B Mahol wrote: >>> On 7/23/19, David Bryant wrote: >>>> On 7/21/19 11:23 PM, Paul B Mahol wrote: >>>>> On 7/22/19, David Bryant wrote: >>>>>> Hi, >>>>>> >>>>>> As I promised late last year, here is a patch to add a WavPack DSD >>>>>> decoder. >>>>>> >>>>>> Thanks! >>>>>> >>>>>> -David Bryant >>>>>> >>>>>> >>>>> Please correct me if I'm wrong, but why this uses new codec id? >>>>> Apparently is also copies some logic from other files. >>>> Yes, I created a new codec ID for WavPack DSD. It would be possible to >>>> just >>>> piggyback on the existing WavPack codec by silently >>>> decimating the DSD to PCM, but that seemed weird and wrong to me. For one >>>> thing, the user would have no idea that the file was >>>> actually DSD and not high sample-rate PCM. >>>> >>>> Also, since regular WavPack has threading enabled but WavPack DSD can't >>>> (because of the dsd2pcm conversion) I would have to turn >>>> off threading for all WavPack (unless there's some way of making that >>>> conditional at runtime). It would also mean that regular >>>> WavPack would be dependent on the dsd2pcm component even if DSD was not >>>> required (not everyone needs DSD). And of course I was >>>> looking closely at the only other DSD codec in FFmpeg (DST) which has its >>>> own codec ID. >>>> >>>> Because regular WavPack PCM and DSD share the same block format and >>>> metadata >>>> structure, there is a little code that is shared >>>> between the two codecs (although they are no longer identical because of >>>> the >>>> threading difference). Is this a problem? I could >>>> combine the two codecs into one file and sprinkle in a few conditionals, >>>> but >>>> I don't think it would be as clean or clear (but >>>> might save a little duplicate code). >>>> >>>> That's my thinking, but obviously I am not as familiar with the FFmpeg >>>> philosophy as you guys. >>> Looking carefully at dsd2pcm code in your patch, it appears it would >>> work only with 1 or 2 channels. >>> Is this correct? >> Individual WavPack blocks can only be 1 or 2 channels. Multichannel files >> contains sequences of blocks which ends up creating >> multiple frame contexts. This was originally implemented for PCM WavPack >> years ago and so it "just works" for DSD WavPack >> because I didn't touch any of that. >> >> I have tested this extensively with multichannel DSD files and it works >> great, including seeking. > Than I fail to see why it could not work for multi-threading too. Really? Because I just looked back at the FFmpeg devel archives (May 2016) and you supplied the patch for the DST codec, the only other DSD compression codec in existence before my recent development. And dstdec.c does not enable multi-threading because, of course, you get clicks. This seems like an unlikely oversight since DST is a well-documented CPU hog and would benefit greatly from multi-threading. > >> >>> About multi-threading, as dsd2pcm can not be made multi-threaded then >>> just do locking when doing dsd2pcm to make it single threaded and >>> update other code as necessary. >> It's not really that dsd2pcm cannot be multi-threaded, it's that the audio >> frames cannot be converted to PCM independently or >> you'll get a click at the frame boundaries. Maybe we're talking about the >> same thing, but I don't see how a lock would fix this. >> In any event, could you please show me an example of doing that? > The ffmpeg have functionality to await progress of some work, which > allows doing some stuff single threaded. > For example look at apng decoder in libavcodec/pngdec.c > See ff_thread_await/report_progress and > .update_thread_context/.init_thread_copy > and relevant documentation of mentioned functions. Okay, I've looked over that stuff and I agree that it might be usable for this, although it is currently only used for video codecs and pictures, and it might be tricky getting it right (at least for someone not intimate with FFmpeg internals). What would be cool is if you could implement this in dstdec.c and I'll copy that... ;-) Or maybe I'll figure it out first and provide a patch to dstdec.
Re: [FFmpeg-devel] avcodec: add a WavPack DSD decoder
On 7/25/19 8:36 AM, Lynne wrote:
> Jul 25, 2019, 6:12 AM by da...@wavpack.com:
>
>>>> + crc += (crc << 1) + code;
>>>>
>>> Don't NIH CRCs, we have av_crc in lavu. See below how to use it.
>>>
>> It's not a standard crc, but more of a recirculating checksum, so the NIH
>> code is required.
>>
> Could you not call it a CRC then? "checksum" is more appropriate.
> Wish a CRC was used, its so much better than a checksum and only slightly
> slower.
Changed variable name to "checksum".
Yes, I too wish I had used a real CRC, but 21 years ago it was not so obvious.
What I came up with is a position-dependent
checksum that's actually pretty close to a CRC in real-world robustness (and
certainly much better than a simple checksum).
>
>
>
>>>> + frame->nb_samples = s->samples + 1;
>>>> + if ((ret = ff_get_buffer(avctx, frame, 0)) < 0)
>>>> + return ret;
>>>> + frame->nb_samples = s->samples;
>>>>
>>> ?. Is the extra sample used as temporary buffer or something?
>>>
>> Your guess is as good as mine. This was part of the code "borrowed" from the
>> PCM version (with the threading removed) so maybe
>> there is (or was) a situation that was writing one extra sample off the end.
>> The code here certainly doesn't, but it seemed
>> pretty innocuous and I don't like just ripping out things I don't understand.
>>
> Just change it and run it through valgrind, I can't see the code using it.
Did that, and also discovered that I had mistakenly removed the freeing of the
frame contexts, which I put back. Oops!
>
>
> Rest looks fine to me.
Thanks for your feedback.
It turns out that this patch has been categorically rejected because it creates
a new codec ID for the new DSD codec. I don't
have time at this point to refactor it appropriately, but I wanted to leave it
in a working state in case someone wanted to test
it or use it as is in the interim. It has been exhaustively tested and I
believe that it is complete and safe.
Kind regards,
David
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
From afce89f76c1472a7f2632f335d79caf274a16ad7 Mon Sep 17 00:00:00 2001
From: David Bryant
Date: Sat, 27 Jul 2019 18:50:46 -0700
Subject: [PATCH] avcodec: add a WavPack DSD decoder
Signed-off-by: David Bryant
---
libavcodec/Makefile | 1 +
libavcodec/allcodecs.c | 1 +
libavcodec/avcodec.h | 1 +
libavcodec/codec_desc.c | 7 +
libavcodec/wavpack.h | 2 +
libavcodec/wavpack_dsd.c | 780 +++
libavformat/wvdec.c | 32 +-
7 files changed, 812 insertions(+), 12 deletions(-)
create mode 100644 libavcodec/wavpack_dsd.c
diff --git a/libavcodec/Makefile b/libavcodec/Makefile
index 3cd73fb..b94327e 100644
--- a/libavcodec/Makefile
+++ b/libavcodec/Makefile
@@ -686,6 +686,7 @@ OBJS-$(CONFIG_VPLAYER_DECODER) += textdec.o ass.o
OBJS-$(CONFIG_VP9_V4L2M2M_DECODER) += v4l2_m2m_dec.o
OBJS-$(CONFIG_VQA_DECODER) += vqavideo.o
OBJS-$(CONFIG_WAVPACK_DECODER) += wavpack.o
+OBJS-$(CONFIG_WAVPACK_DSD_DECODER) += wavpack_dsd.o dsd.o
OBJS-$(CONFIG_WAVPACK_ENCODER) += wavpackenc.o
OBJS-$(CONFIG_WCMV_DECODER)+= wcmv.o
OBJS-$(CONFIG_WEBP_DECODER)+= webp.o
diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
index d2f9a39..a2f414b 100644
--- a/libavcodec/allcodecs.c
+++ b/libavcodec/allcodecs.c
@@ -485,6 +485,7 @@ extern AVCodec ff_vorbis_encoder;
extern AVCodec ff_vorbis_decoder;
extern AVCodec ff_wavpack_encoder;
extern AVCodec ff_wavpack_decoder;
+extern AVCodec ff_wavpack_dsd_decoder;
extern AVCodec ff_wmalossless_decoder;
extern AVCodec ff_wmapro_decoder;
extern AVCodec ff_wmav1_encoder;
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index d234271..8d3a551 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -652,6 +652,7 @@ enum AVCodecID {
AV_CODEC_ID_SBC,
AV_CODEC_ID_ATRAC9,
AV_CODEC_ID_HCOM,
+AV_CODEC_ID_WAVPACK_DSD,
/* subtitle codecs */
AV_CODEC_ID_FIRST_SUBTITLE = 0x17000, ///< A dummy ID pointing at
the start of subtitle codecs.
diff --git a/libavcodec/codec_desc.c b/libavcodec/codec_desc.c
index 4d033c2..bee88b8 100644
--- a/libavcodec/codec_desc.c
+++ b/libavcodec/codec_desc.c
@@ -2985,6 +2985,13 @@ static const AVCodecDescriptor codec_descriptors[] = {
.long_name = NULL_IF_CONFIG_SMALL("HCOM Audio"),
.props = AV_CODEC_PROP_LO
Re: [FFmpeg-devel] avcodec: add a WavPack DSD decoder
On 7/21/19 5:57 PM, Lynne wrote:
> Jul 22, 2019, 12:03 AM by da...@wavpack.com:
>
>> Hi,
>>
>> As I promised late last year, here is a patch to add a WavPack DSD decoder.
>>
>> Thanks!
>>
>> -David Bryant
>>
>> + unsigned char probabilities [MAX_HISTORY_BINS] [256];
>> + unsigned char *value_lookup [MAX_HISTORY_BINS];
> Use uint8_t throughout the patch.
> Also don't add spaces between array declarations or lookups.
Done. New patch attached for all changes (hope that's the right way to do it).
>
>
>> +static void init_ptable (int *table, int rate_i, int rate_s)
>> +{
>> + int value = 0x808000, rate = rate_i << 8, c, i;
>> +
>> + for (c = (rate + 128) >> 8; c--;)
>> + value += (DOWN - value) >> DECAY;
>> +
>> + for (i = 0; i < PTABLE_BINS/2; ++i) {
> What's up with the random increment position in loops? It changes to before
> and after the variable throughout. Make it consistent and after the variable.
> Also we support declarative for (int loops. Can save lines.
Fixed random increment position and put in declarative int loops (even when
they didn't save a line).
>
>
>> + DSDfilters filters [2], *sp = filters;
> Same, spaces after variables for arrays, all throughout the file.
Done. Also fixed spaces after function names.
>
>
>> + if (code > max_probability) {
>> + int zcount = code - max_probability;
>> +
>> + while (outptr < outend && zcount--)
>> + *outptr++ = 0;
>> + }
>> + else if (code)
>> + *outptr++ = code;
>> + else
>> + break;
> We don't put else on a new line, and prefer to have each branch wrapped in
> bracket unless all branches are single lines.
Fixed.
>
>
>> + for (p0 = 0; p0 < history_bins; ++p0) {
>> + int32_t sum_values;
>> + unsigned char *vp;
>> +
>> + for (sum_values = i = 0; i < 256; ++i)
>> + s->summed_probabilities [p0] [i] = sum_values +=
>> s->probabilities [p0] [i];
> sum_values is uninitialized. Does you compiler not warn about this?
This was pointed out already as actually initialized, but I made it clearer
(obviously it wasn't).
>
>
>> + if (sum_values) {
>> + total_summed_probabilities += sum_values;
>> + vp = s->value_lookup [p0] = av_malloc (sum_values);
> I don't like the per-frame alloc. The maximum sum_values can be is 255*255 =
> UINT16_MAX.
> 60k of memory isn't much at all, just define value_lookup[255*255] in the
> context and you'll probably plug a few out of bounds accesses too.
It's actually up to 32 allocs per frame because there's one for each history
bin (value_lookup is an array of pointers, not
uint8s), and I didn't like it either because I had to worry about de-allocing
on error. Refactored to use a single array in the
context as a pool. Thanks for the suggestion!
>
>
>> +mult = high / s->summed_probabilities [p0] [255];
> s->summed_probabilities [p0] [255]; can be zero, you already check if its
> zero when allocating currently. You should probably check for divide by zero
> unless you're very sure it can't happen.
I'm very sure. The checks are within a few lines above each of the three
divides.
>
>
>> + crc += (crc << 1) + code;
> Don't NIH CRCs, we have av_crc in lavu. See below how to use it.
It's not a standard crc, but more of a recirculating checksum, so the NIH code
is required.
>
>
>> +static int wv_unpack_dsd_copy(WavpackFrameContext *s, void *dst_l, void
>> *dst_r)
>> +{
>> + uint8_t *dsd_l = dst_l;
>> + uint8_t *dsd_r = dst_r;
> You're shadowing arguments. Your compiler doesn't warn on this either?
> You're calling the function with uint8_ts anyway, just change the type.
They're not shadowed (dsd vs. dst) which is why my compiler didn't complain,
but I took your suggestion of just changing the types.
>
>
>> + while (total_samples--) {
>> + crc += (crc << 1) + (*dsd_l = bytestream2_get_byte(>gb));
>> + dsd_l += 4;
>> +
>> + if (dst_r) {
>> + crc += (crc << 1) + (*dsd_r = bytestream2_get_byte(>gb));
>> + dsd_r += 4;
>> + }
>> + }
> av_crc(av_crc_get_table(AV_CRC_32_IEEE/LE), UINT32_MAX, dsd_start_r/l,
> dsd_r/l - dsd_start_r/l) should work and be faster.
see above
>
>
>> + s->fdec_num = 0;
> Private codec context is always zeroed already.
removed
Re: [FFmpeg-devel] avcodec: add a WavPack DSD decoder
On 7/23/19 12:47 AM, Paul B Mahol wrote: > On 7/23/19, David Bryant wrote: >> On 7/21/19 11:23 PM, Paul B Mahol wrote: >>> On 7/22/19, David Bryant wrote: >>>> Hi, >>>> >>>> As I promised late last year, here is a patch to add a WavPack DSD >>>> decoder. >>>> >>>> Thanks! >>>> >>>> -David Bryant >>>> >>>> >>> Please correct me if I'm wrong, but why this uses new codec id? >>> Apparently is also copies some logic from other files. >> Yes, I created a new codec ID for WavPack DSD. It would be possible to just >> piggyback on the existing WavPack codec by silently >> decimating the DSD to PCM, but that seemed weird and wrong to me. For one >> thing, the user would have no idea that the file was >> actually DSD and not high sample-rate PCM. >> >> Also, since regular WavPack has threading enabled but WavPack DSD can't >> (because of the dsd2pcm conversion) I would have to turn >> off threading for all WavPack (unless there's some way of making that >> conditional at runtime). It would also mean that regular >> WavPack would be dependent on the dsd2pcm component even if DSD was not >> required (not everyone needs DSD). And of course I was >> looking closely at the only other DSD codec in FFmpeg (DST) which has its >> own codec ID. >> >> Because regular WavPack PCM and DSD share the same block format and metadata >> structure, there is a little code that is shared >> between the two codecs (although they are no longer identical because of the >> threading difference). Is this a problem? I could >> combine the two codecs into one file and sprinkle in a few conditionals, but >> I don't think it would be as clean or clear (but >> might save a little duplicate code). >> >> That's my thinking, but obviously I am not as familiar with the FFmpeg >> philosophy as you guys. > Looking carefully at dsd2pcm code in your patch, it appears it would > work only with 1 or 2 channels. > Is this correct? Individual WavPack blocks can only be 1 or 2 channels. Multichannel files contains sequences of blocks which ends up creating multiple frame contexts. This was originally implemented for PCM WavPack years ago and so it "just works" for DSD WavPack because I didn't touch any of that. I have tested this extensively with multichannel DSD files and it works great, including seeking. > > About multi-threading, as dsd2pcm can not be made multi-threaded then > just do locking when doing dsd2pcm to make it single threaded and > update other code as necessary. It's not really that dsd2pcm cannot be multi-threaded, it's that the audio frames cannot be converted to PCM independently or you'll get a click at the frame boundaries. Maybe we're talking about the same thing, but I don't see how a lock would fix this. In any event, could you please show me an example of doing that? > > I still see no valid reason to add separate codec id. Add another > wavpack profile if you wish to help users know the difference between > the two. Again I apologize for not being too familiar with FFmpeg internals. What would another wavpack "profile" be? As long as the user (or caller) has some way to differentiate between DSD and PCM files without parsing the file themselves I'm happy, but what exactly is the hesitation here? Are codec IDs a limited resource? It seems like an obvious win to me. -David > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] avcodec: add a WavPack DSD decoder
On 7/21/19 11:23 PM, Paul B Mahol wrote: > On 7/22/19, David Bryant wrote: >> Hi, >> >> As I promised late last year, here is a patch to add a WavPack DSD decoder. >> >> Thanks! >> >> -David Bryant >> >> > Please correct me if I'm wrong, but why this uses new codec id? > Apparently is also copies some logic from other files. Yes, I created a new codec ID for WavPack DSD. It would be possible to just piggyback on the existing WavPack codec by silently decimating the DSD to PCM, but that seemed weird and wrong to me. For one thing, the user would have no idea that the file was actually DSD and not high sample-rate PCM. Also, since regular WavPack has threading enabled but WavPack DSD can't (because of the dsd2pcm conversion) I would have to turn off threading for all WavPack (unless there's some way of making that conditional at runtime). It would also mean that regular WavPack would be dependent on the dsd2pcm component even if DSD was not required (not everyone needs DSD). And of course I was looking closely at the only other DSD codec in FFmpeg (DST) which has its own codec ID. Because regular WavPack PCM and DSD share the same block format and metadata structure, there is a little code that is shared between the two codecs (although they are no longer identical because of the threading difference). Is this a problem? I could combine the two codecs into one file and sprinkle in a few conditionals, but I don't think it would be as clean or clear (but might save a little duplicate code). That's my thinking, but obviously I am not as familiar with the FFmpeg philosophy as you guys. -David > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe". ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] avcodec: add a WavPack DSD decoder
Hi,
As I promised late last year, here is a patch to add a WavPack DSD decoder.
Thanks!
-David Bryant
From d80f4fb59e4afec1e2c539391a7b4484f5ae8436 Mon Sep 17 00:00:00 2001
From: David Bryant
Date: Sun, 21 Jul 2019 15:43:25 -0700
Subject: [PATCH] avcodec: add a WavPack DSD decoder
Signed-off-by: David Bryant
---
libavcodec/Makefile | 1 +
libavcodec/allcodecs.c | 1 +
libavcodec/avcodec.h | 1 +
libavcodec/codec_desc.c | 7 +
libavcodec/wavpack.h | 2 +
libavcodec/wavpack_dsd.c | 792 +++
libavformat/wvdec.c | 32 +-
7 files changed, 824 insertions(+), 12 deletions(-)
create mode 100644 libavcodec/wavpack_dsd.c
diff --git a/libavcodec/Makefile b/libavcodec/Makefile
index 3cd73fb..b94327e 100644
--- a/libavcodec/Makefile
+++ b/libavcodec/Makefile
@@ -686,6 +686,7 @@ OBJS-$(CONFIG_VPLAYER_DECODER) += textdec.o ass.o
OBJS-$(CONFIG_VP9_V4L2M2M_DECODER) += v4l2_m2m_dec.o
OBJS-$(CONFIG_VQA_DECODER) += vqavideo.o
OBJS-$(CONFIG_WAVPACK_DECODER) += wavpack.o
+OBJS-$(CONFIG_WAVPACK_DSD_DECODER) += wavpack_dsd.o dsd.o
OBJS-$(CONFIG_WAVPACK_ENCODER) += wavpackenc.o
OBJS-$(CONFIG_WCMV_DECODER)+= wcmv.o
OBJS-$(CONFIG_WEBP_DECODER)+= webp.o
diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
index d2f9a39..a2f414b 100644
--- a/libavcodec/allcodecs.c
+++ b/libavcodec/allcodecs.c
@@ -485,6 +485,7 @@ extern AVCodec ff_vorbis_encoder;
extern AVCodec ff_vorbis_decoder;
extern AVCodec ff_wavpack_encoder;
extern AVCodec ff_wavpack_decoder;
+extern AVCodec ff_wavpack_dsd_decoder;
extern AVCodec ff_wmalossless_decoder;
extern AVCodec ff_wmapro_decoder;
extern AVCodec ff_wmav1_encoder;
diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index d234271..8d3a551 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -652,6 +652,7 @@ enum AVCodecID {
AV_CODEC_ID_SBC,
AV_CODEC_ID_ATRAC9,
AV_CODEC_ID_HCOM,
+AV_CODEC_ID_WAVPACK_DSD,
/* subtitle codecs */
AV_CODEC_ID_FIRST_SUBTITLE = 0x17000, ///< A dummy ID pointing at
the start of subtitle codecs.
diff --git a/libavcodec/codec_desc.c b/libavcodec/codec_desc.c
index 4d033c2..bee88b8 100644
--- a/libavcodec/codec_desc.c
+++ b/libavcodec/codec_desc.c
@@ -2985,6 +2985,13 @@ static const AVCodecDescriptor codec_descriptors[] = {
.long_name = NULL_IF_CONFIG_SMALL("HCOM Audio"),
.props = AV_CODEC_PROP_LOSSY,
},
+{
+.id= AV_CODEC_ID_WAVPACK_DSD,
+.type = AVMEDIA_TYPE_AUDIO,
+.name = "wavpack_dsd",
+.long_name = NULL_IF_CONFIG_SMALL("WavPack DSD"),
+.props = AV_CODEC_PROP_LOSSLESS,
+},
/* subtitle codecs */
{
diff --git a/libavcodec/wavpack.h b/libavcodec/wavpack.h
index 6caad03..43aaac8 100644
--- a/libavcodec/wavpack.h
+++ b/libavcodec/wavpack.h
@@ -35,6 +35,7 @@
#define WV_FLOAT_DATA 0x0080
#define WV_INT32_DATA 0x0100
#define WV_FALSE_STEREO 0x4000
+#define WV_DSD_DATA 0x8000
#define WV_HYBRID_MODE0x0008
#define WV_HYBRID_SHAPE 0x0008
@@ -77,6 +78,7 @@ enum WP_ID {
WP_ID_CORR,
WP_ID_EXTRABITS,
WP_ID_CHANINFO,
+WP_ID_DSD_DATA,
WP_ID_SAMPLE_RATE = 0x27,
};
diff --git a/libavcodec/wavpack_dsd.c b/libavcodec/wavpack_dsd.c
new file mode 100644
index 000..0754314
--- /dev/null
+++ b/libavcodec/wavpack_dsd.c
@@ -0,0 +1,792 @@
+/*
+ * WavPack lossless DSD audio decoder
+ * Copyright (c) 2006,2011 Konstantin Shishkov
+ * Copyright (c) 2019 David Bryant
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+#include "libavutil/channel_layout.h"
+
+#include "avcodec.h"
+#include "bytestream.h"
+#include "internal.h"
+#include "wavpack.h"
+#include "dsd.h"
+
+/**
+ * @file
+ * WavPack lossless DSD audio decoder
+ */
+
+#define DSD_BYTE_READY(low,high) (!(((low) ^ (high)) & 0xff00))
+
+#define PTABLE_BITS 8
+#define PTABLE_BINS (1<CRC) {
+av_log(s->avctx, AV_LOG_ERROR, "CRC error\n");
+return AVERROR_INVALIDDATA;
+}
+
+return 0;
+}
+
Re: [FFmpeg-devel] patch for failing on WavPack DSD files
On 1/6/19 4:43 AM, Carl Eugen Hoyos wrote: > 2019-01-03 6:19 GMT+01:00, David Bryant : >> On 12/28/18 3:56 AM, Paul B Mahol wrote: >>> On 12/24/18, Derek Buitenhuis wrote: >>>> On 24/12/2018 17:47, David Bryant wrote: >>>>> I want to do that, but am swamped at work right now, so it will probably >>>>> be a few months before I can get to that. >>>>> >>>>> In the meantime, I think this patch would be a safe stopgap (and prevent >>>>> the Kodi crash). >>>> I think it's OK for the meantime (my opinion). >>> Applied. >> The guys at Kodi asked me to request that this patch be backported to 4.0. > While this may be ok, somebody should tell the Kodi developers > to fix the crash as I would expect it's possible to create files > that are still tried to be decoded but produce identical output > as before. > > Carl Eugen > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel I agree. In fact, take a WavPack DSD file and clear the bit that this patch checks for, and I'll bet you'll get exactly the old behavior. It should be investigated as to why Kodi crashes with these files while FFmpeg just spews a bunch of error messages. -David ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] patch for failing on WavPack DSD files
On 12/28/18 3:56 AM, Paul B Mahol wrote: > On 12/24/18, Derek Buitenhuis wrote: >> On 24/12/2018 17:47, David Bryant wrote: >>> I want to do that, but am swamped at work right now, so it will probably >>> be a few months before I can get to that. >>> >>> In the meantime, I think this patch would be a safe stopgap (and prevent >>> the Kodi crash). >> I think it's OK for the meantime (my opinion). > Applied. > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel Thanks! The guys at Kodi asked me to request that this patch be backported to 4.0. I don't really know if that's a thing, but if it is then that would be great. -David ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] patch for failing on WavPack DSD files
On 12/24/18 12:21 AM, Paul B Mahol wrote: > On 12/24/18, David Bryant wrote: >> On 11/21/18 9:50 PM, David Bryant wrote: >>> On 11/20/18 10:58 PM, Peter Ross wrote: >>>> On Tue, Nov 20, 2018 at 09:23:03PM -0800, David Bryant wrote: >>>>> Hi, >>>>> >>>>> Was made aware of this problem on Kodi: >>>>> >>>>> https://github.com/xbmc/xbmc/issues/14771 >>>>> >>>>> I'm going to try to add full WavPack DSD support, but thought in the >>>>> meantime it would be a good idea to detect and error out early. >>>>> >>>>> Thanks! >>>> cool. is this dst-based, or your own algorithm? >>>> >>>> -- Peter >>>> (A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B) >>>> >>>> ___ >>>> ffmpeg-devel mailing list >>>> ffmpeg-devel@ffmpeg.org >>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >>> This is my own algorithm. Actually there are two: one is a byte-based >>> algorithm that is very fast and the other is bit-based >>> (like DST) that is slower but compresses better (and still uses less CPU >>> than DST). If I had to make a rough estimation of the >>> average compression ratios, it would be about 0.6 for the fast mode, 0.5 >>> for the high mode, and 0.4 for DST. >>> >>> -David >>> >>> >>> ___ >>> ffmpeg-devel mailing list >>> ffmpeg-devel@ffmpeg.org >>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >> Hi. Is there anything else I can provide to help this along? >> >> The behavior of FFmpeg encountering WavPack DSD files without this patch is >> pretty ugly... > Yes, post full patch that adds DSD support in WavPack. > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel I want to do that, but am swamped at work right now, so it will probably be a few months before I can get to that. In the meantime, I think this patch would be a safe stopgap (and prevent the Kodi crash). Thanks! -David ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] patch for failing on WavPack DSD files
On 11/21/18 9:50 PM, David Bryant wrote: > On 11/20/18 10:58 PM, Peter Ross wrote: >> On Tue, Nov 20, 2018 at 09:23:03PM -0800, David Bryant wrote: >>> Hi, >>> >>> Was made aware of this problem on Kodi: >>> >>> https://github.com/xbmc/xbmc/issues/14771 >>> >>> I'm going to try to add full WavPack DSD support, but thought in the >>> meantime it would be a good idea to detect and error out early. >>> >>> Thanks! >> cool. is this dst-based, or your own algorithm? >> >> -- Peter >> (A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B) >> >> ___ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > This is my own algorithm. Actually there are two: one is a byte-based > algorithm that is very fast and the other is bit-based > (like DST) that is slower but compresses better (and still uses less CPU than > DST). If I had to make a rough estimation of the > average compression ratios, it would be about 0.6 for the fast mode, 0.5 for > the high mode, and 0.4 for DST. > > -David > > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel Hi. Is there anything else I can provide to help this along? The behavior of FFmpeg encountering WavPack DSD files without this patch is pretty ugly... -David ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] patch for failing on WavPack DSD files
On 11/20/18 10:58 PM, Peter Ross wrote: > On Tue, Nov 20, 2018 at 09:23:03PM -0800, David Bryant wrote: >> Hi, >> >> Was made aware of this problem on Kodi: >> >> https://github.com/xbmc/xbmc/issues/14771 >> >> I'm going to try to add full WavPack DSD support, but thought in the >> meantime it would be a good idea to detect and error out early. >> >> Thanks! > cool. is this dst-based, or your own algorithm? > > -- Peter > (A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B) > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel This is my own algorithm. Actually there are two: one is a byte-based algorithm that is very fast and the other is bit-based (like DST) that is slower but compresses better (and still uses less CPU than DST). If I had to make a rough estimation of the average compression ratios, it would be about 0.6 for the fast mode, 0.5 for the high mode, and 0.4 for DST. -David ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] patch for failing on WavPack DSD files
Hi,
Was made aware of this problem on Kodi:
https://github.com/xbmc/xbmc/issues/14771
I'm going to try to add full WavPack DSD support, but thought in the meantime
it would be a good idea to detect and error out early.
Thanks!
David
>From c86aacdf98c3d34a3f8d63233e01c4a3ab55577e Mon Sep 17 00:00:00 2001
From: David Bryant
Date: Tue, 20 Nov 2018 21:00:47 -0800
Subject: [PATCH] detect and error out on WavPack DSD files (which are not
currently supported)
---
libavformat/wvdec.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/libavformat/wvdec.c b/libavformat/wvdec.c
index 8252656..2060523 100644
--- a/libavformat/wvdec.c
+++ b/libavformat/wvdec.c
@@ -40,6 +40,7 @@ enum WV_FLAGS {
WV_HBAL = 0x0400,
WV_MCINIT = 0x0800,
WV_MCEND = 0x1000,
+WV_DSD= 0x8000,
};
static const int wv_rates[16] = {
@@ -97,6 +98,11 @@ static int wv_read_block_header(AVFormatContext *ctx, AVIOContext *pb)
return ret;
}
+if (wc->header.flags & WV_DSD) {
+avpriv_report_missing_feature(ctx, "WV DSD");
+return AVERROR_PATCHWELCOME;
+}
+
if (wc->header.version < 0x402 || wc->header.version > 0x410) {
avpriv_report_missing_feature(ctx, "WV version 0x%03X",
wc->header.version);
--
1.9.1
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
