Re: [PATCH] msacm32: Added converter for ADPCM to PCM 8 bit mono.

[Andrew Eikum]

Thanks for working on this. Some comments below.

On Fri, May 20, 2016 at 02:54:01AM +0200, Fabian Maurer wrote:
> +/***********************************************************************
> + *           R8
> + *
> + * Read a 8 bit sample
> + */
> +static inline short  R8(const unsigned char* src)
> +{
> +    return *src;
> +}
> +

R8 is never used.
> +    nsamp_blk--; /* remove the sample in block header */
> +    for (; nblock > 0; nblock--)
> +    {
> +        const unsigned char*    in_src = src;
> +
> +		/* handle header first */
> +		sample = R16(src);
> +		stepIndex = (unsigned)*(src + 2);
> +			clamp_step_index(&stepIndex);
> +		src += 4;
> +		W8(dst, sample);	dst += 1;
> +
> +		for (nsamp = nsamp_blk; nsamp > 0; nsamp -= 2)
> +			{
> +				process_nibble(*src, &stepIndex, &sample);
> +			W8(dst, sample); dst += 1;
> +				process_nibble(*src++ >> 4, &stepIndex, &sample);
> +			W8(dst, sample); dst += 1;
> +		}

The indentation here and elsewhere is a mess. I suggest just using
4-space indents, no hard tabs.
> +        /* we have now to realign the source pointer on block */
> +        src = in_src + adsi->pwfxSrc->nBlockAlign;
> +    }
> +}
> +

The only difference between cvtMMima8K and cvtMMima16K is a couple of
multiplications by 2 and the use of W8 instead of W16, right? I think
you can have just one function and use adsi->pwfxDst->wBitsPerSample
to determine sample size, to avoid so much code duplication.
> -	/* adpcm decoding... */
> -	if (adsi->pwfxDst->wBitsPerSample == 16 && adsi->pwfxDst->nChannels == 2)
> -	    aad->convert = cvtSSima16K;
> -	if (adsi->pwfxDst->wBitsPerSample == 16 && adsi->pwfxDst->nChannels == 1)
> -	    aad->convert = cvtMMima16K;
> +		/* adpcm decoding... */
> +		if (adsi->pwfxDst->wBitsPerSample == 16 && adsi->pwfxDst->nChannels == 2)
> +			aad->convert = cvtSSima16K;
> +		if (adsi->pwfxDst->wBitsPerSample == 16 && adsi->pwfxDst->nChannels == 1)
> +			aad->convert = cvtMMima16K;
> +		if (adsi->pwfxDst->wBitsPerSample == 8 && adsi->pwfxDst->nChannels == 1)
> +			aad->convert = cvtMMima8K;
> +		/* FIXME: Stereo support for 8bit samples*/
> +		if (adsi->pwfxDst->wBitsPerSample == 8 && adsi->pwfxDst->nChannels == 2)
> +			goto theEnd;

More bad indentation.

Andrew

[PATCH] msacm32: Added converter for ADPCM to PCM 8 bit mono.

[Fabian Maurer]

Hello, and thanks for the feedback.

The code is mostly C&P, so I didn't know if I should reformat it. I'll try to 
merge the two functions and fix the indentation in the process. Just wasn't 
sure if I should touch the working function or rather make a new one for my 
purposes.
 I'll resend the patch when I'm finished and have done the required tests.

Fabian