Fix read_position and write_position tracking #536
Merged
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
This reverts commit 7c4ca0d. The commit introduced a bug where mod_inc returned the incremented value, while the places it is used in expected the previous value. Additionally, the rest of the logic for tracking buffer positions does not work when the read_position and write_position wrap around, since they are directly compared. In particular, in that model, it is impossible to distinguish the case of all buffers full vs. all buffers empty, since both pointers will be equal in both cases. So a more thorough refactor is necessary to fix that... Let's revert this for now, since it breaks everything as it stands. Fixes: umlaeute#535 Signed-off-by: Asahi Lina <lina@asahilina.net>
This basically solves the same problem that 7c4ca0d tried to solve. It is practically impossible for a 64-bit type to actually overflow (it would take 4 billion years of 60 FPS frames), and this avoids having to do a much more thorough refactor to switch to wrapping buffer indices with the extra requirement of tracking buffer fullness separately. I checked that all of the usages should promote/truncate properly: (s64) (OP) (s32 or u32) should always promote to 64-bit, and all of the references to read_position and write_position are either direct comparisons or apply a modulo before storing into a smaller type, except for one which this commit fixes. Signed-off-by: Asahi Lina <lina@asahilina.net>
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 reverts a commit that tried to make those two variables wrap to avoid overflow, but which broke the whole tracking logic since the rest of the code doesn't expect that (on top of other issues). It's not obvious how to refactor the rest of the code to use wrapping buffer indices correctly.
Instead, this switches the indices to 64-bit, which should eliminate any concerns about them ever wrapping.
(I'm actually not sure how this ever worked for anyone, it completely broke v4l2loopback for me...)
Fixes: #535