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: gsteamer plugin for sending ancillary data #1044

Merged
merged 7 commits into from
Jan 24, 2025

Conversation

Sakoram
Copy link
Collaborator

@Sakoram Sakoram commented Jan 17, 2025

This PR adds:

  1. Experimental Pipeline API for ST40 TX. This API is in experimental directory, and shouldn't be relied on. It could be moved to normal include directory after better testing and creating corresponding RX API.
  2. Sink Gstream plugin that enables sending ancillary data with MTL. At the moment the plugin sends raw data inside User Data Words in ST 291 ancillary data packet , for now most other fields are hardcoded, but will be added as parameters to plugin.
  3. Fixed interpretation of data_count field in gst_mtl_st40_rx.c as its endianness was wrong.
  4. Minor stylistic changes

The plugin requires more logic to be able to sync with Video plugin, as it needs to send ancillary data in sync with video frames. ( with the same timestamp). This is to be done later.

@Sakoram Sakoram force-pushed the gsteamer-plugin-st40tx branch 2 times, most recently from f8d84a5 to 1693bee Compare January 20, 2025 09:41
@DawidWesierski4 DawidWesierski4 changed the title Gsteamer plugin for sending ancillary data Add: gsteamer plugin for sending ancillary data Jan 21, 2025
@DawidWesierski4
Copy link
Collaborator

This PR adds:

  1. Pipeline API for ST40 TX. This API is not published in include dir so it is not installed with MTL. The API is for internal use for now. It could be published after better testing and creating corresponding RX API.
  2. Sink Gstream plugin that enables sending ancillary data with MTL. At the moment the plugin sends raw data inside User Data Words in ST 291 ancillary data payload. For now all other fields are hardcoded, but will be passed as parameters to plugin.

The plugin requires more logic to be able to sync with Video plugin, as it needs to send ancillary data in sync with video frames. ( with the same timestamp). This is to be done later.

Please add a paragraph why the code in RX plugin was changed as this is also part of this PR
I think change the Internal paths you use for pipeline to experimental folder and reflect this change in commit message / description with this approach we are using the system already in place

ecosystem/gstreamer_plugin/gst_mtl_st40p_tx.c Outdated Show resolved Hide resolved
ecosystem/gstreamer_plugin/gst_mtl_st40p_tx.c Outdated Show resolved Hide resolved
ecosystem/gstreamer_plugin/meson.build Outdated Show resolved Hide resolved
ecosystem/gstreamer_plugin/gst_mtl_st40_rx.c Show resolved Hide resolved
ecosystem/gstreamer_plugin/gst_mtl_st40p_tx.c Outdated Show resolved Hide resolved
Copy link
Collaborator

@DawidWesierski4 DawidWesierski4 left a comment

Choose a reason for hiding this comment

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

I would try ty keep things consinstant generally but all of those are suggestions
This code is fine :<

But i would love it to be more readable

The things i would recommend is to be consistent with the side of literals

I would put them all on the left side in comparisons in new code
Feel free to ignore the comments you deem unnecessary

This can be a part of a larger refactor

lib/src/st2110/pipeline/st40_pipeline_tx.c Outdated Show resolved Hide resolved
lib/src/st2110/pipeline/st40_pipeline_tx.c Outdated Show resolved Hide resolved
lib/src/st2110/pipeline/st40_pipeline_tx.c Outdated Show resolved Hide resolved
lib/src/st2110/pipeline/st40_pipeline_tx.c Outdated Show resolved Hide resolved
lib/src/st2110/pipeline/st40_pipeline_tx.c Outdated Show resolved Hide resolved
lib/src/st2110/pipeline/st40_pipeline_tx.h Outdated Show resolved Hide resolved
lib/src/st2110/pipeline/st40_pipeline_tx.c Outdated Show resolved Hide resolved
lib/src/st2110/pipeline/st40_pipeline_tx.c Outdated Show resolved Hide resolved
lib/src/st2110/pipeline/st40_pipeline_tx.c Outdated Show resolved Hide resolved
lib/src/st2110/pipeline/st40_pipeline_tx.c Outdated Show resolved Hide resolved
Copy link
Contributor

@MateuszGrabuszynski MateuszGrabuszynski left a comment

Choose a reason for hiding this comment

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

Nothing major, but there are a few places that would require modifications. ;)

ecosystem/gstreamer_plugin/gst_mtl_st40p_tx.c Outdated Show resolved Hide resolved
ecosystem/gstreamer_plugin/gst_mtl_st40p_tx.c Outdated Show resolved Hide resolved
ecosystem/gstreamer_plugin/gst_mtl_st40p_tx.c Outdated Show resolved Hide resolved
lib/src/st2110/pipeline/st40_pipeline_tx.c Outdated Show resolved Hide resolved
lib/src/st2110/pipeline/st40_pipeline_tx.c Outdated Show resolved Hide resolved
lib/src/st2110/pipeline/st40_pipeline_tx.c Outdated Show resolved Hide resolved
Signed-off-by: Kasiewicz, Marek <[email protected]>
Signed-off-by: Kasiewicz, Marek <[email protected]>
Add DID, SDID and framerate parameters to st40 tx plugin
Fix code formatting

Signed-off-by: Kasiewicz, Marek <[email protected]>
Signed-off-by: Kasiewicz, Marek <[email protected]>
Copy link
Collaborator

@DawidWesierski4 DawidWesierski4 left a comment

Choose a reason for hiding this comment

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

LGTM
Fix tests tho

Copy link
Contributor

@MateuszGrabuszynski MateuszGrabuszynski left a comment

Choose a reason for hiding this comment

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

lgtm :)

Signed-off-by: Kasiewicz, Marek <[email protected]>
Signed-off-by: Kasiewicz, Marek <[email protected]>
@Sakoram Sakoram merged commit 921fa0d into OpenVisualCloud:main Jan 24, 2025
23 of 25 checks passed
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.

3 participants