[FFmpeg-devel] [PATCH]lavc/put_bits: Remove usage of BITSTREAM_WRITER_LE.

Ivan Kalvachev ikalvachev at gmail.com
Sat Aug 26 03:13:36 EEST 2017


On 8/24/17, Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:
> 2017-08-23 0:56 GMT+02:00 Ivan Kalvachev <ikalvachev at gmail.com>:
>> On 8/22/17, Ronald S. Bultje <rsbultje at gmail.com> wrote:
>>> Hi,
>>>
>>> On Mon, Aug 21, 2017 at 11:16 AM, Carl Eugen Hoyos <ceffmpeg at gmail.com>
>>> wrote:
>>>
>>>> Hi!
>>>>
>>>> Attached patch tries to slightly simplify and clean up the usage of
>>>> put_bits* in libavcodec: put_bits_le() functions exist for the
>>>> little-endian G.726 encoder, so the define makes less sense now.
>>>>
>>>> Fate passes here, please review, Carl Eugen
>>>
>>> I have to agree with Paul here, I can't say I'm a big fan of this...
>>
>> People,
>> As developers you are required
>> not only to give ultimate final verdicts,
>> but also give (nice technical) reasoning for them.
>>
>> Here is my list of pro and cons:
>>
>> - If it ain't broken, don't change it.
>
> No objection here - on the contrary, I wish this argument
> would count here more often...

Well, this too could be taken to an extreme.

>> + Bytesteam already uses explicit _le/be and it looks good.
>>
>> + Makes the code more clear. It is obvious that the given encoder
>> writes BE stream. Something that could easily be missed with the
>> single define.
>
>> - The type of bitstream however is not really important for the codec
>> working. Aka, there is no confusing, because no codec writes BE and LE
>> at the same time.(yet)
>
> Not at the same time but in the same encoder file (G.726).

It's at the same time...
since the endian-ness is decided at runtime.

There are two codecs g726le and g726, they execute same
code, with just few different conditions.

>> + Removes messy defines that obfuscate put_bits code, by separating
>> the big and little ending code.
>
>> - Duplicates put_bits.h code. It would probably make reworking harder,
>> as changes have to be synced in 2 places.
>
> I don't think this is correct (and not what the diffstats show afair) but
> it doesn't matter: I was under the impression this patch was a
> requirement after a previous commit, I am not particularly keen
> on it, see above.

Oh, there is already put_bits_le() that is always present
and put_bits() that could be le/be depending on the define.

Yeh, that needs a cleanup.

However it seems that the second g726 codecs is added by you
and that the introduction of put_bits_le duplication/unwinding
is also done by you, isn't it?

Maybe there is still more elegant way to do all that,
though I can't think of one now.

Best Regards.


More information about the ffmpeg-devel mailing list