Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.

[foo86]

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.

[foo86]

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.

[Andreas Cadhalpun]

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.

[Andreas Cadhalpun]

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

[foo86]

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.

[foo86]

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(>gb, 5) + 1;
> > +
> > +for (j = 0; j < nspeakers[i]; j++) {
> > +// Decoded channels to output speaker mapping mask
> > +int remap_ch_mask = get_bits(>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(>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.

[foo86]

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.

[Christophe Gisquet]

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.

[Andreas Cadhalpun]

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.

[Andreas Cadhalpun]

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(>gb, 5) + 1;
> +
> +for (j = 0; j < nspeakers[i]; j++) {
> +// Decoded channels to output speaker mapping mask
> +int remap_ch_mask = get_bits(>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(>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.

[Hendrik Leppkes]

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.

[foo86]

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.

[Carl Eugen Hoyos]

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.

[James Almer]

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.

[Christophe Gisquet]

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.

[foo86]

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.

[Andreas Cadhalpun]

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.

[foo86]

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

[Michael Niedermayer]

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

[James Almer]

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

[Michael Niedermayer]

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.

[James Almer]

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.

[Andreas Cadhalpun]

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

[Andreas Cadhalpun]

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

[Hendrik Leppkes]

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

[Hendrik Leppkes]

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

[Ganesh Ajjanagadde]

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.

[Ganesh Ajjanagadde]

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

[Michael Niedermayer]

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

[James Almer]

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

[Hendrik Leppkes]

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

Re: [FFmpeg-devel] [RFC] avcodec: Add native DCA decoder based on libdcadec.

[James Almer]

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.

[wm4]

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.

[James Almer]

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.

[Rostislav Pehlivanov]

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

[Carl Eugen Hoyos]

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.

[foo86]

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