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

Reimar Döffinger Reimar.Doeffinger at gmx.de
Thu Oct 9 08:13:21 CEST 2014

On 7 October 2014 08:41:09 CEST, Peter Ross <pross at xvid.org> wrote:
>> > +            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;
>> ?

Ok, I thought this part of the code would not be performance relevant.
I also didn't like that it probably isn't going to be very fast on hardware with slow multipliers.
But I don't have a better suggestion right now.

>> 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
>It is a lookup table, and is used _a lot_.
>When I move this into the context struct, decoder performance drops

It would be good to know why it is slower...

>What is a reasonable size for on-stack variables?

We have no hard rules.
I would prefer if we could support old OpenBSD, which means an overall stack size of 64kB, and you need to leave some space for the application's stack.
Otherwise I'd put the limit at around 200-500 kB (but note: for a whole call stack on FFmpeg side), above that I think our stack usage risks being a significant cause for crashes.
As a library we should try to use only a rather minor portion of the overall available stack (8MB on Linux, but a bit less on other OS).

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

As Micheal said it's used a lot so no reason for rejecting a patch.
However we had loads of bugs due to it, it is hard to read especially for people not that used to C and it only saves a single line.
To me personally that's still a completely unreasonable risk/benefit ratio.
Normally when we see a feature that is a repeated cause of bugs we avoid it. For some reason people are too attached to this one specifically.
I still argue against it because I still believe it makes no sense to use it.
Just my opinion though.

More information about the ffmpeg-devel mailing list