[FFmpeg-devel] [RFC] Direct Stream Transfer (DST) decoder

[Peter Ross]

Thanks for the review (also thanks compn).
I will address all your comments -- some further questions below:

On Tue, Sep 30, 2014 at 09:00:12PM +0200, Reimar Döffinger wrote:
> On Tue, Sep 30, 2014 at 11:42:20PM +1000, Peter Ross wrote:
> > +/**
> > + * Calculate FS44 ratio
> > + */
> > +#define DSD_FS44(sample_rate) (sample_rate / 44100)
> > +
> > +/**
> > + * Calculate DST frame size
> > + * @return samples per frame (1-bit samples)
> > + */
> > +#define DST_SAMPLES_PER_FRAME(sample_rate) (588 * DSD_FS44(sample_rate))
> 
> A header for just these seems a bit overkill.
> Also I'd prefer static inline functions over macros when there is no
> reason to use macros.

Agree. These are used by the DSDIFF/DST demuxer (and my experimental DST encoder.)

> > +            for (k = 0; k < 256; k++) {
> > +                int v = 0;
> > +                for (l = 0; l < total; l++)
> > +                    v += (((k >> l) & 1) * 2 - 1) * fsets->coeff[i][j * 8 + l];
> 
> Is this faster in a relevant way than something more readable like
> coeff = fsets->coeff[i][j * 8 + l];
> v += k & (1 << l) ? coeff : -coeff;
> ?

Faster!

> > +    unsigned int half_prob[DST_MAX_CHANNELS];
> > +    unsigned int map_ch_to_felem[DST_MAX_CHANNELS];
> > +    unsigned int map_ch_to_pelem[DST_MAX_CHANNELS];
> > +    DECLARE_ALIGNED(16, uint8_t, status)[DST_MAX_CHANNELS][16];
> > +    DECLARE_ALIGNED(16, int16_t, filter)[DST_MAX_ELEMENTS][16][256];
> 
> At least the last one is quite large I think.

Other structures (not shown above) are also large, so I have moved these into the context.

> Please consider putting it in the context or so instead.
> That's also less problematic with alignment.

The 'filter' array is ~100 kilobytes ((6ch x 2) x 16 x 256 x sizeof(int16_t)).
It is a lookup table, and is used _a lot_.
When I move this into the context struct, decoder performance drops 15%.

What is a reasonable size for on-stack variables?

> > +    if ((ret = init_get_bits8(&gb, avpkt->data, avpkt->size)) < 0)
> > +    if ((ret = read_map(&gb, &fsets, map_ch_to_felem, avctx->channels)) < 0)
> > +        if ((ret = read_map(&gb, &probs, map_ch_to_pelem, avctx->channels)) < 0)
> 
> I still think putting assignments into an if is really bad idea all
> around.

No exception for simple error return codes?

I will post the updated decoder soon, along with AV_SAMPLE_FMT_DSD patches :)

-- Peter
(A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20141007/f9fcddd0/attachment.asc>