-
Notifications
You must be signed in to change notification settings - Fork 142
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/aac missing timeline #222
Bug/aac missing timeline #222
Conversation
8c0f81c
to
f13fc63
Compare
Could you please test this with the following command: I don't think the seek will work if the input is not a seekable file! You will need to use the seek buffer in these cases. |
Alright. As discussed, it now guarantees being rewindable , getting rid of the nasty panic and making it possible to set a max bytes to sample the approximate frames |
8c807be
to
d492f7c
Compare
28a49a2
to
0ea6c85
Compare
0d17703
to
eef52c3
Compare
Getting very good results with this last set of changes! |
01b85f2
to
e93994b
Compare
e93994b
to
7af4571
Compare
Alright, fixed all CI problems |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to cause errors when testing with symphonia-check
. I'm assuming we're dropping a few initial packets for some reason...
symphonia-codec-aac/src/adts.rs
Outdated
let step = total_len / 3; | ||
if source.is_seekable() { | ||
let mut new_pos = total_len / 2; | ||
|
||
loop { | ||
if new_pos >= total_len { | ||
break; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit strange to me. You are attempting to seek to and examine 3 location in the file (guessing because you're doing total_len / 3
), but you start from the middle of the file (new_pos = total_len / 2
) which means you are skipping half of the file.
Assuming you want to sample at points a, b, and c, shouldn't you calculate step = (total_len - original_pos) / 4
, and start at new_pos = orignal_pos + step
? Diagram below.
a b c
| | | | |
^ original_pos ^ total_len
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This loop
could also be made into a for loop:
for new_pos in (original_pos..total_len).step_by(step).skip(1) {
No need for mutable variables!
symphonia-codec-aac/src/adts.rs
Outdated
break; | ||
} | ||
|
||
for _ in 0..=500 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
500 is a lot of frames! That's over 11 seconds of audio at 44.1 kHz. Can we do a magnitude less?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that seems on the extreme side. I was not sure what numbers to plug in, i lowered it now and it still gives acceptable output to me. Let me know if this works for you or how we should go about finding a sweet spot.
86efee6
to
0cc3e6a
Compare
Fixes #196