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

Add FrameInfo #1121

Closed
wants to merge 1 commit into from
Closed

Conversation

Sean-Der
Copy link
Contributor

Depacketizers will include metadata about assembled frame.

@Sean-Der Sean-Der force-pushed the frame-info branch 5 times, most recently from fab084d to 1c75ffd Compare March 4, 2024 01:55
@Sean-Der Sean-Der changed the title Add FrameInfo Add MediaSample Mar 4, 2024
@Sean-Der Sean-Der marked this pull request as ready for review March 4, 2024 01:57
@Sean-Der
Copy link
Contributor Author

Sean-Der commented Mar 4, 2024

@paullouisageneau this is ready for review, thank you!

Copy link
Owner

@paullouisageneau paullouisageneau left a comment

Choose a reason for hiding this comment

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

As I understand, you use the same MediaSample for storing metadata and sometimes data, so it also inherits from binary. Why not simply adding a pure metadata strut (I actually liked the name FrameInfo) that you'd pass to the user alongside the data, for instance with onFrame(std::function<void(binary data, FrameInfo frame)>?

@Sean-Der
Copy link
Contributor Author

Sean-Der commented Mar 5, 2024

@paullouisageneau 👍 will fix that now!

@Sean-Der Sean-Der changed the title Add MediaSample Add FrameInfo Mar 5, 2024
Depacketizers will include metadata about assembled media.
@Sean-Der
Copy link
Contributor Author

Sean-Der commented Mar 6, 2024

@paullouisageneau done! Can I get another review please?

Copy link
Owner

@paullouisageneau paullouisageneau left a comment

Choose a reason for hiding this comment

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

I have a couple nitpicks, but otherwise it looks good, thanks for the work!

src/h264rtpdepacketizer.cpp Show resolved Hide resolved
include/rtc/message.hpp Show resolved Hide resolved
@Sean-Der Sean-Der closed this Mar 8, 2024
@Sean-Der Sean-Der deleted the frame-info branch March 8, 2024 15:15
@Sean-Der
Copy link
Contributor Author

Sean-Der commented Mar 8, 2024

Moved to Opus PR

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