Skip to content

Filter NALs globally#1403

Merged
Vixea merged 7 commits intoalvr-org:masterfrom
deiteris:remove-filter-nal
Jan 28, 2023
Merged

Filter NALs globally#1403
Vixea merged 7 commits intoalvr-org:masterfrom
deiteris:remove-filter-nal

Conversation

@deiteris
Copy link
Collaborator

@deiteris deiteris commented Jan 21, 2023

Handle NAL filtering globally and remove video buffer copy caused by it from Linux code.

@deiteris deiteris force-pushed the remove-filter-nal branch 2 times, most recently from 90d26c6 to c8fa538 Compare January 21, 2023 22:38
@deiteris deiteris requested a review from nowrep January 23, 2023 08:38
Copy link
Collaborator

@nowrep nowrep left a comment

Choose a reason for hiding this comment

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

filter_NAL needs to stay because radeon vaapi includes AUD and x264 includes SEI.
But it may be possible to just use offset into the encoded buffer instead of doing a copy?

@deiteris
Copy link
Collaborator Author

Since ClientConnection already implements header NALs extraction, maybe filtering part could be moved there so it's consistent across all implementations.

@deiteris deiteris force-pushed the remove-filter-nal branch 3 times, most recently from f46e71e to be37d3a Compare January 24, 2023 22:14
@deiteris
Copy link
Collaborator Author

deiteris commented Jan 24, 2023

Only after I've reworked filtering code I've realized why it actually doesn't work properly with AUD. AUD is the first NAL that we may encounter. This results in configuration headers never sent because they're not detected, and that's why stream may not work properly when it's present.

Ideally, this should be resolved by global configuration headers as it's done in #1385. For now, just skipping AUD, if it's the first header, is enough. SEI NALs will work as is.

@deiteris deiteris marked this pull request as ready for review January 25, 2023 08:23
@deiteris deiteris changed the title Linux: Remove filter_NAL and avoid copy Filter NALs globally Jan 25, 2023
Copy link
Member

@zmerp zmerp left a comment

Choose a reason for hiding this comment

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

Overall good to me, but I would personally remove the temporary b and l variables in processH264Nals and processH265Nals.

@deiteris
Copy link
Collaborator Author

I think this was only tested with AMD VA-API. If someone could test AMF and NVEnc with recent changes and confirm it works properly, that would be great.

@nowrep
Copy link
Collaborator

nowrep commented Jan 26, 2023

Works fine with AMF.

@deiteris deiteris requested a review from nowrep January 26, 2023 15:28
@deiteris
Copy link
Collaborator Author

Ready to merge.

@Vixea Vixea merged commit 58093df into alvr-org:master Jan 28, 2023
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.

4 participants