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 video converter #25

Merged
merged 5 commits into from
Jan 13, 2025
Merged

Conversation

gBillal
Copy link
Collaborator

@gBillal gBillal commented Jan 5, 2025

In this PR, we extract the video converter into its own struct and expose an Elixir module to convert pixel format of video samples.

The next step would be adding scaling

@mickel8
Copy link
Member

mickel8 commented Jan 10, 2025

@gBillal Sorry for no response. I will try to merge this tomorrow!

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.

Really nice job!

Just a small improvement in the API and a couple of nitpicks and we can merge 🎉

lib/video_converter.ex Outdated Show resolved Hide resolved
enum AVPixelFormat out_pix_fmt = av_get_pix_fmt(out_format);

if (in_pix_fmt == AV_PIX_FMT_NONE || out_pix_fmt == AV_PIX_FMT_NONE) {
ret = xav_nif_error(env, "unknown_format");
Copy link
Member

Choose a reason for hiding this comment

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

Let's raise here instead. We do a similar thing for decoder when user passes unsupported codec.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I updated it to raise however I think we should return an error because we don't have the list of available formats in the documentation or in the code where we can do the validation in Elixir code.

c_src/xav/video_converter.c Outdated Show resolved Hide resolved
c_src/xav/video_converter.c Outdated Show resolved Hide resolved
return -1;
}

return video_converter_init(xav_decoder->vc, frame->width, frame->height,
Copy link
Member

Choose a reason for hiding this comment

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

if this fails, we need to free video_converter. I can see that I implemented audio the same way but I think that's a bug

Copy link
Member

Choose a reason for hiding this comment

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

Okay, maybe that's not a bug as in our case, decoder's destructor will do the thing but I think we should fix this anyway i.e. free converter and leave xav_decoder->vc untouched.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes the destructor will free the converter in case of error.

lib/video_converter.ex Outdated Show resolved Hide resolved
c_src/xav/xav_video_converter.c Outdated Show resolved Hide resolved
@gBillal gBillal requested a review from mickel8 January 13, 2025 10:41
@mickel8
Copy link
Member

mickel8 commented Jan 13, 2025

Thanks!! 🎉

@mickel8 mickel8 merged commit b52115d into elixir-webrtc:master Jan 13, 2025
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