Allow non-static sources

[a1phyr]

This allows reading data from borrowed buffers and such.

This is a breaking change, because it adds many lifetimes in codebase.

Closes #117

[pdeljanov]

Thank you, this looks useful! I've tagged this for the next sem-ver breaking release.

[a1phyr]

Rebased onto dev-0.6 branch

[a1phyr]

Hi @pdeljanov !

If you eventually want to merge this, could you do it quickly please ? This PR touches a lot of parts of the codebase, so changes on dev-0.6 create many merge conflicts.

The clippy failure is unrelated to the PR, it fails on the previous commit too.

[pdeljanov]

Hi @a1phyr,

Thanks for diligently updating your PR! I was pushing out a large backlog of changes I had for 0.6, but I'll pause that until we finish with your PR.

It looks like a new clippy warning has appeared after the latest Rust release, but I'll just waive that since it's unrelated to your changes.

I'll try to review this PR sometime in the next few days, but from my initial look-through it seems reasonable enough.

[pdeljanov]

Now that I had a chance to sit down with this, I think for the original intent, this is good as-is.

However, I had a thought. Would it be better instead to make the readers generic across M: MediaSource instead of just the lifetime 's? The original design of Symphonia discounted this idea because I didn't want to pay the monomorphization cost per MediaSource type. However, I realize now that this can be avoided by adding an impl MediaSource for Box<dyn MediaSource> to the core::io module. Then, a Box'd MediaSource could be used with dynamic dispatch just as it is right now without forcing it on all users. Does this seem like a good idea?

I understand this is also probably outside the scope you had intended for this PR. I would be happy to merge your PR as-is and I can follow up with another commit, but if you'd like to try and make the change, just let me know.

[a1phyr]

If possible, I would prefer making the readers generic across M: MediaSource in another PR. I am not sure of the benefits though, I wouldn't do it without measuring the perf improvements and the compile time/binary size cost.

For this PR, I also made 0d177fd (but not pushed it to this PR) to avoid the generic lifetime on FormatReader by splitting the trait (full diff). What do you think ?

[pdeljanov]

I am not sure of the benefits though, I wouldn't do it without measuring the perf improvements and the compile time/binary size cost.

I think the main benefit is that if a user only requires one particular type of MediaSource (e.g., std::fs::File), then forcing them to box it creates an unnecessary allocation, and also makes all reads on the media source indirect calls.

Of course, the practical overhead of this is basically 0. MediaSourceStream was specifically created to amortize the overhead of those read calls by buffering chunks of data at a time. So I don't think we will see much of a performance gain, but I also definitely don't think we will see a loss.

Likewise, assuming the user continues to use a Box<dyn MediaSource>, the binary size should not change. My only concern here would be if someone did the following instead of using a box with dynamic dispatch:

enum SymphoniaFormatReader {
    File(FormatReader<std::fs::File>),
    Buffer(FormatReader<std::io::Cursor>,
}

The current design basically prevents them from being able to do this.

We have a number of tickets relating to MediaSources: #117, #119, #171. I don't think we can implement every suggestion, but I think a generic media source may address most things.

For this PR, I also made 0d177fd (but not pushed it to this PR) to avoid the generic lifetime on FormatReader by splitting the trait (full diff). What do you think ?

I'm not sure about this one. Is the motivation primarily for cleanliness?

One issue I see is that that it's not possible to call into_inner from a format created by the Probe since it returns Box<dyn FormatReader +'s> and erases the type of the media source.

If possible, I would prefer making the readers generic across M: MediaSource in another PR.

How do you feel about just pushing it as a second commit to this PR? If it doesn't work out we can just drop the commit. Usually I just squash all PRs down at merge time so it'll keep the history clean.

[a1phyr]

I'm not sure about this one. Is the motivation primarily for cleanliness?

It is also to avoid having a useless lifetime parameter to FormatReader, which is more annoying than useful. Also, removing all genericity from the trait is essential if we want to add a generic type parameter to the readers.

One issue I see is that that it's not possible to call into_inner from a format created by the Probe since it returns Box<dyn FormatReader +'s> and erases the type of the media source.

I found a way to keep into_inner in FormatReader.
It works well with lifetimes but would be more difficult with generic readers.

If possible, I would prefer making the readers generic across M: MediaSource in another PR.

How do you feel about just pushing it as a second commit to this PR? If it doesn't work out we can just drop the commit. Usually I just squash all PRs down at merge time so it'll keep the history clean.

I tried doing that but it requires far more work as it needs to touch most functions of the codebase, and would also requires some redesign (many functions actually depend on MediaSource + ReadBytes + SeekBuffered). I don't want to do so much work for such a small gain. Sorry :/

[a1phyr]

Hi @pdeljanov !

Does the current state satisfy you ?

[pdeljanov]

Hi @a1phyr,

I haven't forgotten about this! Due to real-world reasons, I haven't been able to dedicate any meaningful time to Symphonia.

I still would like to prototype and explore being generic over a MediaSource, as well as looking into alternatives to the newly introduced BuildFormatReader trait. This will take some time on my end.

I'm still open to merging the PR as it was prior to the BuildFormatReader commit since I think it laid a good foundation for my exploration and ultimately resolved the initial problem at hand. Otherwise, I think it may still be a while yet before I can provide a more substantial response.