Support Packed Audio #33
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Only ID3v4 tags are supported and only the PRIV frames, which are the ones used for synchronization.
Don't ask me why, but this is working correctly
mat-hek
requested changes
Oct 4, 2023
Comment on lines
+215
to
+216
| ns = Membrane.Time.nanoseconds(round(secs * 1_000_000_000)) | ||
| {id, ns} |
There was a problem hiding this comment.
Suggested change
| ns = Membrane.Time.nanoseconds(round(secs * 1_000_000_000)) | |
| {id, ns} | |
| time = round(secs * Membrane.Time.second())) | |
| {id, time} |
| end | ||
|
|
||
| defp parse_id3v4_tag(<<"ID3", version::binary-size(2), _rest::binary>>) do | ||
| raise "Unsupported ID3 header version #{inspect(version)}" |
There was a problem hiding this comment.
can't we just skip the header in this case?
| {tags, payload} = parse_id3v4_tags(buffer.payload, []) | ||
|
|
||
| state = | ||
| if state.start_pts == nil do |
There was a problem hiding this comment.
it seems it's already checked in update_start_pts
Comment on lines
+151
to
+158
| defp update_start_pts(state, tags) do | ||
| start_pts = | ||
| Enum.find_value(tags, fn {id, val} -> | ||
| if(id == "com.apple.streaming.transportStreamTimestamp", do: val, else: nil) | ||
| end) | ||
|
|
||
| if start_pts != nil, do: %{state | start_pts: start_pts}, else: state | ||
| end |
There was a problem hiding this comment.
Suggested change
| defp update_start_pts(state, tags) do | |
| start_pts = | |
| Enum.find_value(tags, fn {id, val} -> | |
| if(id == "com.apple.streaming.transportStreamTimestamp", do: val, else: nil) | |
| end) | |
| if start_pts != nil, do: %{state | start_pts: start_pts}, else: state | |
| end | |
| defp update_start_pts(%{start_pts: nil} = state, tags) do | |
| start_pts = | |
| Enum.find_value(tags, fn {id, val} -> | |
| if id == "com.apple.streaming.transportStreamTimestamp", do: val | |
| end) | |
| %{state | start_pts: start_pts} | |
| end |
| if start_pts != nil, do: %{state | start_pts: start_pts}, else: state | ||
| end | ||
|
|
||
| defp correct_pts(buffer, nil), do: %Buffer{buffer | pts: Ratio.to_float(buffer.pts) |> round()} |
There was a problem hiding this comment.
PTS and DTS in buffers aren't ratios anymore, they're Membrane.Time.t (integers underneath)
Comment on lines
+108
to
+116
| actions = | ||
| Enum.map(output, fn | ||
| {:buffer, buffer} -> | ||
| value = correct_pts(buffer, state.start_pts) | ||
| {:buffer, {:output, value}} | ||
|
|
||
| {action, value} -> | ||
| {action, {:output, value}} | ||
| end) |
There was a problem hiding this comment.
Let's pass already corrected timestamp to ADTS.parse_adts instead
| defp correct_pts(buffer, start_pts), | ||
| do: %Buffer{buffer | pts: Ratio.add(buffer.pts, start_pts) |> Ratio.to_float() |> round()} | ||
|
|
||
| defp parse_id3v4_tags(data, acc) do |
There was a problem hiding this comment.
let's move all this ID3 parsing to a separate module
mat-hek
requested changes
Oct 4, 2023
| max_bit_rate: max_bit_rate | ||
| max_bit_rate: max_bit_rate, | ||
| # For id3 handling | ||
| start_pts: nil |
There was a problem hiding this comment.
Suggested change
| start_pts: nil | |
| pts_offset: nil |
|
Hi @dmorn, any update on this? |
|
Hi @mat-hek! Unfortunately not! |
|
@dmorn got it, but is that something you still need? |
|
Hi @mat-hek, right now no! Thanks! |
|
... we're very close though |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As discussed with @mat-hek, before merging this one we should consider
removing the ID3 parsing code and instead use a library, as there is
code duplication between here and the mp3 decoder.
Right now tests are passing, but