Skip to content

Added move semantics to AJAAncillaryList.#38

Open
zerodefect wants to merge 2 commits intoaja-video:mainfrom
zerodefect:feature/move_semantics_in_ajaancillarylist
Open

Added move semantics to AJAAncillaryList.#38
zerodefect wants to merge 2 commits intoaja-video:mainfrom
zerodefect:feature/move_semantics_in_ajaancillarylist

Conversation

@zerodefect
Copy link
Copy Markdown
Contributor

The AJAAncillaryList encapsulates the following data types:

	AJAAncillaryDataList	m_ancList;		///< @brief	My packet list
	bool					m_rcvMultiRTP;	///< @brief	True: Rcv 1 RTP pkt per Anc pkt;  False: Rcv 1 RTP pkt for all Anc pkts
	bool					m_xmitMultiRTP;	///< @brief	True: Xmit 1 RTP pkt per Anc pkt;  False: Xmit 1 RTP pkt for all Anc pkts
	bool					m_ignoreCS;		///< @brief	True: ignore checksum errors;  False: don't ignore CS errors

...and AJAAncillaryDataList is a typedef for:

typedef std::vector <AJAAncillaryData*>			AJAAncillaryDataList;

which means the underlying types already have support for move semantics with a C++11 compiler.

I have the following function in my code:

AJAAncillaryList content_processor_aja_sdi_output::get_vanc_data_to_insert(...)
{
    AJAAncillaryList pkts;
    // ...
    return pkts;
}

Every time this function is called, instead of moving the packets, the packets get copied (allocation/deallocation per ancillary type) when they can merely be moved.

Changes tested within a linux environment - not tested on Windows. Need to be careful of introducing memory leaks, but I think I've covered all bases.

@zerodefect
Copy link
Copy Markdown
Contributor Author

Cleaned up the code/doxygen comments.

@zerodefect
Copy link
Copy Markdown
Contributor Author

There is a question on what should happen on a move. Read this here (for std::optional):

https://stackoverflow.com/questions/51805059/why-does-moving-stdoptional-not-reset-state

A commenter writes:

Unless otherwise specified, a moved-from object of class type is left in a valid but unspecified state. Not necessarily a "reset state", and definitely not "invalidated".

Why is that relevant. For the move-assignment operator, it could be argued that:

AJAAncillaryList & AJAAncillaryList::operator = (AJAAncillaryList && inRHS)
{
	if (this != &inRHS)
	{
		m_xmitMultiRTP = inRHS.m_xmitMultiRTP;
		inRHS.m_xmitMultiRTP = false;

		m_rcvMultiRTP = inRHS.m_rcvMultiRTP;			
		inRHS.m_rcvMultiRTP	= true;
		
		m_ignoreCS = inRHS.m_ignoreCS;
		inRHS.m_ignoreCS = false;
		
		Clear(); // Clear down any packets currently being held in 'this'
		m_ancList = std::move(inRHS.m_ancList);
	}
	return *this;
}

should become:

AJAAncillaryList & AJAAncillaryList::operator = (AJAAncillaryList && inRHS)
{
	if (this != &inRHS)
	{
		m_xmitMultiRTP = inRHS.m_xmitMultiRTP;
		m_rcvMultiRTP = inRHS.m_rcvMultiRTP;
		m_ignoreCS = inRHS.m_ignoreCS;
		
		Clear(); // Clear down any packets currently being held in 'this'
		m_ancList = std::move(inRHS.m_ancList);
	}
	return *this;
}

and:

AJAAncillaryList::AJAAncillaryList(AJAAncillaryList && inRHS)
	:	m_ancList(std::move(inRHS.m_ancList)),
		m_rcvMultiRTP(inRHS.m_rcvMultiRTP),
		m_xmitMultiRTP(inRHS.m_xmitMultiRTP),
		m_ignoreCS(inRHS.m_ignoreCS)
{
	// Reset RHS.
	inRHS.m_rcvMultiRTP = true;
	inRHS.m_xmitMultiRTP = false;
	inRHS.m_ignoreCS = false;
	// inRHS.m_ancList - already moved/reset.
}

should become:

AJAAncillaryList::AJAAncillaryList(AJAAncillaryList && inRHS) = default;

Thoughts? You are more than welcome to make changes to my commits as you so choose.

paulh-aja pushed a commit that referenced this pull request Nov 20, 2024
• added “BFT_AncListMoveSemantics” test case to ut_ajaanc, plus minor edits
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.

1 participant