improve performance and seek accuracy #48
Conversation
core/src/read/read_stream.rs
Outdated
| @@ -756,58 +756,84 @@ impl<D: Decoder> ReadDiskStream<D> { | |||
| reached_end_of_file = true; | |||
| } | |||
|
|
|||
| let copy_block_into_read_buffer = | |||
There was a problem hiding this comment.
Why not extract this function at the module level instead of using a local closure?
There was a problem hiding this comment.
Since it is only used by this one method, I think it makes sense to use a closure. But if you think it would be more readable as a function, we could do that.
There was a problem hiding this comment.
Ok, I extracted it into a function and did that let-else thing. You're right, I think it is easier to read that way.
core/src/read/read_stream.rs
Outdated
| block_index: usize, | ||
| start_frame_in_block: usize, | ||
| frames: usize| { | ||
| let maybe_block = { |
There was a problem hiding this comment.
Using let-else statements with early returns might improve the readability.
There was a problem hiding this comment.
Oh, I didn't know Rust had let-else statements. Awesome, learn something new every day.
|
Oh wait, let-else statements were only stabilized in version 1.65, but creek has set its minimum supported version 1.62. Since bumping the minimum supported version probably breaks semver, I'll remove it. |
What's the reasoning for keeping creek backwards compatible with 1.62? |
|
Go ahead and bump the minimum Rust version if it's helpful (which it is in this case). |
MSRV increase is not a semver breaking change. |
|
I'm fine with bumping MSRV. I have not encountered anyone in the wild using pre 1.65 versions |
Oh it's not? Okay, I'll bump the version and merge this then. |
I discovered that the new safe code could perform worse when decoding near the end of a file. It would zero-out blocks even if those blocks contained no data. So I fixed that.
Since fixing this required a change inside
ReadDiskStream::read(), I also went ahead and cleaned up that method by consolidating redundant code into a closure.I also noticed that the seeking accuracy in the Symphonia decoder could be improved, so I also did that. Ideally Symphonia should have a method to request an exact frame to seek to, but for now this should be better.
Note this change does not fix #43. I plan to look into that in a different PR.
Also another side note, I noticed that for some reason I used
usizefor the value of frames instead ofu64. The whole point of disk streaming is to support very long files, andusizewould potentially be too limiting on 32 bit platforms. But changing this would be a breaking change, so I'll wait to fix that in a different PR.