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

isomp4 take track id from the media source #324

Closed
wants to merge 1 commit into from

Conversation

sscobici
Copy link

for isomp4 track id is assigned sequentially from 0, while track id for mkv is taken from the media source.
According to description from the core Track - id should be taken from the media source. This PR fixes track id for mp4.

@pdeljanov
Copy link
Owner

pdeljanov commented Oct 23, 2024

This is going to break in the cases the id read from the container does not match identically with the index of the track. The track_num in TrackState (the field id is saved to) is often used to map to an index into a vector to get track-specific data. This vector would need to be changed to a HashMap since the id is not guaranteed to be sequential.

Changing this will be quite a substantial modification with potential performance impacts.

@sscobici
Copy link
Author

The track_num in TrackState (the field id is saved to) is often used to map to an index into a vector to get track-specific data.
Changing this will be quite a substantial modification with potential performance impacts.

My bad, I haven't observed this. Strange that mkv is not considering this but probably there were no examples with non sequential track ids. I will take further look into this.

ffmpeg AVStream has two fields:

  • index - zero based index for vector
    id - the value from the media source

Is it worth introducing second trackid into isomp4 code to not touch existing one, but provide the media source value outside? This will fix mediainfo comparison and also present the values which are read from file.

@sscobici
Copy link
Author

it seems that track_id(id from file) and track_num(index) are the names that should be used.
Unfortunately these two values are mixed now and this will probably break the parsing for files where track id is not sequential or not starting from 0.

This would be much harder to fix and risky but still possible. Let me know if you want me to do this and what would be the high level approach to allow your easy review?

@sscobici sscobici marked this pull request as draft October 24, 2024 14:40
@pdeljanov
Copy link
Owner

I played around with this, and I think I may take this one since I basically implemented it. It's actually not a big set of changes. Just need to add the track_id to TrackState, and use it where appropriate (mainly, seek and next_packet).

pdeljanov added a commit that referenced this pull request Oct 25, 2024
@pdeljanov
Copy link
Owner

Fixed. Thanks for noticing the issue though!

@pdeljanov pdeljanov closed this Oct 25, 2024
@sscobici sscobici deleted the mp4-fix-track-id branch October 25, 2024 19:29
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

Successfully merging this pull request may close these issues.

2 participants