On 8/24/17, Carl Eugen Hoyos <ceffm...@gmail.com> wrote: > 2017-08-23 0:56 GMT+02:00 Ivan Kalvachev <ikalvac...@gmail.com>: >> On 8/22/17, Ronald S. Bultje <rsbul...@gmail.com> wrote: >>> Hi, >>> >>> On Mon, Aug 21, 2017 at 11:16 AM, Carl Eugen Hoyos <ceffm...@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. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
