rewrite timestamps #55
Merged
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
bartkrak
requested review from
FelonEkonom
and removed request for
FelonEkonom
FelonEkonom
requested changes
Dec 20, 2023
FelonEkonom
requested changes
Jan 3, 2024
FelonEkonom
requested changes
Jan 12, 2024
lib/membrane_opus/parser.ex
Outdated
| processor, | ||
| [packet | packets], | ||
| channels, | ||
| updated_state = |
There was a problem hiding this comment.
You can just name this variable state
test/membrane_opus/parser_test.exs
Outdated
| metadata: %{duration: fixture.duration} | ||
| } | ||
| end) | ||
| defp buffer_from_fixture(fixtures, self_delimited? \\ false) do |
There was a problem hiding this comment.
Suggested change
| defp buffer_from_fixture(fixtures, self_delimited? \\ false) do | |
| defp buffers_from_fixtures(fixtures, self_delimited? \\ false) do |
FelonEkonom
requested changes
Jan 12, 2024
lib/membrane_opus/encoder.ex
Outdated
| @@ -58,6 +58,7 @@ defmodule Membrane.Opus.Encoder do | |||
| options | |||
| |> Map.from_struct() | |||
| |> Map.merge(%{ | |||
| pts_current: nil, | |||
There was a problem hiding this comment.
Suggested change
| pts_current: nil, | |
| current_pts: nil, |
FelonEkonom
reviewed
Jan 12, 2024
lib/membrane_opus/encoder.ex
Outdated
Comment on lines
167
to
177
| defp check_pts_integrity(true = _flag, %Buffer{pts: pts}, input_pts) do | ||
| if pts != input_pts do | ||
| raise """ | ||
| PTS values are not continuous | ||
| """ | ||
| end | ||
| end | ||
|
|
||
| defp check_pts_integrity(false = _flag, %Buffer{pts: _pts}, _input_pts) do | ||
| end | ||
|
|
There was a problem hiding this comment.
In the another comment I have written that is should be deleted, but FYI if you want to write function that do some checks and raises if any of them is not met and does nothing otherwise,
- If all checks pass, function should return
:okinstead ofnil - name of the function should end with
!
Co-authored-by: Feliks Pobiedziński <[email protected]>
Co-authored-by: Feliks Pobiedziński <[email protected]>
Co-authored-by: Feliks Pobiedziński <[email protected]>
FelonEkonom
requested changes
Jan 15, 2024
FelonEkonom
approved these changes
Jan 15, 2024
mat-hek
approved these changes
Jan 17, 2024
lib/membrane_opus/encoder.ex
Outdated
Comment on lines
135
to
139
| defp set_current_pts(%{queue: <<>>} = state, input_pts) do | ||
| %{state | current_pts: input_pts} | ||
| end | ||
|
|
||
| defp set_current_pts(state, _input_pts), do: state |
There was a problem hiding this comment.
let's move this below the public functions, or at least below the place it's first used
mat-hek
force-pushed
the
elem-rewrite-timestamps
branch
from
a7c53de to
1ac385b
Compare
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.
solves membraneframework/membrane_core#687