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

Expose pixel format in xcoder-quadra #201

Merged
merged 8 commits into from
Jul 17, 2024
Merged

Expose pixel format in xcoder-quadra #201

merged 8 commits into from
Jul 17, 2024

Conversation

Xaeroxe
Copy link
Contributor

@Xaeroxe Xaeroxe commented Jul 11, 2024

No description provided.

@Xaeroxe Xaeroxe requested review from scottlamb and epingel2 July 11, 2024 22:19
xcoder/xcoder-quadra/src/lib.rs Show resolved Hide resolved
@@ -253,6 +256,7 @@ impl<F> XcoderEncoder<F> {
XcoderEncoderCodec::H264 { .. } => sys::_ni_codec_format_NI_CODEC_FORMAT_H264,
XcoderEncoderCodec::H265 { .. } => sys::_ni_codec_format_NI_CODEC_FORMAT_H265,
};
(**session).pixel_format = i32::try_from(config.pixel_format.repr()).expect("cast should never fail");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there are a couple things elsewhere in this function that are assuming yuv420p. Search for ni_get_hw_yuv420p_dim, which probably should be replaced with ni_get_frame_dim. Could you try adding a unit test first that uses bgra? I would expect it to fail as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, dang you're right. I had deluded myself into thinking that happened downstream.

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Jul 13, 2024

Updating the xcoder library seems to have induced some behavior that is a stack overflow without a debugger, and a segfault with a debugger. Pending further investigation.

@Xaeroxe Xaeroxe force-pushed the pixel-fmt branch 2 times, most recently from 6964695 to 44f8809 Compare July 15, 2024 22:03
@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Jul 15, 2024

Issue resolved, _ni_xcoder_params had grown too large to be kept on the stack.

@Xaeroxe
Copy link
Contributor Author

Xaeroxe commented Jul 16, 2024

Tests pass now, the bgra output looks good upon being inspected.

@Xaeroxe Xaeroxe merged commit a5d72a6 into main Jul 17, 2024
5 checks passed
@Xaeroxe Xaeroxe deleted the pixel-fmt branch July 17, 2024 17:25
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