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