-
Notifications
You must be signed in to change notification settings - Fork 1
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
Create Decoders #1
Conversation
[] | ||
end | ||
|
||
buffers = Enum.map(decoded_frames, &%Buffer{payload: &1, pts: pts}) |
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.
Do we have a guarantee that Native.decode_frame
won't return some "buffered" frames, that should have different PTS? (I am slightly worried of that as decode_frame
returns a list of frames)
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.
I believe we do, since there is no flushing mechanism
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.
Ok, so could we match on [decoded_frame] = decoded_frames
? This way we will have a clear error message if for some reason decoder returns more than one frame, that could potentially mess up timestamps
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.
Just one tiny new comment and one follow up on a conversation from the last review's round
state.framerate || raise("Framerate not provided") | ||
} | ||
|
||
%^codec_module{width: width, height: height, framerate: framerate} -> |
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.
do we really need that codec_module
here? It's only stored in the state to be used here.
You could match on %{width: width, height: height, framerate: framerate}
and the stream format definition will make sure that no other module than VP8
or VP9
can be applied here.
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.
I did this to ensure the stream format is actually matching the codec, but i guess accepted_format is enough
The decoding nifs are mostly based on this example