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

bug: ADTS timebase missing #196

Closed
erikas-taroza opened this issue Mar 25, 2023 · 5 comments · Fixed by #222
Closed

bug: ADTS timebase missing #196

erikas-taroza opened this issue Mar 25, 2023 · 5 comments · Fixed by #222

Comments

@erikas-taroza
Copy link
Contributor

Hello.

I use this crate as a dependency in my project. A user ran into an issue with a missing timebase for an ADTS file.

Here is the sample file I was provided.

Using symphonia-play, the file is able to be played. However, there is no information about the position or duration (as expected with a missing timebase). Playing the file in VLC works as expected (duration + position are shown).

@pdeljanov
Copy link
Owner

I think there are probably two problems:

  1. The time base is missing. This should be pretty simple to fix since for audio it's just 1 / sample_rate and the ADTS header provides it.

  2. A missing time base just means the timestamps can't be converted to a time in seconds. However, you would still see the position and duration represented as sample count counts (played and total). The second issue is that ADTS, not unlike MP3, doesn't actually record the total duration. Instead, it must be estimated by finding the average bitrate and dividing the file size by the bitrate. This is a bit more work, but the MP3 demuxer is a good starting point. MP3 also sometimes has XING, LAME, or VBRI frames embedded into the stream to more accurately provide the duration. If there are similar frames for ADTS I'd be interested in hearing about them if someone is aware of any.

I believe @dedobbin is already investigating this issue, but I figured I should make a note of these observations here.

@erikas-taroza
Copy link
Contributor Author

  1. The time base is missing. This should be pretty simple to fix since for audio it's just 1 / sample_rate and the ADTS header provides it.

Yes, this is the workaround that my project is currently using.

I believe @dedobbin is already investigating this issue, but I figured I should make a note of these observations here.

Sounds good :)

@dedobbin
Copy link
Contributor

dedobbin commented Aug 18, 2023

Hey! Been some time, sorry about that.
In symphonia-codec-aac/src/adts.rs i added .with_time_base(TimeBase::new(1, header.sample_rate)) but this was not enough.
.with_n_frames(n_frames); is also necessary, the trouble is here that each frame has it's own header with a variable size, so it's a bit tricky the find the amount.

Lazy solution would be to just go through the entire file to find the individual sizes and determine the amount using that, but that's not great.

I found another decent solution by just parsing a few blocks and using the average size to estimate the total length. It's obviously not super accurate but it doesn't seem to be an uncommon approach.
I need to call MediaSource::byte_len for it though, which is costly.

Do you agree with this approach @pdeljanov ?

@BSteffaniak
Copy link

Hi, I just ran into this issue as well. I tried out the fix in dedobbin:bug/aac-missing-timeline and it seemed to work for me as well.

I see that the PR #222 might be related to the milestone for v0.5.4? Is there any timeline for releasing the version with this fix?

Thanks!

@pdeljanov
Copy link
Owner

Hi @BSteffaniak,

We are probably a few days away from a v0.5.4 release, however we've identified some issues with PR #222 when probing variable bitrate files. In some cases the estimated duration may be way off. To that end, I'm probably going to release v0.5.4 without it, but I'll cut a v0.5.5 release once the issue gets resolved since it's an isolated change.

pdeljanov pushed a commit that referenced this issue Mar 10, 2024
Approximate the duration of ADTS streams by sampling the
average bitrate at multiple locations.

Fixes #196.
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 a pull request may close this issue.

4 participants