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 reader #43

Merged
merged 7 commits into from
Dec 21, 2023
Merged

Ogg reader #43

merged 7 commits into from
Dec 21, 2023

Conversation

LVala
Copy link
Member

@LVala LVala commented Dec 20, 2023

No description provided.

@LVala LVala self-assigned this Dec 20, 2023
Copy link

codecov bot commented Dec 20, 2023

Codecov Report

Merging #43 (3ca40fa) into master (a8709d5) will decrease coverage by 0.87%.
The diff coverage is 74.46%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #43      +/-   ##
==========================================
- Coverage   88.15%   87.29%   -0.87%     
==========================================
  Files          14       15       +1     
  Lines         701      748      +47     
==========================================
+ Hits          618      653      +35     
- Misses         83       95      +12     
Files Coverage Δ
lib/ex_webrtc/media/ogg_reader.ex 74.46% <74.46%> (ø)

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 a8709d5...3ca40fa. Read the comment docs.

@LVala LVala marked this pull request as ready for review December 20, 2023 15:55
@LVala LVala requested a review from mickel8 December 20, 2023 16:00
@mickel8 mickel8 mentioned this pull request Dec 20, 2023
54 tasks
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.

I didn't dive into Ogg itself but other than that looks great! I just added a couple of minor suggestions

4. Press the play button.

You can replace `video.avf` or `audio.ogg` and use your own files instead.
Copy link
Member

Choose a reason for hiding this comment

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

video.avf -> video.ivf

# and time spent on reading and parsing the file
# that's why you might hear short pauses in audio playback, when using this example
Process.send_after(self(), :send_audio_packet, duration)
rtp_packet = ExRTP.Packet.new(packet, 111, 1000, state.last_audio_timestamp, 1000)
Copy link
Member

Choose a reason for hiding this comment

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

I would pass 0 for everything that RTP sender is responsible for. Right now, we might confuse user that they are responsible for handling e.g. pt

@impl true
def handle_info(:send_audio_packet, state) do
case OggReader.next_packet(state.audio_reader) do
{:ok, reader, {packet, duration}} ->
Copy link
Member

Choose a reason for hiding this comment

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

The convention is to return reader at the last place in the result tuple. Take a look at Map.pop and others

audio_track = MediaStreamTrack.new(:audio)
video_track = MediaStreamTrack.new(:video)

{:ok, _} = PeerConnection.add_track(pc, audio_track)
Copy link
Member

Choose a reason for hiding this comment

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

Brilliant!

Process.send_after(self(), :send_frame, 30)
%{state | ivf_reader: ivf_reader, payloader: payloader}
Process.send_after(self(), :send_video_frame, 30)
Process.send_after(self(), :send_audio_packet, 20)
Copy link
Member

Choose a reason for hiding this comment

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

Actually, we could do Process.sendafter 0

Copy link
Member Author

Choose a reason for hiding this comment

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

Even better, send(self(), ...) 🙂

with {:ok, file} <- File.open(path),
reader <- %{file: file, packets: [], rest: <<>>},
# for now, we ignore ID Header and Comment Header
{:ok, reader, <<@id_signature, _rest::binary>>} <- do_next_packet(reader),
Copy link
Member

Choose a reason for hiding this comment

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

As above, keep reader at the end of return tuple

Reads next Ogg packet.

One Ogg packet is equivalent to one Opus packet.
This function also returns the duration of the audio in milliseconds, based on Opus packet TOC sequence.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe let's elaborate on TOC

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'll just redirect the reader to the RFC.

@LVala LVala merged commit ff7a979 into master Dec 21, 2023
4 checks passed
@LVala LVala deleted the ogg-reader branch December 21, 2023 08:43
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