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

[Reimar Döffinger]

On Sun, Apr 17, 2011 at 05:37:40PM +0200, Peter Belkner wrote:
> >>+#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
> 
> I was thinking of systems with int < 32 bits where long may be 32
> bits (not very common these days and may be not supported by FFmpeg
> anyway)

It was decided that such systems are definitely not supported
by FFmpeg and would be a huge effort to make them supported.

> >>@@ -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);
> >}
> >
> 
> I'm not very happy with this because with this style clean-up code
> is spread across the whole world and is very hard to maintain.

No it doesn't, you didn't use the code I suggested!
I meant to suggest that adding an argument to function with the
only effect of making that function return exactly that value
seems silly.
My suggestion does still (re-)use MP3lame_encode_close!

> >>+#ifdef __lame_encode_buffer_int32
> >>+    av_freep(&s->s32_data.left);
> >I think you should also set .right to NULL.
> 
> Had removed this on request:
> 
>    On 16.04.2011 11:22, Carl Eugen Hoyos wrote:
> >>    +    if (AV_SAMPLE_FMT_S16 == avctx->sample_fmt) {
> >>    +        s->s32_data.left = NULL;
> >>    +        s->s32_data.right = NULL;
> >    This looks unneeded.
>    I removed this assuming that the memory block priv_data is pointing
>    to is zeroed out.
> 
> Please advise.

The code Carl Eugen replied to is from a different place,
I assume initialization?
At the very least it is not the case that I commented on, where you just
freed the memory block s->s32_data.right pointed to.
It's better to NULL it for the same reason it is better to use av_freep instead
of av_free, even if strictly speaking it is not necessary.