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

[Peter Belkner]

On 17.04.2011 11:37, Reimar Döffinger wrote:
> 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

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)

> and where int isn't using two's
> complement (though I suspect FFmpeg might assume the later one
> as well).
>    

I was thinking about the endiness as well. But this patch is not for 
dealing with the endiness but only for 32 bit mode.


> Also not that identifiers starting with __ or _ and an uppercase
> letter are reserved for the system so you should avoid using them.
>    

Changed prefix from "__" to "av_"

>    
>> @@ -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.

Changed it anyway.

> (Note that AVERROR_NOMEM is deprecated).
>    

Changed it.

>    
>> -            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.
>    

Because we've made sure that the incoming data is 4 bytes a simple cast 
suffices.

>    
>> +#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.

Find a revised patch below.

Peter


-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: libmp3lame-32-bit-v2-revised.patch
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20110417/0497d530/attachment.ksh>