[FFmpeg-devel] [PATCH v3 2/2] libavcodec/flacenc: Implement encoding of 32 bit-per-sample PCM

[Andreas Rheinhardt]

Martijn van Beurden:
> First of all, thanks for reviewing.
> 
> Op vr 2 sep. 2022 om 17:11 schreef Andreas Rheinhardt <
> andreas.rheinhardt at outlook.com>:
> 
>>> +static inline void put_sbits64(PutBitContext *pb, int n, int64_t value)
>>> +{
>>> +    av_assert2(n >= 0 && n <= 64);
>>> +
>>> +    put_bits64(pb, n, (uint64_t)(value) & (~(UINT64_MAX << n)));
>>
>> Shifting by 64 bits here is UB, so better modify the assert to disallow
>> it. And rename the function to put_sbits63().
>>
>>
> I could also add specific handling for the 64-bit case. Perhaps something
> like
> 
> put_bits64(pb, n, (uint64_t)(value) & (~((n < 64)?(UINT64_MAX << n):0)));
> 
> or should I leave that to whoever needs that functionality?
> 

Leave it to whoever needs this.

> 
>>> -static inline void set_sr_golomb_flac(PutBitContext *pb, int i, int k,
>>> -                                      int limit, int esc_len)
>>> +static inline void set_sr_golomb_flac(PutBitContext *pb, int i, int k)
>>
>> This seems to be only used by flacenc.c, so IMO it would be better to
>> move it there.
>>
>>
> Yes, I was wondering what to do with this, similarly with the functions in
> get_bits, mathops and put_bits. I suppose the additions to get_bits,
> mathops and put_bits might be useful to others in the future. The golomb
> code is probably not useful for other codecs indeed. Is that reasoning
> valid?

Yes.