[FFmpeg-cvslog] alsdec: channel sorting

[Thilo Borgmann]

Am 21.12.12 15:51, schrieb Paul B Mahol:
> ffmpeg | branch: master | Paul B Mahol <onemda at gmail.com> | Fri Dec 21 12:51:08 2012 +0000| [ae27b70b9d9dfb0c1e09b6dfcd2fd74fffc5bbac] | committer: Paul B Mahol
> 
> alsdec: channel sorting
> 
> Signed-off-by: Paul B Mahol <onemda at gmail.com>
> 
>> http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=ae27b70b9d9dfb0c1e09b6dfcd2fd74fffc5bbac
> ---
> 
>  libavcodec/alsdec.c |   16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/alsdec.c b/libavcodec/alsdec.c
> index 1f725a1..7133bfd 100644
> --- a/libavcodec/alsdec.c
> +++ b/libavcodec/alsdec.c
> @@ -355,11 +355,16 @@ static av_cold int read_specific_config(ALSDecContext *ctx)
>          if (!(sconf->chan_pos = av_malloc(avctx->channels * sizeof(*sconf->chan_pos))))
>              return AVERROR(ENOMEM);
>  
> -        for (i = 0; i < avctx->channels; i++)
> +        for (i = 0; i < avctx->channels; i++) {
>              sconf->chan_pos[i] = get_bits(&gb, chan_pos_bits);
> +            if (sconf->chan_pos[i] >= avctx->channels) {
> +                av_log(avctx, AV_LOG_WARNING, "Invalid original channel position.\n");

Just a nit here: I think the original channel position cannot possibly have been
wrong. Maybe "Invalid channel positioning." or "Invalid channel reordering." is
more clear for the user.

> +                sconf->chan_sort = 0;
> [...]
> @@ -1480,9 +1484,15 @@ static int decode_frame(AVCodecContext *avctx, void *data, int *got_frame_ptr,
>      {                                                              \
>          int##bps##_t *dest = (int##bps##_t*)ctx->frame.data[0];    \
>          shift = bps - ctx->avctx->bits_per_raw_sample;             \
> +        if (!sconf->chan_sort) {                                   \
>          for (sample = 0; sample < ctx->cur_frame_length; sample++) \
>              for (c = 0; c < avctx->channels; c++)                  \
>                  *dest++ = ctx->raw_samples[c][sample] << shift;    \
> +        } else {                                                                     \
> +            for (sample = 0; sample < ctx->cur_frame_length; sample++)               \
> +                for (c = 0; c < avctx->channels; c++)                                \
> +                    *dest++ = ctx->raw_samples[sconf->chan_pos[c]][sample] << shift; \
> +        }                                                                            \
>      }

I really don't like sconf->* to be set to something else than there is given in
the file. Also, it does break the debug print function (ffprobe maybe, at least
in debug too, I think). Two ideas arise:

a) Always use sconf->chan_pos[] array, even a generic {1,2,3,...,n} array if
channels are not really reordered (sconf->chan_sort == 0). I don't know how
"bad" it would be to always spend these few bytes for the array and the
dereferencing in the inner loop though... please comment.

Or: b) extend the decoders context to store a "chan_sort_ignore" bit.

-Thilo