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

Ogg writer #50

Merged
merged 6 commits into from
Jan 5, 2024
Merged

Ogg writer #50

merged 6 commits into from
Jan 5, 2024

Conversation

LVala
Copy link
Member

@LVala LVala commented Jan 3, 2024

Adds the OggWriter and its tests.

Example will be updated in the following Pull Request.

@LVala LVala self-assigned this Jan 3, 2024
Copy link

codecov bot commented Jan 3, 2024

Codecov Report

Merging #50 (b8c1554) into master (579773b) will increase coverage by 0.81%.
The diff coverage is 89.10%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #50      +/-   ##
==========================================
+ Coverage   87.21%   88.03%   +0.81%     
==========================================
  Files          19       23       +4     
  Lines         829      894      +65     
==========================================
+ Hits          723      787      +64     
- Misses        106      107       +1     
Files Coverage Δ
lib/ex_webrtc/media/ogg_reader.ex 88.23% <100.00%> (+13.76%) ⬆️
lib/ex_webrtc/media/ogg/page.ex 97.87% <97.87%> (ø)
lib/ex_webrtc/media/ogg_writer.ex 96.66% <96.66%> (ø)
lib/ex_webrtc/media/ogg/header.ex 66.66% <66.66%> (ø)
lib/ex_webrtc/media/opus.ex 41.66% <41.66%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 579773b...b8c1554. Read the comment docs.

@LVala LVala marked this pull request as ready for review January 4, 2024 09:18
@LVala LVala requested a review from mickel8 January 4, 2024 09:24
Copy link
Member

@mickel8 mickel8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! 🎉

@@ -161,7 +161,7 @@ defmodule Peer do
Process.send_after(self(), :send_audio_packet, duration)
# values set to 0 are handled by PeerConnection.set_rtp
rtp_packet = OpusPayloader.payload(packet)
rtp_packet = %{rtp_packet | timestamp: state.last_audio_timestamp}
rtp_packet = %{rtp_packet | timestamp: trunc(state.last_audio_timestamp)}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this ever be non-integer? We calculate last_audio_timestamp as: last_audio_timestamp: state.last_audio_timestamp + timestamp_delta where timestamp_delta is timestamp_delta = trunc(duration * 48_000 / 1000) (so integer) and initial last_audio_timestamp is also integer

Copy link
Member Author

@LVala LVala Jan 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is just temporary (duration from OggReader.next_packet is now a float), I'll make sure there's no unnecessary truncs in the examples PR.

@@ -1,6 +1,6 @@
defmodule ExWebRTC.Media.OggReader do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have Ogg.Header and Ogg.Packet, it might be resonable to make everything the same in this PR (including IVF modules)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering about the same, but all of the Ogg submodules are @moduledoc false so from API'a standpoint, it's still consistent.

@mickel8 mickel8 changed the title Ogg writer Add Ogg writer Jan 4, 2024
@mickel8 mickel8 mentioned this pull request Jan 4, 2024
54 tasks
@mickel8 mickel8 changed the title Add Ogg writer Ogg writer Jan 4, 2024
@LVala LVala merged commit cf3142e into master Jan 5, 2024
4 checks passed
@LVala LVala deleted the ogg-writer branch January 5, 2024 09:04
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