On 4/27/2016 12:35 PM, Alexandra Hájková wrote: > Greetings, > I'd like to introduce a new bitstream reader I wrote. The advantages of the > new bitreader are: > * it is easier to use: > for example there is just one function to read 0-32 bits instead of > 3 of them in the get_bits.h > * it is more consistent and easier to follow (macros are not needed anymore) > * it is better documented > * it is faster for some decoders (no speed loss when not faster) > * it could be made a public header. > > This work has been sponsored by Luminem.it and it follows the ideas suggested > by Niels Möller in his email from > the time when he was working on DCA XLL extention > (https://www.mail-archive.com/libav-devel%40libav.org/msg57746.html). > > This patchset does some preparation, adds the new bitreader and converts a > few decoders to use it. The following patchets > will subsequently convert all remaning decoders and demuxers that uses > get_bits.h to the new bitreader and original get_bits.h > will be removed in the end of the last patchset.
I have several points: 1. Why was was this new impl not adapted to the current get_bits API? Why do we need a giant invasive change to all of avcodec? 2. Why was this (paid!) large patch not really discussed before just... showing up? I really dislike the "well it's already paid for and implemented" mentality this creates when considering whether or not its even a good idea hasn't even been discussed. 3. Some of the points aren't really good arguments for a new API, such as "it is better documented". 4. "it could be made a public header" -> Hell no. Don't bit a bitreader in public API. 5. Not sure I buy "easier to use". 6. Where are the speed test results? You're not going to sell an invasive patch set like this to anyone without very good benefits provided... Just my initial stream of thoughts. - Derek _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
