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

[Michael Niedermayer]

On Tue, Oct 07, 2014 at 05:41:09PM +1100, Peter Ross wrote:
> Thanks for the review (also thanks compn).
> I will address all your comments -- some further questions below:

[...]

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

it might be worth taking a quick look at te generated asm code,
for the 2 cases maybe gcc does something silly


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

if ((ret = func()) < 0)

is used alot in existing code so IMHO theres no problem, the main
risk is getting the () wrong, which did happens a few times over the
years ...

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire
-------------- 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/fc7a24ac/attachment.asc>