2016-05-03 10:20 GMT+02:00 Alexandra Hájková <[email protected]>: >>> +typedef struct BitstreamContext { >>> + uint64_t bits; // stores bits read from the buffer >> >> It is 32 bits for get_bits. >> >> So, did you write the new code with a 64b from the start? Because, >> except for the _long/_64 version, it would have been probably simple & >> interesting to see what a version with 32b provides. > > There are not such a member in GetBitContext, so it's not clear to me what > exactly do you mean by this. Do you mean unsigned int _cache as equivalent > variable to bits? In any case, bits can't be 32bit because I need to do shifts > and bits >> 32 would be undefined operation, also making bits variable > 32bit will > cause slowdown because refill() will be called more often.
My intent was to say that I didn't care about technical reasons/situations (eg your shifts) that *forces* the use of a 64b type, but rather: what's the performance benefit that BitstreamContext gets over GetBitContext because of this? This can be seen as: what if get_bits uses a 64b type (ignoring cases you mention)? What if BitstreamContext uses a 32b one? I mean, at some point, the API change and the technical change have different merits and should be distangled, as the API one is still being discussed. One could potentially modify get_bits, keeping its API, but using your implementation. Slightly less effort in porting. >> This seems to behave differently from bitstream_read_32(s, 1). Is >> there a particular rule/hypothesis for using bitstream_read_bit ? > > What do you mean by different behaviour? This should always > return the same bit as bitstream_read_32(s, 1), if you know about > the case, when this is not true, please tell me how/when exactly > this's happening. There are 2 aspects: First, the 2 functions do not seem to invoke the same operations (refill_32 vs refill_64, cast to uint8_t, update of bits_left). Maybe because some are provably not needed, I don't know. For the 2nd aspect, you've already told you're not interested in that code, but when replacing bitstream_read_32(s, 1) by bitstream_read_bit(s) at this location: https://github.com/kurosu/ffmpeg/blob/gb/libavcodec/diracdec.c#L1082 causes the following warning with gcc 5.1/Win64: warning: array subscript is above array bounds [-Warray-bounds] s->globalmc[ref].pan_tilt[0] = dirac_get_se_golomb(gb); (and for every dirac_get_se_golomb(gb) in the codeblocks there) Now, I don't know what Undefined Behaviour it may invoke, if the compiler has a bug (a reduced test case doesn't show it), or if the above code has one in spite of my staring at it. Maybe bitstream_read_bit is a nice moniker, but implementing it with bitstream_read_32(s, 1) could be fine, as the code already expects sufficient optimizations abilities from the compiler. >> Why is bitstream_peek_63 different from bitstream_peek_32? Can't >> show_val be used? >> Hopefully the compiler optimizes the function a lot and does not >> really copy things... Although it's probably not that useful a hot >> function. > > You're right, show_val() can be used here, also this func is not used > anywhere and it should be discussed if it should be removed. Case in point I guess for it not being used, the return of show_val is "unsigned int", but should be uint64_t like get_val in such a case. No need to change it if bitstream_peek_63 is not used. >> get_bits has it inlined too. Maybe not that useful? Only if removing >> the inline doesn't actually increase size. > > I think using inline is useful here, it's just a recommendation for > the compiler, I'm not forcing inlining with av_always_inline Sure. I haven't benchmarked size- or speed-wise. >>> +/* Skip bits to a byte boundary */ >>> +static inline const uint8_t *bitstream_align(BitstreamContext *s) >>> +{ >>> + int n = -bitstream_tell(s) & 7; >>> + if (n) >>> + bitstream_skip(s, n); >>> + return s->buffer + (bitstream_tell(s) >> 3); >>> +} >> >> Is doing the extra _tell operation that useful if nobody uses it and >> it can be done separately when needed? >> Probably not a hot function though. > > I'm not sure, what do you mean by this. Bitstream_align/align_get_bits > is useful function used by various decoders and I don't see how this > could be done with just one "tell". I would have just used: static inline voidbitstream_align(BitstreamContext *s) { int n = -bitstream_tell(s) & 7; if (n) bitstream_skip(s, n); } but the compiler is likely smart enough to notice the return value is unused and remove its computation. -- Christophe _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
