Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tracks are not required to be ordered by track id in the MOOV box #43

Closed
udoprog opened this issue Jan 27, 2021 · 6 comments
Closed

Tracks are not required to be ordered by track id in the MOOV box #43

udoprog opened this issue Jan 27, 2021 · 6 comments

Comments

@udoprog
Copy link
Contributor

udoprog commented Jan 27, 2021

Trying to decode such a file currently panics on this assertion:
https://github.com/alfg/mp4-rust/blob/master/src/reader.rs#L69

image

There's two possible solutions I can imagine:

  1. Sort the tracks after reading them - this does however mean that there must be a hard requirement in the spec that track ids are contiguous.
  2. Maintain a separate indexing from track_id to index to allow fns such as sample_count which uses the track_id to work like a HashMap<u32, usize>.

Personally I'd go for option 2.

@alfg
Copy link
Owner

alfg commented Jan 27, 2021

Hi @udoprog,

Thanks for catching this. Option 2 sounds like a good idea. Maybe a track_id and track_idx. I'll check into this when I get a chance.

Edit: Also, thanks for your PRs. I'll try them out soon and merge. Jenkins queue is really backed up today. 😞

@udoprog
Copy link
Contributor Author

udoprog commented Jan 27, 2021

Thanks, if you'd like I can do 2 as well.

You already seem to have GH Actions in place, so removing Travis might be an option if it's causing issues!

@alfg
Copy link
Owner

alfg commented Jan 27, 2021

Contributions are welcome. :)

And yeah, GH Actions will probably have to do for now, but I'm sure the Travis queue will catch up tonight for the stable, beta and (optional) nightly Rust builds.

@nlfiedler
Copy link
Contributor

In the mean time, would it make sense to replace the assert with a return of an Err result? That seems more suitable for a library.

@alfg
Copy link
Owner

alfg commented Jul 9, 2021

Yeah, good catch. That should be updated to return Err.

cTopher added a commit to cTopher/mp4-rust that referenced this issue Jul 28, 2021
alfg pushed a commit that referenced this issue Jul 30, 2021
@alfg
Copy link
Owner

alfg commented Jul 30, 2021

#58

@alfg alfg closed this as completed Jul 30, 2021
jprochazk pushed a commit to jprochazk/mp4 that referenced this issue Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants