[FFmpeg-devel] [PATCH 5/6] truehd: break out part of output_data into platform-specific callback.

[Ben Avison]

On Wed, 19 Mar 2014 18:27:54 -0000, James Almer <jamrial at gmail.com> wrote:
>> +    int32_t (*mlp_pack_output)(int32_t lossless_check_data,
>> +                               int32_t (*sample_buffer)[MAX_CHANNELS],
>> +                               void *data,
>> +                               uint16_t blockpos,
>> +                               uint8_t max_matrix_channel,
>> +                               int is32,
>> +                               uint8_t *ch_assign,
>> +                               int8_t *output_shift);
>>  } MLPDSPContext;
>
> As i said elsewhere, please put pointers first if possible, like you did
> for mlp_rematrix_channel.

OK, some reordering is possible, but putting all the pointers first would
add some cycles. Some of the ordering was a side-effect of how it was
convenient to write the assembly, although obviously what's convenient
will vary from platform to platform. In particular:

* lossless_check_data is a kind of accumulator: it's passed in, processed
   a bit, and passed out as the function return value. Having it as the
   first argument means no register shuffling is needed at runtime.

* max_matrix_channel, is32, ch_assign, output_shift: I don't particularly
   care about the relative order of these amongst themselves. However, 3
   or 4 of them are actually unused by all the individual implementations
   of the function. Having them at the 5th and subsequent arguments means
   (with the ARM ABI) that they're on the stack, so for each of the
   unused arguments, we save a word load. If lossless_check_data and
   blockpos came after all the pointers, they'd both be on the stack
   instead, and they would need loading on every call.

Would an order like

int32_t lossless_check_data,
int32_t (*sample_buffer)[MAX_CHANNELS],
void *data,
uint16_t blockpos,
uint8_t *ch_assign,
int8_t *output_shift,
uint8_t max_matrix_channel,
int is32

be more acceptable? Or perhaps

int32_t lossless_check_data,
uint16_t blockpos,
int32_t (*sample_buffer)[MAX_CHANNELS],
void *data,
uint8_t *ch_assign,
int8_t *output_shift,
uint8_t max_matrix_channel,
int is32

where all the pointers are grouped together?

Ben