Frame Rate Converter node #46

shamilatesoglu · 2026-01-07T13:50:43Z

This node adds a generic frame-rate conversion capability between producer and consumer threads.

Plugins/nosReflect/Source/RingBuffer.hpp

caner-milko · 2026-01-07T13:55:56Z

Plugins/nosReflect/Source/RingBuffer.hpp

+	{
+		std::unique_lock lock(Mutex);
+		Tail = (Tail + count) % Capacity;
+		Size = (Size >= count) ? (Size - count) : 0;


soft check on this case being false?

caner-milko · 2026-01-07T13:57:42Z

Plugins/nosReflect/Source/FrameRateConverter.cpp

+
+	bool CapacityUpdatedViaPathCommand = false;
+
+	FrameRateConverterNode() : Ring(1, RingBufferServeMode::WaitUntilFull)


This node's serve mode should be either dynamic(via pin or template parameter on nosnode) or we can make two separate nodes

Copilot

Pull request overview

This PR introduces a new Frame Rate Converter node that enables generic frame-rate conversion between producer and consumer threads. The implementation refactors the existing RingBuffer infrastructure to support runtime mode selection and adds multi-element push/pop operations.

Key changes:

New FrameRateConverter node that can convert between different frame rates using a configurable ratio
Refactoring of RingBuffer from compile-time template-based mode selection to runtime-based mode configuration
Addition of multi-element push/pop operations (BeginPushMultiple, EndPushMultiple, BeginPopMultiple, EndPopMultiple) to support batch operations

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 15 comments.

Show a summary per file

File	Description
Plugins/nosReflect/Types/Reflect.fbs	Adds RingBufferServeMode enum to replace compile-time template parameter
Plugins/nosReflect/Source/RingBuffer.hpp	Refactors RingBuffer to use runtime mode selection and adds multi-element push/pop methods; migrates pin watching from OnPinObjectChanged to AddPinValueWatcher
Plugins/nosReflect/Source/RingBufferNodes.cpp	Updates node definitions to use explicit constructors with runtime mode specification
Plugins/nosReflect/Source/BoundedQueueNodes.cpp	Updates node definitions to use explicit constructors with ServeImmediately mode
Plugins/nosReflect/Source/FrameRateConverter.cpp	Implements new FrameRateConverter node with ratio-based frame rate conversion using multi-element ring buffer operations
Plugins/nosReflect/Source/PluginMain.cpp	Registers the new FrameRateConverter node
Plugins/nosReflect/Nodes/FrameRateConverter.nosnode	Defines the FrameRateConverter node configuration with Capacity and Ratio pins

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-07T13:57:40Z

Plugins/nosReflect/Source/RingBuffer.hpp

+		{
+			State = RingState::Serving;
+			ReadyForPopCV.notify_all();
+		}


The notification logic after EndPushMultiple has changed from the previous implementation. Previously, when Mode was ServeImmediately (old WaitUntilFull == false), consumers were notified on every push. Now, consumers are only notified when transitioning from Filling to Serving state. This means in ServeImmediately mode, consumers won't be notified when new data becomes available, potentially causing them to wait until timeout. Add notification when State is already Serving to maintain the previous behavior.

Suggested change

}

}

else if (State == RingState::Serving)

{

// In ServeImmediately mode (or once already serving), notify consumers on each push

ReadyForPopCV.notify_all();

}

Copilot · 2026-01-07T13:57:41Z

Plugins/nosReflect/Source/RingBuffer.hpp

+	explicit RingBuffer(size_t capacity, RingBufferServeMode mode = RingBufferServeMode::WaitUntilFull)
 		: Capacity(capacity),
-		Buffer(capacity),
+		Buffer(),
 		Head(0),
 		Tail(0),
 		Size(0),
 		ExitRequested(false)
 	{
+		Reset(capacity, mode);
 	}


The Reset method should initialize the Mode member before the constructor body is reached. Currently, the constructor calls Reset(capacity, mode) at line 26, but the Mode member is already initialized with a default value at line 234. The Reset call should properly set Mode, but it's cleaner to initialize Mode in the initializer list using the mode parameter directly.

Copilot · 2026-01-07T13:57:41Z

Plugins/nosReflect/Source/RingBuffer.hpp

+		if (State == RingState::Filling && Size == Capacity)
+		{
+			State = RingState::Serving;
+			ReadyForPopCV.notify_all();


The notification strategy after EndPushMultiple should be consistent with EndPush. EndPush uses notify_one() when transitioning to Serving state (line 73), but EndPushMultiple uses notify_all() (line 85). This inconsistency could lead to unnecessary wake-ups. Consider using notify_one() here for consistency unless there's a specific reason multiple waiters need to be notified.

Suggested change

ReadyForPopCV.notify_all();

ReadyForPopCV.notify_one();

Copilot · 2026-01-07T13:57:41Z

Plugins/nosReflect/Source/RingBuffer.hpp

+		Tail = (Tail + count) % Capacity;
+		Size = (Size >= count) ? (Size - count) : 0;
+		lock.unlock();
+		ReadyForPushCV.notify_all();


The notification strategy after EndPopMultiple should be consistent with EndPop. EndPop uses notify_one() (line 137), but EndPopMultiple uses notify_all() (line 146). This inconsistency could lead to unnecessary wake-ups. Consider using notify_one() here for consistency unless there's a specific reason multiple waiters need to be notified.

Suggested change

ReadyForPushCV.notify_all();

ReadyForPushCV.notify_one();

Copilot · 2026-01-07T13:57:41Z

Plugins/nosReflect/Source/FrameRateConverter.cpp

+		auto lcm = std::lcm(Ratio.x(), Ratio.y());
+		auto requiredEffectiveCapacity = lcm * Capacity;


The calculation of requiredEffectiveCapacity at line 51 can overflow if lcm * Capacity exceeds the maximum value of uint32_t. This could happen with large ratio values and capacity. Consider checking for potential overflow before the multiplication or using a larger integer type for the intermediate calculation.

Copilot · 2026-01-07T13:57:43Z

Plugins/nosReflect/Source/RingBuffer.hpp

 		ReadyForPushCV.notify_all();
 	}

+	RingBufferServeMode GetMode() const


The GetMode method should be marked as noexcept since it only returns a member variable and cannot throw exceptions. This is consistent with GetCapacity which is already marked noexcept.

Suggested change

RingBufferServeMode GetMode() const

RingBufferServeMode GetMode() const noexcept

Copilot · 2026-01-07T13:57:43Z

Plugins/nosReflect/Source/RingBuffer.hpp

+				{
+					nosPathCommand ringSizeChange{.Event = NOS_RING_SIZE_CHANGE, .RingSize = Capacity};
+					nosEngine.SendPathCommand(*GetPinId(NSN_Input), ringSizeChange);
+					CapacityUpdatedViaPathCommand = false;


The CapacityUpdatedViaPathCommand flag is checked at line 291 to prevent sending a path command if it was already updated via path command, but then it's immediately set to false at line 295. This makes the check ineffective as the flag will always be false after this code executes. The assignment at line 295 should be removed, as the flag should remain true after being set and only be reset when the path command is actually processed.

Suggested change

CapacityUpdatedViaPathCommand = false;

Copilot · 2026-01-07T13:57:43Z

Plugins/nosReflect/Source/FrameRateConverter.cpp

+		auto requiredEffectiveCapacity = lcm * Capacity;
+		if (requiredEffectiveCapacity != EffectiveCapacity)
+		{
+			EffectiveCapacity = requiredEffectiveCapacity;


After updating EffectiveCapacity at line 54, the Ring buffer should be reset to reflect the new capacity. However, there's no Ring.Reset call here. The reset only happens later when SendPathRestart is called at line 56, which will eventually trigger OnPathStart. Consider calling Ring.Reset(EffectiveCapacity) here to ensure the ring buffer capacity is consistent with EffectiveCapacity.

Suggested change

EffectiveCapacity = requiredEffectiveCapacity;

EffectiveCapacity = requiredEffectiveCapacity;

Ring.Reset(EffectiveCapacity);

@copilot Reset should happen after path restart.

Copilot · 2026-01-07T13:57:44Z

Plugins/nosReflect/Source/FrameRateConverter.cpp

+		TypeInfo typeInfo(params->IncomingTypeName);
+		if (typeInfo->BaseType != NOS_BASE_TYPE_ARRAY)
+		{
+			strncpy(params->OutErrorMessage, "Connected pin type must be an array type!", 42);


The strncpy call copies exactly 42 bytes, but the string "Connected pin type must be an array type!" is 43 characters long (including the null terminator). This will result in a non-null-terminated string in the error message buffer. Either increase the count to include the null terminator or use a safer alternative like snprintf.

@copilot Are you sure?

Copilot · 2026-01-07T13:57:44Z

Plugins/nosReflect/Source/FrameRateConverter.cpp

+
+	FrameRateConverterNode() : Ring(1, RingBufferServeMode::WaitUntilFull)
+	{
+		Ring.Reset(EffectiveCapacity);


The Ring.Reset call at line 34 does not specify the mode parameter, so it will keep the mode from the constructor. However, it's called before the EffectiveCapacity is calculated from the pin values. Since EffectiveCapacity is 1 at this point and will be recalculated later, this Reset call may be unnecessary or should be removed to avoid confusion.

Suggested change

Ring.Reset(EffectiveCapacity);

Copilot · 2026-01-07T14:09:05Z

@shamilatesoglu I've opened a new pull request, #47, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot · 2026-01-07T14:10:26Z

@shamilatesoglu I've opened a new pull request, #48, to work on those changes. Once the pull request is ready, I'll request review from you.

Co-authored-by: Caner <[email protected]>

shamilatesoglu added 2 commits January 7, 2026 12:45

WIP: Add frame rate converter cross-thread node

e7d9054

FrameRateConverter: Improve robustness and add status messages

b509551

shamilatesoglu requested a review from Copilot January 7, 2026 13:50

Copilot started reviewing on behalf of shamilatesoglu January 7, 2026 13:51 View session

caner-milko reviewed Jan 7, 2026

View reviewed changes

Plugins/nosReflect/Source/RingBuffer.hpp Outdated Show resolved Hide resolved

caner-milko reviewed Jan 7, 2026

View reviewed changes

Copilot AI reviewed Jan 7, 2026

View reviewed changes

Simplify RingBuffer data structure

f72a876

Copilot AI mentioned this pull request Jan 7, 2026

Clarify strncpy usage in FrameRateConverter error handling #47

Closed

Copilot AI mentioned this pull request Jan 7, 2026

Confirm Ring.Reset() timing is correct after path restart #48

Closed

shamilatesoglu marked this pull request as draft January 7, 2026 14:19

shamilatesoglu and others added 2 commits January 7, 2026 17:53

FrameRateConverter: Fix repeats when on ring mode

7707da5

FrameRateConverter: Send correct number of schedule requests

aedb559

Co-authored-by: Caner <[email protected]>

shamilatesoglu force-pushed the wip-frame-rate-converter branch from 3ca3cea to aedb559 Compare January 7, 2026 15:41

caner-milko approved these changes Jan 7, 2026

View reviewed changes

Soft-checks and better names in RingBuffer

73b13e5

shamilatesoglu force-pushed the wip-frame-rate-converter branch from 345dec4 to 73b13e5 Compare January 7, 2026 15:59

shamilatesoglu marked this pull request as ready for review January 7, 2026 15:59

shamilatesoglu merged commit c0cc667 into dev Jan 7, 2026

caner-milko deleted the wip-frame-rate-converter branch January 8, 2026 09:26


		bool CapacityUpdatedViaPathCommand = false;

		FrameRateConverterNode() : Ring(1, RingBufferServeMode::WaitUntilFull)

-		}
+		}
+		else if (State == RingState::Serving)
+		{
+			// In ServeImmediately mode (or once already serving), notify consumers on each push
+			ReadyForPopCV.notify_all();
+		}

		auto lcm = std::lcm(Ratio.x(), Ratio.y());
		auto requiredEffectiveCapacity = lcm * Capacity;

	RingBufferServeMode GetMode() const
	RingBufferServeMode GetMode() const noexcept

	EffectiveCapacity = requiredEffectiveCapacity;
	EffectiveCapacity = requiredEffectiveCapacity;
	Ring.Reset(EffectiveCapacity);

Frame Rate Converter node #46

Frame Rate Converter node #46

Uh oh!

Conversation

shamilatesoglu commented Jan 7, 2026

Uh oh!

Uh oh!

caner-milko Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

caner-milko Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

shamilatesoglu Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

shamilatesoglu Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Jan 7, 2026

Uh oh!

Copilot AI commented Jan 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants