search

MeadowlarkDAW / creek

Realtime disk streaming IO for audio
Other
112 stars 12 forks source link

improve performance and seek accuracy #48

Closed BillyDM closed 9 months ago

BillyDM commented 9 months ago

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 https://github.com/MeadowlarkDAW/creek/issues/43. I plan to look into that in a different PR.

Also another side note, I noticed that for some reason I used usize for the value of frames instead of u64. The whole point of disk streaming is to support very long files, and usize would 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.

BillyDM commented 9 months ago

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.

uklotzde commented 9 months ago

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?

BillyDM commented 9 months ago

What's the reasoning for keeping creek backwards compatible with 1.62?

I'm not sure actually. Was it @orottier or @Be-ing that added that?

Be-ing commented 9 months ago

Go ahead and bump the minimum Rust version if it's helpful (which it is in this case).

Be-ing commented 9 months ago

Since bumping the minimum supported version probably breaks semver, I'll remove it.

MSRV increase is not a semver breaking change.

orottier commented 9 months ago

I'm fine with bumping MSRV. I have not encountered anyone in the wild using pre 1.65 versions

BillyDM commented 9 months ago

MSRV increase is not a semver breaking change.

Oh it's not? Okay, I'll bump the version and merge this then.