BitReader cleanup #922
Merged
BitReader cleanup #922
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
min_bits > 64 should always be false since this is a private function and is only called with 32 and 64 as inputs. If true, it would have caused an infinite loop, repeatedly executing the else branch. `if (m_bitIndex >= CHAR_BIT)` is unnecessary since otherwise / returns 0 and % returns m_bitIndex unchanged. Change if (shift > bits) to >= to prevent a possible unnecessary buffer read on the next call. Previously when equal the else branch executed, m_bitIndex becomes CHAR_BIT and on the next iteration bits is 0, meaning no new bits are added in during that iteration. shift and bits should be unsigned like all the other variables. Since now min_bits <= 64 and by the loop condition m_cacheSize < min_bits, m_cacheSize < 64, which means 1 <= shift <= 64. Also, since now m_bitIndex < CHAR_BIT, 1 <= bits <= CHAR_BIT. Therefore, shift and bits are now always positive and can be safely stored in an unsigned int. Additionally, the if statement in the while loop is now more easily proven to be always true for refill_cache(32) since min(shift) = 64 - (min_bits - 1) and max(bits) = CHAR_BIT therefore shift >= bits is always true when min_bits < 65 - CHAR_BIT. Finally, if shift < bits, then after the else block executes m_cacheSize is 64 and m_cache is full, so the loop condition is now guaranteed to fail (min_bits <= 64) and then return. Since the else block is only ever executed at most once per call, add an explicit return to avoid unnecessarily testing the loop condition.
BitReader does not use `char`s, so CHAR_BIT is not the correct value to use, even though POSIX requires it to be 8. BitReader uses the fixed width uint8_t type to read memory. However, the code would probably work with other sizes for the smallest addressable unit of memory.
In any cached BitReader implementation, the behavior is undefined if the data
being read is changed after the BitReader is constructed since the cache may
contain the old data, which could result in the returned read values being a mix
of the old and new data. To prevent this you would need to invalidate the
cache, which would turn the cached BitReader into an uncached BitReader.
Therefore, a cached BitReader implementation can assume that the data being read
is never modified during its lifetime (which makes any cached data always valid).
Assuming that the value pointed to by m_buffer has not changed since it was last
read into m_cache, the masking is unnecessary since the cleared values are
either equal to those already in m_cache or will be shifted out.
e.g.:
*m_buffer = 0b1101'1011
m_cache = 0b1001011001101010110000000000000000000000000000000000000000000000
m_cacheSize = 19
m_bitIndex = 3
Once shifted *m_buffer becomes
0b0000000000000000110110110000000000000000000000000000000000000000
The three bits that were formerly cleared are identical to the corresponding
bits in m_cache.
ulmus-scott
referenced
this pull request
Jul 29, 2024
The clang-tidy core.BitwiseShift pointed out undefined behavior refilling from cache. I cannot find an obvious problem here, but the calculation depends on state from outside the function. Unless code elsewhere is wrong, the shift variable should be restricted to the range [0..63] and the bits variable should be restricted to the range [0..7]. Tidy is complaining that the clause "1<<bits" is overflowing an integer, which isn't possible given these restrictions. https://clang.llvm.org/docs/analyzer/checkers.html#core-bitwiseshift
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a change to make ByteReader not depend on FFmpeg and some minor tweaks to BitReader. The only functional change is in the last commit in regards to how BitReader responds when the data it is reading has been modified since it was constructed (which should never happen). See the individual commits for details.
Checklist