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

kvssink CI enhancements! #1226

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

kvssink CI enhancements! #1226

wants to merge 7 commits into from

Conversation

sirknightj
Copy link
Contributor

@sirknightj sirknightj commented Jan 24, 2025

Issue #, if available:
#1225

Description of changes:

1. Fixing the unit tests with kvssink.

Unit tests were reported broken in the issue above. Due to a combination of two combined issues:

  1. Implicit bad gchar* (aka char*) -> std::string conversions. If a nullptr is passed into the std::string constructor, a segfault will occur.
  2. Missing pads in the unit tests

In KVS client & stream initialization, the kvssink->content_type (gchar*) field is obtained from the element directly to the left (pad). However, in the unit tests, the pad was not constructed or attached. As such, that kvssink->content_type is a nullptr. When it gets passed into the StreamDefinition constructor, it gets implicitly cast into the std::string.

Note that the null inputs case is only possible through programatic use of GStreamer and kvssink. When using gst-launch-1.0 or gst_parse_launch(), properties can't be set to null, since the kvssink properties are configured with default values. Hence, this effort will assist developers using kvssink programatically.

To fix this:

  1. The pads were constructed and linked in the unit tests.
  2. Added null checks to the C- char* strings before converting them to C++ strings.
  3. Added new unit tests to check that kvssink exits gracefully if the gchar* fields are nullptrs.

2. Adding a github action to build and run the unit tests

2.1. Adding a session token parameter to the unit tests

  • Since we need to use temporary credentials, the gstreamer unit tests now have the session token logic added.

2.2. Running the repaired gstreamer tests automatically for every pull request

  • Using the docker images from the AWS ECR gallery to avoid random github actions runner updates from breaking the builds.

Testing:

  • Ran it on my local setup to check that the tests pass. Tried both with session token and without.
  • The new error messages show up as part of the GStreamer logs. Run GST_DEBUG=4 ./tst/gstkvsplugintest.
0:00:00.035717708 33125 0x600000e8c5d0 WARN                 kvssink gstkvssink.cpp:1672:gst_kvs_sink_change_state:<kvssink> error: Failed to init kvs producer. Error: Assertion failed: kvssink->stream_name is unexpectedly NULL!

0:00:00.031656167 25576 0x600000f5c960 WARN                 kvssink gstkvssink.cpp:1672:gst_kvs_sink_change_state:<kvssink> error: Failed to init kvs producer. Error: Assertion failed: kvssink->content_type is unexpectedly NULL!

3. Verifying the debug dump dir and kvssink on different flavors (Windows, mac, linux).

  • Run kvssink with videotestsrc for 15 seconds with the KVS_DEBUG_DUMP_DATA_FILE_DIR set. Then, run mkvinfo on the output MKV file to verify that it's generated.

Note

The valgrind checks currently work but have been commented out as they are failing. Fixing the memory leak in the kvssink unit tests can come as part of a future development.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@sirknightj sirknightj force-pushed the kvssink-tests branch 4 times, most recently from 59526a2 to cc57fab Compare January 24, 2025 07:05
@sirknightj sirknightj force-pushed the kvssink-tests branch 9 times, most recently from fd1514b to c21ae41 Compare February 4, 2025 06:26
@sirknightj sirknightj force-pushed the kvssink-tests branch 2 times, most recently from 543e32a to adb4c62 Compare February 5, 2025 06:53
@sirknightj sirknightj changed the title Repair kvssink unit tests and CI kvssink CI enhancements! Feb 5, 2025
@sirknightj sirknightj marked this pull request as ready for review February 5, 2025 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.4.3 CI gstreamer Changes to kvssink or the kvssink samples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant