[FFmpeg-devel] [PATCH] libmp3lame: 32 bit encoding

[Reimar Döffinger]

On Sun, Apr 17, 2011 at 09:45:31AM +0200, Peter Belkner wrote:
> On 16.04.2011 23:36, Reimar Döffinger wrote:
> >In reality not really.
> >Running the a conversion even if 99.9% of all cases don't need
> >it doesn't seem reasonable.
> 
> The type conversion is an implicit goody. The main reason for the
> loops is to split interleaved into seperated channels.

Sorry I didn't read the patch properly!
I have some small comments if you are interested (hopefully not
due to misreading the code this time).

> That the code block guarded by the condition is moved may be not
> obvious by only looking at the patch.

It would have been if I had looked properly, sorry again.

> +#if (2147483647 == INT_MAX)
> +#define __lame_encode_buffer_int32 lame_encode_buffer_int
> +typedef int __lame_int32_t;
> +#elif (2147483647L == LONG_MAX)
> +#define __lame_encode_buffer_int32 lame_encode_buffer_long2
> +typedef long __lame_int32_t;
> +#endif

In principle IMO it is rather pointless: FFmpeg already does not
support systems where int is 32 bit.
That basically leaves the cases where lame_encode_buffer_int
at systems with int > 32 bit and where int isn't using two's
complement (though I suspect FFmpeg might assume the later one
as well).
Also not that identifiers starting with __ or _ and an uppercase
letter are reserved for the system so you should avoid using them.

> @@ -69,8 +85,19 @@ static av_cold int MP3lame_encode_init(AVCodecContext *avctx)
>  
>      avctx->frame_size = lame_get_framesize(s->gfp);
>  
> -    avctx->coded_frame= avcodec_alloc_frame();
> +    if(!(avctx->coded_frame= avcodec_alloc_frame()))
> +        return MP3lame_encode_close_internal(avctx, AVERROR_NOMEM);

I'd suggest not to change the close function but simply do
if (!avctx->coded_frame) {
    MP3lame_encode_close(avctx);
    return AVERROR(ENOMEM);
}

(Note that AVERROR_NOMEM is deprecated).

> -            lame_result = lame_encode_buffer(
> +            int32_t *rp = (int32_t *)data;
> +            int32_t *mp = rp + avctx->frame_size;
> +            __lame_int32_t *wp = s->s32_data.left;
> +
> +            while (rp < mp)
> +                *wp++ = *rp++;

This is just a memcpy now I think.

> +
> +            lame_result = __lame_encode_buffer_int32(
>                  s->gfp,
> -                data,
> -                data,
> +                s->s32_data.left,
> +                s->s32_data.left,

An thus I don't see the point of doing the copying at all.

> +#ifdef __lame_encode_buffer_int32
> +    av_freep(&s->s32_data.left);

I think you should also set .right to NULL.