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)
signature.asc
Description: Digital signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
