Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On Thu, Jan 14, 2016 at 12:28:32AM +0100, Andreas Cadhalpun wrote: > On 13.01.2016 19:18, foo86 wrote: > > Hmm, core sample rate and number of samples are already checked to be > > compatible with XLL at this point. There must be some bug in upsampling > > logic if these asserts are triggering, however it is not obvious for me > > where the problem is... Can you provide a sample which triggers these? > > It would be helpful. > > Sure, I'll send them to you privately. Thanks for the samples. This issue should be fixed now. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On 13.01.2016 19:18, foo86 wrote: > On Tue, Jan 12, 2016 at 11:58:43PM +0100, Andreas Cadhalpun wrote: >> It's not completely fixed yet, because the check is only done if >> static_fields_present is 1. > > You are right, placing the check there doesn't account for case when > static_fields_present becomes 0 later. I moved the check closer to > problematic get_bits() call for a more complete fix. OK. >> I think the correct fix would be to set mix_metadata_enabled to 0 if >> static_fields_present is 0, e.g. in the else branch in ff_dca_exss_parse. > > The spec doesn't give many details about this, but when > static_fields_present is 0, values parsed earlier are supposedly > retained for stream duration, so clearing mix_metadata_enabled doesn't > seem correct in this case. I see. >> Thanks to your hints I could increase the coverage above 90%. >> I found a few more issues along the way: >> >> static void get_array(GetBitContext *s, int *array, int size, int n) >> { >> int i; >> >> for (i = 0; i < size; i++) >> array[i] = get_sbits(s, n); >> >> This should be get_sbits_long, because n can be larger than 25. > > Added xbr_bit_allocation[][] range check to make sure get_sbits() > argument doesn't exceed 23. Thanks. >> static int chs_assemble_freq_bands(DCAXllDecoder *s, DCAXllChSet *c) >> { >> int i, ch, nsamples = s->nframesamples, *ptr; >> >> av_assert1(c->nfreqbands > 1); >> >> This assert can be triggered. > > There was supposed to be a check to ensure that all channel sets have > the same number of frequency bands, but apparently it got lost during > porting. Re-added it. That fixed it. >> static void combine_residual_frame(DCAXllDecoder *s, DCACoreDecoder *core, >> DCAXllChSet *c) >> { >> int ch, nsamples = s->nframesamples; >> DCAXllChSet *o; >> >> av_assert0(c->freq == core->output_rate); >> av_assert0(nsamples == core->npcmsamples); >> >> These two, as well. >> Maybe they should be regular error checks instead? > > Hmm, core sample rate and number of samples are already checked to be > compatible with XLL at this point. There must be some bug in upsampling > logic if these asserts are triggering, however it is not obvious for me > where the problem is... Can you provide a sample which triggers these? > It would be helpful. Sure, I'll send them to you privately. >> static void scale_down_mix(DCAXllDecoder *s, DCAXllChSet *o, int band) >> { >> int i, j, nchannels = 0, nsamples = s->nframesamples; >> DCAXllChSet *c; >> >> for (i = 0, c = s->chset; i < s->nactivechsets; i++, c++) { >> if (!c->hier_chset) >> continue; >> for (j = 0; j < c->nchannels; j++) { >> int scale = o->dmix_scale[nchannels++]; >> if (scale != (1 << 15)) { >> s->dcadsp->dmix_scale(c->bands[band].msb_sample_buffer[j], >> scale, nsamples); >> >> c->bands[band].msb_sample_buffer[j] can be NULL here, which causes a crash. > > A check re-added earlier to ensure that all channels sets have the same > number of frequency bands should fix this as well, I think. Yes, it does. >> Additionally there are some rarely happening overflows in dcadsp.c. >> (More precisely in filter0, filter1, dmix_sub_c and dmix_scale_c.) >> It would be nice to avoid those, if that's possible without significant >> speed loss. > > I wouldn't worry much about overflows if they happen only with fuzzed > files... Though this is definitely a problem if normal streams trigger > these. They only happen with fuzzed samples and quite rarely at that. I just wanted to mention it for completeness. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On Tue, Jan 12, 2016 at 11:58:43PM +0100, Andreas Cadhalpun wrote: > It's not completely fixed yet, because the check is only done if > static_fields_present is 1. You are right, placing the check there doesn't account for case when static_fields_present becomes 0 later. I moved the check closer to problematic get_bits() call for a more complete fix. > I think the correct fix would be to set mix_metadata_enabled to 0 if > static_fields_present is 0, e.g. in the else branch in ff_dca_exss_parse. The spec doesn't give many details about this, but when static_fields_present is 0, values parsed earlier are supposedly retained for stream duration, so clearing mix_metadata_enabled doesn't seem correct in this case. > Thanks to your hints I could increase the coverage above 90%. > I found a few more issues along the way: > > static void get_array(GetBitContext *s, int *array, int size, int n) > { > int i; > > for (i = 0; i < size; i++) > array[i] = get_sbits(s, n); > > This should be get_sbits_long, because n can be larger than 25. Added xbr_bit_allocation[][] range check to make sure get_sbits() argument doesn't exceed 23. > static int chs_assemble_freq_bands(DCAXllDecoder *s, DCAXllChSet *c) > { > int i, ch, nsamples = s->nframesamples, *ptr; > > av_assert1(c->nfreqbands > 1); > > This assert can be triggered. There was supposed to be a check to ensure that all channel sets have the same number of frequency bands, but apparently it got lost during porting. Re-added it. > static void combine_residual_frame(DCAXllDecoder *s, DCACoreDecoder *core, > DCAXllChSet *c) > { > int ch, nsamples = s->nframesamples; > DCAXllChSet *o; > > av_assert0(c->freq == core->output_rate); > av_assert0(nsamples == core->npcmsamples); > > These two, as well. > Maybe they should be regular error checks instead? Hmm, core sample rate and number of samples are already checked to be compatible with XLL at this point. There must be some bug in upsampling logic if these asserts are triggering, however it is not obvious for me where the problem is... Can you provide a sample which triggers these? It would be helpful. > static void scale_down_mix(DCAXllDecoder *s, DCAXllChSet *o, int band) > { > int i, j, nchannels = 0, nsamples = s->nframesamples; > DCAXllChSet *c; > > for (i = 0, c = s->chset; i < s->nactivechsets; i++, c++) { > if (!c->hier_chset) > continue; > for (j = 0; j < c->nchannels; j++) { > int scale = o->dmix_scale[nchannels++]; > if (scale != (1 << 15)) { > s->dcadsp->dmix_scale(c->bands[band].msb_sample_buffer[j], > scale, nsamples); > > c->bands[band].msb_sample_buffer[j] can be NULL here, which causes a crash. A check re-added earlier to ensure that all channels sets have the same number of frequency bands should fix this as well, I think. > Additionally there are some rarely happening overflows in dcadsp.c. > (More precisely in filter0, filter1, dmix_sub_c and dmix_scale_c.) > It would be nice to avoid those, if that's possible without significant > speed loss. I wouldn't worry much about overflows if they happen only with fuzzed files... Though this is definitely a problem if normal streams trigger these. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On 08.01.2016 15:34, foo86 wrote: > On Thu, Jan 07, 2016 at 08:17:59PM +0100, Andreas Cadhalpun wrote: >> On 03.01.2016 18:49, foo86 wrote: >>> +for (i = 0; i < s->nmixoutconfigs; i++) { >>> +for (j = 0; j < nchannels_dmix; j++) { >>> +// Mix output mask >>> +int mix_map_mask = get_bits(&s->gb, s->nmixoutchs[i]); >> >> Here s->nmixoutchs[i] can be zero. If that should not happen, there needs >> to be an error check and otherwise it should use get_bitsz, because >> get_bits doesn't support reading 0 bits. > > It doesn't seem that zero channel mask should normally happen, added an > error check earlier. It's not completely fixed yet, because the check is only done if static_fields_present is 1. I think the correct fix would be to set mix_metadata_enabled to 0 if static_fields_present is 0, e.g. in the else branch in ff_dca_exss_parse. >> Anyway, I still think the code is pretty robust. :-) > > Thanks for testing! You're welcome. >> I'd be glad to increase fuzz-testing coverage further, but I'm lacking >> input examples. It would be great if you could share some (tiny) samples >> triggering the HEADER_XCH/HEADER_XXCH cases and/or *_down_mix functions. > > As Hendrik pointed out, there are lidcadec test suite samples you can > use for this. To trigger all downmixing functions you may need to > provide -request_channel_layout 3 option. Thanks to your hints I could increase the coverage above 90%. I found a few more issues along the way: static void get_array(GetBitContext *s, int *array, int size, int n) { int i; for (i = 0; i < size; i++) array[i] = get_sbits(s, n); This should be get_sbits_long, because n can be larger than 25. static int chs_assemble_freq_bands(DCAXllDecoder *s, DCAXllChSet *c) { int i, ch, nsamples = s->nframesamples, *ptr; av_assert1(c->nfreqbands > 1); This assert can be triggered. static void combine_residual_frame(DCAXllDecoder *s, DCACoreDecoder *core, DCAXllChSet *c) { int ch, nsamples = s->nframesamples; DCAXllChSet *o; av_assert0(c->freq == core->output_rate); av_assert0(nsamples == core->npcmsamples); These two, as well. Maybe they should be regular error checks instead? static void scale_down_mix(DCAXllDecoder *s, DCAXllChSet *o, int band) { int i, j, nchannels = 0, nsamples = s->nframesamples; DCAXllChSet *c; for (i = 0, c = s->chset; i < s->nactivechsets; i++, c++) { if (!c->hier_chset) continue; for (j = 0; j < c->nchannels; j++) { int scale = o->dmix_scale[nchannels++]; if (scale != (1 << 15)) { s->dcadsp->dmix_scale(c->bands[band].msb_sample_buffer[j], scale, nsamples); c->bands[band].msb_sample_buffer[j] can be NULL here, which causes a crash. Additionally there are some rarely happening overflows in dcadsp.c. (More precisely in filter0, filter1, dmix_sub_c and dmix_scale_c.) It would be nice to avoid those, if that's possible without significant speed loss. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On Thu, Jan 07, 2016 at 03:32:50PM +0100, Christophe Gisquet wrote: > One commit implements "sum/diff decoding", introducing sumdiff_X > functions, with X fixed or float. > > I think those are the corresponding butterflies_X functions in the > AV(Float|Fixed)DSPContext structures. But I haven't verified the > alignment requirements. Seems to be it, thanks for your suggestion. Changed the floating point code to use butterflies_float. Core decoder doesn't use AVFixedDSPContext currently, so I left an ad-hoc fixed point approach. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On Thu, Jan 07, 2016 at 08:17:59PM +0100, Andreas Cadhalpun wrote: > On 03.01.2016 18:49, foo86 wrote: > > +for (i = 0; i < spkr_remap_nsets; i++) { > > +// Number of channels to be decoded for speaker remapping > > +int nch_for_remaps = get_bits(&s->gb, 5) + 1; > > + > > +for (j = 0; j < nspeakers[i]; j++) { > > +// Decoded channels to output speaker mapping mask > > +int remap_ch_mask = get_bits(&s->gb, nch_for_remaps); > > Here nch_for_remaps can be up to 32, so this has to use get_bits_long, as > get_bits only supports reading 1-25 bits. Missed this one, changed to get_bits_long. > > +for (i = 0; i < s->nmixoutconfigs; i++) { > > +for (j = 0; j < nchannels_dmix; j++) { > > +// Mix output mask > > +int mix_map_mask = get_bits(&s->gb, s->nmixoutchs[i]); > > Here s->nmixoutchs[i] can be zero. If that should not happen, there needs > to be an error check and otherwise it should use get_bitsz, because > get_bits doesn't support reading 0 bits. It doesn't seem that zero channel mask should normally happen, added an error check earlier. > Anyway, I still think the code is pretty robust. :-) Thanks for testing! > I'd be glad to increase fuzz-testing coverage further, but I'm lacking > input examples. It would be great if you could share some (tiny) samples > triggering the HEADER_XCH/HEADER_XXCH cases and/or *_down_mix functions. As Hendrik pointed out, there are lidcadec test suite samples you can use for this. To trigger all downmixing functions you may need to provide -request_channel_layout 3 option. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On 07.01.2016 20:20, Hendrik Leppkes wrote: > On Thu, Jan 7, 2016 at 8:17 PM, Andreas Cadhalpun > wrote: >> >> I'd be glad to increase fuzz-testing coverage further, but I'm lacking >> input examples. It would be great if you could share some (tiny) samples >> triggering the HEADER_XCH/HEADER_XXCH cases and/or *_down_mix functions. >> > > These samples should cover nearly all features in the decoder: > https://github.com/foo86/dcadec-samples Thanks, I'll try fuzzing with these. > Once its commited, we should make them FATE tests as well. Yes, that would be good. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On 06.01.2016 23:17, Andreas Cadhalpun wrote: > On 06.01.2016 18:32, foo86 wrote: >> Otherwise testing coverage will be decreased somewhat. The easiest way to do >> this is to modify ff_dca2_check_crc() to always return 0. > > I tried this (comment out everything in ff_dca2_check_crc except 'return 0') > and there doesn't seem to be much difference. The reason why this didn't make any difference is that this function wasn't executed at all for my fuzzed samples. :-/ I fixed that and coverage is now much better. Hence I also have a few more (minor) comments: On 03.01.2016 18:49, foo86 wrote: > +for (i = 0; i < spkr_remap_nsets; i++) { > +// Number of channels to be decoded for speaker remapping > +int nch_for_remaps = get_bits(&s->gb, 5) + 1; > + > +for (j = 0; j < nspeakers[i]; j++) { > +// Decoded channels to output speaker mapping mask > +int remap_ch_mask = get_bits(&s->gb, nch_for_remaps); Here nch_for_remaps can be up to 32, so this has to use get_bits_long, as get_bits only supports reading 1-25 bits. > +for (i = 0; i < s->nmixoutconfigs; i++) { > +for (j = 0; j < nchannels_dmix; j++) { > +// Mix output mask > +int mix_map_mask = get_bits(&s->gb, s->nmixoutchs[i]); Here s->nmixoutchs[i] can be zero. If that should not happen, there needs to be an error check and otherwise it should use get_bitsz, because get_bits doesn't support reading 0 bits. Anyway, I still think the code is pretty robust. :-) I'd be glad to increase fuzz-testing coverage further, but I'm lacking input examples. It would be great if you could share some (tiny) samples triggering the HEADER_XCH/HEADER_XXCH cases and/or *_down_mix functions. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
Hi, 2016-01-07 12:48 GMT+01:00 foo86 : > bench dca pcm_f32le > bench dca2 pcm_f32le > bench libdcadec pcm_s32le OK, that was mostly out of curiosity, as "dca2" has benefits surpassing such issues anyway. And the improvement is 10% for where it matters (raspberry). > synth_filter integrates just nicely without any changes, there is no need for > rewrite. There is 64 subband version of it needed though, as well as fixed > point versions for 32 and 64 subbands. In my recollection, the code was weirdly shaped, so I could have imagined needs for change (e.g., your libdcadec may not be able to reuse it). > Other parts can't be directly reused unfortunately. They need to be converted > to operate on integer input and process entire frame at once (not just one > subsubframe). The gap between dca and dca2 will probably increase, as these "constrains" should actually help make the operation faster. > Temporary converting the input to floating point before calling them (what > current dca code does) seems to negate any performance gain from using these > assembly optimized routines in the first place... I think it is ok to change the code and remove the arch-specific dsp implementations. Whether to put that specific removal in a separate patch is up to you. -- Christophe ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On Wed, Jan 06, 2016 at 02:53:32PM -0300, James Almer wrote: > On 1/6/2016 2:32 PM, foo86 wrote: > > OK, I'll start changing the patch. > > > > Some questions so far: > > > > 1. Should I remove old decoder files in separate commit first or should > > I simply proceed with replacing entire content of certain files (e.g., > > dcadec.c, dca_exss.c, dca_xll.c)? > > Yeah, send it as separate patches to make reviewing easier. Whoever commits it > can then squash it into a single patch (Like it was done in commit b08569a2) > if > necessary. > > Also, while at it, try to split the main header into separate headers for some > of the modules (xll, exss, dsp which already exists for the old decoder, etc). OK. > > 2. Is it OK to leave arch-specific dca code as well as dcadsp.c > > untouched for now? I'd really like to postpone dealing with that until > > I'm done with generic code, especially since the new decoder is not > > slower already. This means some assembly functions (ff_dca_lfe_fir32_*, > > ff_dca_qmf_32_subbands_*) will still be compiled in but unused. > > libavcodec/dcadsp.c will be still around but not compiled. > > Try to disable or remove code that will not be used. Git will easily let you > recover any of it in the future if needed. > Better than keeping dead code in the tree, IMO, but do what you think will > make > your work easier. Yeah, I'll probably remove it for now and recover any needed parts later. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On 06.01.2016 18:32, foo86 wrote: > On Tue, Jan 05, 2016 at 10:46:19PM +0100, Andreas Cadhalpun wrote: >> OK. This decoder seems to be quite robust in handling fuzzed samples, >> so from a security point of view it should be fine to replace the >> old dca decoder with this one. > > Out of interest, did you disable CRC checks in the decoder while fuzzing? No, I tested it as is. > Otherwise testing coverage will be decreased somewhat. The easiest way to do > this is to modify ff_dca2_check_crc() to always return 0. I tried this (comment out everything in ff_dca2_check_crc except 'return 0') and there doesn't seem to be much difference. >> Out of curiosity: Can you post a few benchmarks comparing the performance >> of the old and the new decoder? > > Here are some benchmarks I did with 3 available decoders on 3 different > systems: (1) x86_64 desktop, (2) ancient i386 laptop, (3) Raspberry Pi Model > B+. > > Measuring how much time it takes to loop 2000 times over a short 5.1 > channel core sample (for a total duration of 00:51:56.22) yields the > following results: > > dca dca2libdcadec > System 1: 0:11.90 0:11.16 0:19.73 > System 2: 0:57.00 0:55.23 1:21.33 > System 3: 7:41.31 7:00.84 13:16.70 > > The new decoder appears slightly faster than dca even though it doesn't > use any assembly optimized code for dcadsp (besides synth_filter). Thanks for sharing these benchmarks. They look quite nice. ;-) Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
Hi, 2016-01-06 18:32 GMT+01:00 foo86 : > dca dca2libdcadec > System 1: 0:11.90 0:11.16 0:19.73 > System 2: 0:57.00 0:55.23 1:21.33 > System 3: 7:41.31 7:00.84 13:16.70 Just to be sure, because I won't check myself: is this an apple-to-apple comparison for dca vs dca2? I mean, things like favorable/native output format, eg without conversion, etc ? > The new decoder appears slightly faster than dca even though it doesn't > use any assembly optimized code for dcadsp (besides synth_filter). synth_filter was in my benchmarks (2? years ago, on a non-avx2 system with 5.1 core dts) around 50% of the execution time. It's good that it can be reused, but people have "suffered" using dca without it for near a decade, so they'll probably handle waiting another decade for it to be rewritten if it needs to be. But the other parts of dcadsp are probably contributing less than 10%, so dca2 looks good. And anyhow, seeing the enthusiastic support for your version, performance is probably secondary to a lot of people. -- Christophe ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On 1/6/2016 2:32 PM, foo86 wrote: > On Wed, Jan 06, 2016 at 12:28:00AM +0100, Hendrik Leppkes wrote: >> So that leaves us with a bunch of positive comments, on this side >> anyway, and noone opposed yet. >> >> Arguments for a switch include: >> - Nearly complete coverage of all DTS features, well tested and >> confirmed bit-exact (only DTS Express is missing, which is technically >> its own little codec using DTS EXSS headers) >> - Slightly faster (~5%) >> - Active maintainer >> - Andreas seal of security approval ;) >> >> If anyone thinks we should not replace our decoder, speak now or >> forever hold your peace (and bring proper arguments). >> I will try to do a code review to the best of my abilities in the upcoming >> days. >> >> foo86, could you work on changing the patch to replace the original instead? >> >> After it is merged, we could think about integrating your test-suite >> into the FATE system, but all in good time. > > OK, I'll start changing the patch. > > Some questions so far: > > 1. Should I remove old decoder files in separate commit first or should > I simply proceed with replacing entire content of certain files (e.g., > dcadec.c, dca_exss.c, dca_xll.c)? Yeah, send it as separate patches to make reviewing easier. Whoever commits it can then squash it into a single patch (Like it was done in commit b08569a2) if necessary. Also, while at it, try to split the main header into separate headers for some of the modules (xll, exss, dsp which already exists for the old decoder, etc). > > 2. Is it OK to leave arch-specific dca code as well as dcadsp.c > untouched for now? I'd really like to postpone dealing with that until > I'm done with generic code, especially since the new decoder is not > slower already. This means some assembly functions (ff_dca_lfe_fir32_*, > ff_dca_qmf_32_subbands_*) will still be compiled in but unused. > libavcodec/dcadsp.c will be still around but not compiled. Try to disable or remove code that will not be used. Git will easily let you recover any of it in the future if needed. Better than keeping dead code in the tree, IMO, but do what you think will make your work easier. > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
foo86 gmail.com> writes: > 1. Should I remove old decoder files in separate > commit first Please do that, it makes the diff much easier to read afaict, > 2. Is it OK to leave arch-specific dca code as > well as dcadsp.c untouched for now? Only if you believe that this makes your future work easier;-) Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On Wed, Jan 06, 2016 at 12:28:00AM +0100, Hendrik Leppkes wrote: > So that leaves us with a bunch of positive comments, on this side > anyway, and noone opposed yet. > > Arguments for a switch include: > - Nearly complete coverage of all DTS features, well tested and > confirmed bit-exact (only DTS Express is missing, which is technically > its own little codec using DTS EXSS headers) > - Slightly faster (~5%) > - Active maintainer > - Andreas seal of security approval ;) > > If anyone thinks we should not replace our decoder, speak now or > forever hold your peace (and bring proper arguments). > I will try to do a code review to the best of my abilities in the upcoming > days. > > foo86, could you work on changing the patch to replace the original instead? > > After it is merged, we could think about integrating your test-suite > into the FATE system, but all in good time. OK, I'll start changing the patch. Some questions so far: 1. Should I remove old decoder files in separate commit first or should I simply proceed with replacing entire content of certain files (e.g., dcadec.c, dca_exss.c, dca_xll.c)? 2. Is it OK to leave arch-specific dca code as well as dcadsp.c untouched for now? I'd really like to postpone dealing with that until I'm done with generic code, especially since the new decoder is not slower already. This means some assembly functions (ff_dca_lfe_fir32_*, ff_dca_qmf_32_subbands_*) will still be compiled in but unused. libavcodec/dcadsp.c will be still around but not compiled. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On Tue, Jan 05, 2016 at 10:46:19PM +0100, Andreas Cadhalpun wrote: > OK. This decoder seems to be quite robust in handling fuzzed samples, > so from a security point of view it should be fine to replace the > old dca decoder with this one. Out of interest, did you disable CRC checks in the decoder while fuzzing? Otherwise testing coverage will be decreased somewhat. The easiest way to do this is to modify ff_dca2_check_crc() to always return 0. > Out of curiosity: Can you post a few benchmarks comparing the performance > of the old and the new decoder? Here are some benchmarks I did with 3 available decoders on 3 different systems: (1) x86_64 desktop, (2) ancient i386 laptop, (3) Raspberry Pi Model B+. Measuring how much time it takes to loop 2000 times over a short 5.1 channel core sample (for a total duration of 00:51:56.22) yields the following results: dca dca2libdcadec System 1: 0:11.90 0:11.16 0:19.73 System 2: 0:57.00 0:55.23 1:21.33 System 3: 7:41.31 7:00.84 13:16.70 The new decoder appears slightly faster than dca even though it doesn't use any assembly optimized code for dcadsp (besides synth_filter). ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On Wed, Jan 6, 2016 at 4:01 AM, James Almer wrote: > On 1/5/2016 11:51 PM, Michael Niedermayer wrote: >> On Tue, Jan 05, 2016 at 11:44:04PM -0300, James Almer wrote: >>> On 1/5/2016 11:35 PM, Michael Niedermayer wrote: On Tue, Jan 05, 2016 at 11:27:25PM -0300, James Almer wrote: > On 1/5/2016 11:21 PM, Michael Niedermayer wrote: >> [...] > > Ideally, once this decoder is committed replacing the current one we'd > replace > the samples for the set available here: > https://github.com/foo86/dcadec-samples we can add new fate samples, we cannot replace fate samples replacing would break previous checkouts and releases not sure you actually meant to replace any ... >>> >>> Yeah, forgot about that. Kinda makes for a bloated fate suit in the long >>> run... >> >> this wouldnt happen if reference files where properly generated >> that is with some reference decoder or taken from the input to a >> reference encoder or from some referece test suite. >> >> for subtitles or other where files are tiny that is a non issue though >> ... > > Well, whoever made the xll test knew it would probably become obsolete at some > point, since the decoder was not bitexact when the pcm reference was created. > At least i assume that's why the test fails with this new bitexact decoder. If its properly bitexact now, we can use a md5 test instead of needing a reference sample, so that would safe us adding a new one. It would be nice to add all the tests from the dcadec test suite later to get more coverage, dca is slightly under-tested as it is. - Hendrik ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On 1/5/2016 11:51 PM, Michael Niedermayer wrote: > On Tue, Jan 05, 2016 at 11:44:04PM -0300, James Almer wrote: >> On 1/5/2016 11:35 PM, Michael Niedermayer wrote: >>> On Tue, Jan 05, 2016 at 11:27:25PM -0300, James Almer wrote: On 1/5/2016 11:21 PM, Michael Niedermayer wrote: > [...] >>> >>> Ideally, once this decoder is committed replacing the current one we'd replace the samples for the set available here: https://github.com/foo86/dcadec-samples >>> >>> we can add new fate samples, we cannot replace fate samples >>> replacing would break previous checkouts and releases >>> not sure you actually meant to replace any ... >> >> Yeah, forgot about that. Kinda makes for a bloated fate suit in the long >> run... > > this wouldnt happen if reference files where properly generated > that is with some reference decoder or taken from the input to a > reference encoder or from some referece test suite. > > for subtitles or other where files are tiny that is a non issue though > ... Well, whoever made the xll test knew it would probably become obsolete at some point, since the decoder was not bitexact when the pcm reference was created. At least i assume that's why the test fails with this new bitexact decoder. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On Tue, Jan 05, 2016 at 11:44:04PM -0300, James Almer wrote: > On 1/5/2016 11:35 PM, Michael Niedermayer wrote: > > On Tue, Jan 05, 2016 at 11:27:25PM -0300, James Almer wrote: > >> On 1/5/2016 11:21 PM, Michael Niedermayer wrote: [...] > > > > > >> > >> Ideally, once this decoder is committed replacing the current one we'd > >> replace > >> the samples for the set available here: > >> https://github.com/foo86/dcadec-samples > > > > we can add new fate samples, we cannot replace fate samples > > replacing would break previous checkouts and releases > > not sure you actually meant to replace any ... > > Yeah, forgot about that. Kinda makes for a bloated fate suit in the long > run... this wouldnt happen if reference files where properly generated that is with some reference decoder or taken from the input to a reference encoder or from some referece test suite. for subtitles or other where files are tiny that is a non issue though ... [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is dangerous to be right in matters on which the established authorities are wrong. -- Voltaire signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On 1/5/2016 11:35 PM, Michael Niedermayer wrote: > On Tue, Jan 05, 2016 at 11:27:25PM -0300, James Almer wrote: >> On 1/5/2016 11:21 PM, Michael Niedermayer wrote: >>> On Tue, Jan 05, 2016 at 11:38:00PM +0300, foo86 wrote: On Tue, Jan 05, 2016 at 08:45:22PM +0100, Andreas Cadhalpun wrote: > On 03.01.2016 18:49, foo86 wrote: >> +// 5.3.1 - Bit stream header >> +static int parse_frame_header(DCA2CoreDecoder *s) >> +{ > [...] >> +// Source PCM resolution >> +s->source_pcm_res = ff_dca_bits_per_sample[pcmr_index = >> get_bits(&s->gb, 3)]; > > This can cause an out-of-bounds read if get_bits returns 7, because > ff_dca_bits_per_sample > only has 7 elements. Fixed locally, thanks. P.S. To avoid resending this huge patch, I've put the fixes accumulated so far in a private dcadec2 branch on github [1] (will be rebased frequently against FFmpeg master). >>> [1]: https://github.com/foo86/FFmpeg/tree/dcadec2 >>> >>> breaks "make fate", something needs to be updated >>> or a new reference sample uploaded if teh one we have is wrong >>> >>> stddev: 297.72 PSNR: 46.85 MAXDIFF: 3474 bytes: 8994816/ 9601024 >>> MAXDIFF: |3474 - 0| >= 1 >>> size: |8994816 - 9601024| >= 0 >>> Test dca-xll failed. Look at tests/data/fate/dca-xll.err for details. >>> make: *** [fate-dca-xll] Error 1 >>> make: *** Waiting for unfinished jobs >> >> Was this run using foo86's decoder, or the current one? If the former then >> it's >> not unexpected since the xll test was made for the current decoder, which is >> not >> bitexact. > > i just locally merged the branch and did a make -j12 fate I see dca2 is above dca in allcodecs.c on this patch so i guess it's using the former, which would explain why it fails. > > >> >> Ideally, once this decoder is committed replacing the current one we'd >> replace >> the samples for the set available here: >> https://github.com/foo86/dcadec-samples > > we can add new fate samples, we cannot replace fate samples > replacing would break previous checkouts and releases > not sure you actually meant to replace any ... Yeah, forgot about that. Kinda makes for a bloated fate suit in the long run... Since the lossy tests (fate-dca-core and fate-dts_es) apparently still work with this decoder then there's no need to remove them, but the new extensions still need the above sample suit. > > [...] > > > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On Tue, Jan 05, 2016 at 11:27:25PM -0300, James Almer wrote: > On 1/5/2016 11:21 PM, Michael Niedermayer wrote: > > On Tue, Jan 05, 2016 at 11:38:00PM +0300, foo86 wrote: > >> On Tue, Jan 05, 2016 at 08:45:22PM +0100, Andreas Cadhalpun wrote: > >>> On 03.01.2016 18:49, foo86 wrote: > +// 5.3.1 - Bit stream header > +static int parse_frame_header(DCA2CoreDecoder *s) > +{ > >>> [...] > +// Source PCM resolution > +s->source_pcm_res = ff_dca_bits_per_sample[pcmr_index = > get_bits(&s->gb, 3)]; > >>> > >>> This can cause an out-of-bounds read if get_bits returns 7, because > >>> ff_dca_bits_per_sample > >>> only has 7 elements. > >> > >> Fixed locally, thanks. > >> > >> P.S. To avoid resending this huge patch, I've put the fixes accumulated > >> so far in a private dcadec2 branch on github [1] (will be rebased > >> frequently against FFmpeg master). > >> > > > >> [1]: https://github.com/foo86/FFmpeg/tree/dcadec2 > > > > breaks "make fate", something needs to be updated > > or a new reference sample uploaded if teh one we have is wrong > > > > stddev: 297.72 PSNR: 46.85 MAXDIFF: 3474 bytes: 8994816/ 9601024 > > MAXDIFF: |3474 - 0| >= 1 > > size: |8994816 - 9601024| >= 0 > > Test dca-xll failed. Look at tests/data/fate/dca-xll.err for details. > > make: *** [fate-dca-xll] Error 1 > > make: *** Waiting for unfinished jobs > > Was this run using foo86's decoder, or the current one? If the former then > it's > not unexpected since the xll test was made for the current decoder, which is > not > bitexact. i just locally merged the branch and did a make -j12 fate > > Ideally, once this decoder is committed replacing the current one we'd replace > the samples for the set available here: > https://github.com/foo86/dcadec-samples we can add new fate samples, we cannot replace fate samples replacing would break previous checkouts and releases not sure you actually meant to replace any ... [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB Dictatorship: All citizens are under surveillance, all their steps and actions recorded, for the politicians to enforce control. Democracy: All politicians are under surveillance, all their steps and actions recorded, for the citizens to enforce control. signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On 1/5/2016 11:21 PM, Michael Niedermayer wrote: > On Tue, Jan 05, 2016 at 11:38:00PM +0300, foo86 wrote: >> On Tue, Jan 05, 2016 at 08:45:22PM +0100, Andreas Cadhalpun wrote: >>> On 03.01.2016 18:49, foo86 wrote: +// 5.3.1 - Bit stream header +static int parse_frame_header(DCA2CoreDecoder *s) +{ >>> [...] +// Source PCM resolution +s->source_pcm_res = ff_dca_bits_per_sample[pcmr_index = get_bits(&s->gb, 3)]; >>> >>> This can cause an out-of-bounds read if get_bits returns 7, because >>> ff_dca_bits_per_sample >>> only has 7 elements. >> >> Fixed locally, thanks. >> >> P.S. To avoid resending this huge patch, I've put the fixes accumulated >> so far in a private dcadec2 branch on github [1] (will be rebased >> frequently against FFmpeg master). >> > >> [1]: https://github.com/foo86/FFmpeg/tree/dcadec2 > > breaks "make fate", something needs to be updated > or a new reference sample uploaded if teh one we have is wrong > > stddev: 297.72 PSNR: 46.85 MAXDIFF: 3474 bytes: 8994816/ 9601024 > MAXDIFF: |3474 - 0| >= 1 > size: |8994816 - 9601024| >= 0 > Test dca-xll failed. Look at tests/data/fate/dca-xll.err for details. > make: *** [fate-dca-xll] Error 1 > make: *** Waiting for unfinished jobs Was this run using foo86's decoder, or the current one? If the former then it's not unexpected since the xll test was made for the current decoder, which is not bitexact. Ideally, once this decoder is committed replacing the current one we'd replace the samples for the set available here: https://github.com/foo86/dcadec-samples ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On Tue, Jan 05, 2016 at 11:38:00PM +0300, foo86 wrote: > On Tue, Jan 05, 2016 at 08:45:22PM +0100, Andreas Cadhalpun wrote: > > On 03.01.2016 18:49, foo86 wrote: > > > +// 5.3.1 - Bit stream header > > > +static int parse_frame_header(DCA2CoreDecoder *s) > > > +{ > > [...] > > > +// Source PCM resolution > > > +s->source_pcm_res = ff_dca_bits_per_sample[pcmr_index = > > > get_bits(&s->gb, 3)]; > > > > This can cause an out-of-bounds read if get_bits returns 7, because > > ff_dca_bits_per_sample > > only has 7 elements. > > Fixed locally, thanks. > > P.S. To avoid resending this huge patch, I've put the fixes accumulated > so far in a private dcadec2 branch on github [1] (will be rebased > frequently against FFmpeg master). > > [1]: https://github.com/foo86/FFmpeg/tree/dcadec2 breaks "make fate", something needs to be updated or a new reference sample uploaded if teh one we have is wrong stddev: 297.72 PSNR: 46.85 MAXDIFF: 3474 bytes: 8994816/ 9601024 MAXDIFF: |3474 - 0| >= 1 size: |8994816 - 9601024| >= 0 Test dca-xll failed. Look at tests/data/fate/dca-xll.err for details. make: *** [fate-dca-xll] Error 1 make: *** Waiting for unfinished jobs [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is what and why we do it that matters, not just one of them. signature.asc Description: Digital signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On Tue, Jan 5, 2016 at 3:42 PM, Hendrik Leppkes wrote: > On Wed, Jan 6, 2016 at 12:40 AM, Ganesh Ajjanagadde wrote: [...] >> For now, I definitely think we should replace our decoder. >> Just a clarification: in the long run, isn't it a good idea to get >> this into the repo and not use an external library? For instance, if >> and when asm code gets written for this, isn't it easier to work >> within FFmpeg's codebase, which has some infrastructure for writing >> asm? >> > > The whole idea of this patch is to integrate the decoder fully into > our codebase, and no longer use the external library. I see, sorry for the confusion and noise. [...] ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On Wed, Jan 6, 2016 at 12:40 AM, Ganesh Ajjanagadde wrote: > On Tue, Jan 5, 2016 at 3:28 PM, Hendrik Leppkes wrote: >> On Tue, Jan 5, 2016 at 10:46 PM, Andreas Cadhalpun >> wrote: >>> On 05.01.2016 21:38, foo86 wrote: On Tue, Jan 05, 2016 at 08:45:22PM +0100, Andreas Cadhalpun wrote: > On 03.01.2016 18:49, foo86 wrote: >> +// 5.3.1 - Bit stream header >> +static int parse_frame_header(DCA2CoreDecoder *s) >> +{ > [...] >> +// Source PCM resolution >> +s->source_pcm_res = ff_dca_bits_per_sample[pcmr_index = >> get_bits(&s->gb, 3)]; > > This can cause an out-of-bounds read if get_bits returns 7, because > ff_dca_bits_per_sample > only has 7 elements. Fixed locally, thanks. >>> >>> Thanks. >>> P.S. To avoid resending this huge patch, I've put the fixes accumulated so far in a private dcadec2 branch on github [1] (will be rebased frequently against FFmpeg master). [1]: https://github.com/foo86/FFmpeg/tree/dcadec2 >>> >>> OK. This decoder seems to be quite robust in handling fuzzed samples, >>> so from a security point of view it should be fine to replace the >>> old dca decoder with this one. >>> >> >> >> So that leaves us with a bunch of positive comments, on this side >> anyway, and noone opposed yet. >> >> Arguments for a switch include: >> - Nearly complete coverage of all DTS features, well tested and >> confirmed bit-exact (only DTS Express is missing, which is technically >> its own little codec using DTS EXSS headers) >> - Slightly faster (~5%) >> - Active maintainer >> - Andreas seal of security approval ;) >> >> If anyone thinks we should not replace our decoder, speak now or >> forever hold your peace (and bring proper arguments). >> I will try to do a code review to the best of my abilities in the upcoming >> days. > > For now, I definitely think we should replace our decoder. > Just a clarification: in the long run, isn't it a good idea to get > this into the repo and not use an external library? For instance, if > and when asm code gets written for this, isn't it easier to work > within FFmpeg's codebase, which has some infrastructure for writing > asm? > The whole idea of this patch is to integrate the decoder fully into our codebase, and no longer use the external library. PS: What I forgot to mention in my last mail, foo86 if you have any questions about ripping out the old dca decoder, feel free to contact me. - Hendrik ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On Tue, Jan 5, 2016 at 3:28 PM, Hendrik Leppkes wrote: > On Tue, Jan 5, 2016 at 10:46 PM, Andreas Cadhalpun > wrote: >> On 05.01.2016 21:38, foo86 wrote: >>> On Tue, Jan 05, 2016 at 08:45:22PM +0100, Andreas Cadhalpun wrote: On 03.01.2016 18:49, foo86 wrote: > +// 5.3.1 - Bit stream header > +static int parse_frame_header(DCA2CoreDecoder *s) > +{ [...] > +// Source PCM resolution > +s->source_pcm_res = ff_dca_bits_per_sample[pcmr_index = > get_bits(&s->gb, 3)]; This can cause an out-of-bounds read if get_bits returns 7, because ff_dca_bits_per_sample only has 7 elements. >>> >>> Fixed locally, thanks. >> >> Thanks. >> >>> P.S. To avoid resending this huge patch, I've put the fixes accumulated >>> so far in a private dcadec2 branch on github [1] (will be rebased >>> frequently against FFmpeg master). >>> >>> [1]: https://github.com/foo86/FFmpeg/tree/dcadec2 >> >> OK. This decoder seems to be quite robust in handling fuzzed samples, >> so from a security point of view it should be fine to replace the >> old dca decoder with this one. >> > > > So that leaves us with a bunch of positive comments, on this side > anyway, and noone opposed yet. > > Arguments for a switch include: > - Nearly complete coverage of all DTS features, well tested and > confirmed bit-exact (only DTS Express is missing, which is technically > its own little codec using DTS EXSS headers) > - Slightly faster (~5%) > - Active maintainer > - Andreas seal of security approval ;) > > If anyone thinks we should not replace our decoder, speak now or > forever hold your peace (and bring proper arguments). > I will try to do a code review to the best of my abilities in the upcoming > days. For now, I definitely think we should replace our decoder. Just a clarification: in the long run, isn't it a good idea to get this into the repo and not use an external library? For instance, if and when asm code gets written for this, isn't it easier to work within FFmpeg's codebase, which has some infrastructure for writing asm? > > foo86, could you work on changing the patch to replace the original instead? > > After it is merged, we could think about integrating your test-suite > into the FATE system, but all in good time. > > - Hendrik > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On Tue, Jan 5, 2016 at 10:46 PM, Andreas Cadhalpun wrote: > On 05.01.2016 21:38, foo86 wrote: >> On Tue, Jan 05, 2016 at 08:45:22PM +0100, Andreas Cadhalpun wrote: >>> On 03.01.2016 18:49, foo86 wrote: +// 5.3.1 - Bit stream header +static int parse_frame_header(DCA2CoreDecoder *s) +{ >>> [...] +// Source PCM resolution +s->source_pcm_res = ff_dca_bits_per_sample[pcmr_index = get_bits(&s->gb, 3)]; >>> >>> This can cause an out-of-bounds read if get_bits returns 7, because >>> ff_dca_bits_per_sample >>> only has 7 elements. >> >> Fixed locally, thanks. > > Thanks. > >> P.S. To avoid resending this huge patch, I've put the fixes accumulated >> so far in a private dcadec2 branch on github [1] (will be rebased >> frequently against FFmpeg master). >> >> [1]: https://github.com/foo86/FFmpeg/tree/dcadec2 > > OK. This decoder seems to be quite robust in handling fuzzed samples, > so from a security point of view it should be fine to replace the > old dca decoder with this one. > So that leaves us with a bunch of positive comments, on this side anyway, and noone opposed yet. Arguments for a switch include: - Nearly complete coverage of all DTS features, well tested and confirmed bit-exact (only DTS Express is missing, which is technically its own little codec using DTS EXSS headers) - Slightly faster (~5%) - Active maintainer - Andreas seal of security approval ;) If anyone thinks we should not replace our decoder, speak now or forever hold your peace (and bring proper arguments). I will try to do a code review to the best of my abilities in the upcoming days. foo86, could you work on changing the patch to replace the original instead? After it is merged, we could think about integrating your test-suite into the FATE system, but all in good time. - Hendrik ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On 05.01.2016 21:38, foo86 wrote: > On Tue, Jan 05, 2016 at 08:45:22PM +0100, Andreas Cadhalpun wrote: >> On 03.01.2016 18:49, foo86 wrote: >>> +// 5.3.1 - Bit stream header >>> +static int parse_frame_header(DCA2CoreDecoder *s) >>> +{ >> [...] >>> +// Source PCM resolution >>> +s->source_pcm_res = ff_dca_bits_per_sample[pcmr_index = >>> get_bits(&s->gb, 3)]; >> >> This can cause an out-of-bounds read if get_bits returns 7, because >> ff_dca_bits_per_sample >> only has 7 elements. > > Fixed locally, thanks. Thanks. > P.S. To avoid resending this huge patch, I've put the fixes accumulated > so far in a private dcadec2 branch on github [1] (will be rebased > frequently against FFmpeg master). > > [1]: https://github.com/foo86/FFmpeg/tree/dcadec2 OK. This decoder seems to be quite robust in handling fuzzed samples, so from a security point of view it should be fine to replace the old dca decoder with this one. Out of curiosity: Can you post a few benchmarks comparing the performance of the old and the new decoder? Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On Tue, Jan 05, 2016 at 08:45:22PM +0100, Andreas Cadhalpun wrote: > On 03.01.2016 18:49, foo86 wrote: > > +// 5.3.1 - Bit stream header > > +static int parse_frame_header(DCA2CoreDecoder *s) > > +{ > [...] > > +// Source PCM resolution > > +s->source_pcm_res = ff_dca_bits_per_sample[pcmr_index = > > get_bits(&s->gb, 3)]; > > This can cause an out-of-bounds read if get_bits returns 7, because > ff_dca_bits_per_sample > only has 7 elements. Fixed locally, thanks. P.S. To avoid resending this huge patch, I've put the fixes accumulated so far in a private dcadec2 branch on github [1] (will be rebased frequently against FFmpeg master). [1]: https://github.com/foo86/FFmpeg/tree/dcadec2 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On 03.01.2016 18:49, foo86 wrote: > +// 5.3.1 - Bit stream header > +static int parse_frame_header(DCA2CoreDecoder *s) > +{ [...] > +// Source PCM resolution > +s->source_pcm_res = ff_dca_bits_per_sample[pcmr_index = get_bits(&s->gb, > 3)]; This can cause an out-of-bounds read if get_bits returns 7, because ff_dca_bits_per_sample only has 7 elements. Best regards, Andreas ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On Sun, Jan 03, 2016 at 07:14:28PM +0100, Hendrik Leppkes wrote: > Having two dca decoders with varying degrees of features is no > advantages, and considering the native decoder lacks a long list of > features and fails decoding a bunch of XLL streams, the aim should be > to replace. > I'm not aware of a single advantage of the old decoder, so... Having both decoders available during development makes things like comparing their output and performance a bit simpler, but I agree that keeping both decoders doesn't make much sense in the end. The final version of the patch should probably replace the old decoder. This will offer some opportunity to clean up dcadata.c, etc. > I have been using libdcadec through the library wrapper in ffmpeg for > a while now, and I am not aware of any file that doesn't decode > properly today, so yes, I tend to agree. > Other people will probably want to apply fuzz testing, I'm not sure > how much you have done to this degree. I did a few manual runs with valgrind/ubsan and zzuf to make sure the decoder doesn't do anything particularly stupid, but no real stress testing yet. > Trying to get some extra functionality into SIMD-able functions would > be a good goal for later, but even the previous decoder doesn't have > that much. > So something for later. If someone hinted what parts of code are easily optimizable with SIMD I could later provide DSP hooks for it. > Out of curiousity, do you plan to continue developing the external > library, or would you abandon it if its merged? > And more directly, I would hope we can also count on basic maintenance > of the new decoder in ffmpeg? :) I've implemented basically everything I wanted in external library, so I don't really expect any more substanial development, just the bug fixes. Regaring the new decoder in FFmpeg, I'll try to be around, assuming nothing bad happens to me :) ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
James Almer gmail.com> writes: > But by all means lets not have another prores > or asf/wmv demuxer situation. The asf situation can be fixed anytime, nobody is working on the broken demuxer... Carl Eugen ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
>IMO this should replace the current dca decoder. Based on what i saw from libdcadec >yours is cleaner, more feature complete, you're a knowledgeable maintainer, and >since the current developer for ffdca over at libav is porting code from yours to >get bitexact xll working the code duplication would be considerable if we keep both. I agree. On 3 January 2016 at 18:24, James Almer wrote: > On 1/3/2016 2:49 PM, foo86 wrote: > > This is initial version of libdcadec DCA decoder port I hacked together > over > > the last week. This patch is of RFC/inquiry type and not meant for > immediate > > inclusion. > > > > Any comments about the code are appreciated, as well as suggestions > concerning > > how should I proceed with this patch. Should I aim for including it as a > > separate decoder or a replacement for existing DCA decoder? Or it might > be that > > you prefer to continue improving existing decoder instead and not merge > this at > > all. > > IMO this should replace the current dca decoder. Based on what i saw from > libdcadec > yours is cleaner, more feature complete, you're a knowledgeable > maintainer, and > since the current developer for ffdca over at libav is porting code from > yours to > get bitexact xll working the code duplication would be considerable if we > keep both. > > > > > Attached decoder is feature complete enough to fully decode any > practically > > available DTS stream configuration. It supports any possible > combinations of > > XCH, XXCH, XBR and X96 core extensions to deliver up to 7.1 ch, 96 kHz > floating > > point output. It also implements lossless decoding of XLL extension to > deliver > > up to 7.1 ch, 192 kHz fixed point output. Bit rate managed XLL streams > are > > supported. Downmixing to 5.1 and stereo using embedded coefficients is > > supported. > > > > Performance wise, this decoder used to be around 10% slower than pure > floating > > point version of existing native DCA decoder. However, after native DCA > decoder > > has been partially converted to fixed point, this decoder became 5% > faster than > > native one. Measurements were done looping over 5.1 core only stream in > > Another reason to have this replace the current one, then. > > Thanks. > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On 1/3/2016 3:44 PM, wm4 wrote: > On Sun, 3 Jan 2016 20:49:10 +0300 > foo86 wrote: > >> This is initial version of libdcadec DCA decoder port I hacked together over >> the last week. This patch is of RFC/inquiry type and not meant for immediate >> inclusion. >> >> Any comments about the code are appreciated, as well as suggestions >> concerning >> how should I proceed with this patch. Should I aim for including it as a >> separate decoder or a replacement for existing DCA decoder? Or it might be >> that >> you prefer to continue improving existing decoder instead and not merge this >> at >> all. > > Maybe we could have a vote whether or not the existing builtin decoder > should be replaced with this? I think calling for a vote would make sense if people start commenting against it. So far nobody did, but in any case there's time to discuss. Keep in mind there are not many things, if any, in favor of the current decoder, especially now that it stopped being faster. There's no maintainer for it working on ffmpeg side of things, has less features, and the current developer on libav is essentially taking bits from this one to catch up on features. But by all means lets not have another prores or asf/wmv demuxer situation. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On Sun, 3 Jan 2016 20:49:10 +0300 foo86 wrote: > This is initial version of libdcadec DCA decoder port I hacked together over > the last week. This patch is of RFC/inquiry type and not meant for immediate > inclusion. > > Any comments about the code are appreciated, as well as suggestions concerning > how should I proceed with this patch. Should I aim for including it as a > separate decoder or a replacement for existing DCA decoder? Or it might be > that > you prefer to continue improving existing decoder instead and not merge this > at > all. Maybe we could have a vote whether or not the existing builtin decoder should be replaced with this? ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On 1/3/2016 2:49 PM, foo86 wrote: > This is initial version of libdcadec DCA decoder port I hacked together over > the last week. This patch is of RFC/inquiry type and not meant for immediate > inclusion. > > Any comments about the code are appreciated, as well as suggestions concerning > how should I proceed with this patch. Should I aim for including it as a > separate decoder or a replacement for existing DCA decoder? Or it might be > that > you prefer to continue improving existing decoder instead and not merge this > at > all. IMO this should replace the current dca decoder. Based on what i saw from libdcadec yours is cleaner, more feature complete, you're a knowledgeable maintainer, and since the current developer for ffdca over at libav is porting code from yours to get bitexact xll working the code duplication would be considerable if we keep both. > > Attached decoder is feature complete enough to fully decode any practically > available DTS stream configuration. It supports any possible combinations of > XCH, XXCH, XBR and X96 core extensions to deliver up to 7.1 ch, 96 kHz > floating > point output. It also implements lossless decoding of XLL extension to deliver > up to 7.1 ch, 192 kHz fixed point output. Bit rate managed XLL streams are > supported. Downmixing to 5.1 and stereo using embedded coefficients is > supported. > > Performance wise, this decoder used to be around 10% slower than pure floating > point version of existing native DCA decoder. However, after native DCA > decoder > has been partially converted to fixed point, this decoder became 5% faster > than > native one. Measurements were done looping over 5.1 core only stream in Another reason to have this replace the current one, then. Thanks. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.
On Sun, Jan 3, 2016 at 6:49 PM, foo86 wrote: > This is initial version of libdcadec DCA decoder port I hacked together over > the last week. This patch is of RFC/inquiry type and not meant for immediate > inclusion. > > Any comments about the code are appreciated, as well as suggestions concerning > how should I proceed with this patch. Should I aim for including it as a > separate decoder or a replacement for existing DCA decoder? Or it might be > that > you prefer to continue improving existing decoder instead and not merge this > at > all. Having two dca decoders with varying degrees of features is no advantages, and considering the native decoder lacks a long list of features and fails decoding a bunch of XLL streams, the aim should be to replace. I'm not aware of a single advantage of the old decoder, so... > > Attached decoder is feature complete enough to fully decode any practically > available DTS stream configuration. It supports any possible combinations of > XCH, XXCH, XBR and X96 core extensions to deliver up to 7.1 ch, 96 kHz > floating > point output. It also implements lossless decoding of XLL extension to deliver > up to 7.1 ch, 192 kHz fixed point output. Bit rate managed XLL streams are > supported. Downmixing to 5.1 and stereo using embedded coefficients is > supported. I have been using libdcadec through the library wrapper in ffmpeg for a while now, and I am not aware of any file that doesn't decode properly today, so yes, I tend to agree. Other people will probably want to apply fuzz testing, I'm not sure how much you have done to this degree. > > Performance wise, this decoder used to be around 10% slower than pure floating > point version of existing native DCA decoder. However, after native DCA > decoder > has been partially converted to fixed point, this decoder became 5% faster > than > native one. Measurements were done looping over 5.1 core only stream in > floating point mode on a x86_64 system. Of course, decoding extensions like > X96 > and XLL makes the decoder much slower. These can be ignored with -core_only > option. Trying to get some extra functionality into SIMD-able functions would be a good goal for later, but even the previous decoder doesn't have that much. So something for later. > > The libdcadec code this decoder is based on has been rather well tested and > stable, thus I don't expect any fundamental issues with this new decoder. I > might have introduced some bugs during porting; these should be hopefully > found > by regression tests and fixed. Note that I've not run this decoder through any > serious regression testing yet, I just checked that it works on several select > samples. > > This decoder depends on assembly versions of existing native dcadsp, however > it > only uses assembly optimized version of synth_filter currently. Because > synth_filter implicitly depends on dcadsp this is required to allow decoder to > build when existing DCA decoder is disabled. I plan to overhaul this area > (interacting with existing dcadsp) later. > Out of curiousity, do you plan to continue developing the external library, or would you abandon it if its merged? And more directly, I would hope we can also count on basic maintenance of the new decoder in ffmpeg? :) - Hendrik ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel