[core/cyclicbuffer] : buffer reads should not let the tail exceed the head.

[msieben]

Custom lengths should consider (maximum) distance between tail and head.

[pwielders]

I am hoping that this is not needed. Length should always be oke. It could be that after line 363 more data was written into the cyclic buffer and it is up to the consumer of the cursor (GetReadSize() in line 370) to see if he want to read it. The only restriction for this Consumer is the amount of data that can be stored into the passed in Read Buffer with length " length". So I do not think this change fixes any usecase, potentially it only slows down the read process casue now if during line 364 and 370 more data is added to the cyclic buffer it is not treated as being available, where the consumer of the cursor in line 370 might now conclude that it can not read that data as the buffer to store it in is to small (limited to the amount of data that was available at line 363) but maybe the read required 2 or more writes for a full frame to load.
Is there a uses case that is now fixed with this change? If so I would like to evaluate that use-case.

[msieben]

You might have to clarify, perhaps, offline. I believe I shared the reasoning with @MFransen69 via our internal communication medium. If memory serves me right it relates to the memcpy a few code lines further. I will share that reasoning.

[MFransen69]

Hi @msieben I cannot remember we discussed this in too much detail, so better indeed to share it here.

[msieben]

@MFransen69 I already shared the transcript with @pwielders

[msieben]

If analysis is correct, but correct me if I am wrong.

In short what has been shared.

https://github.com/rdkcentral/Thunder/blob/master/Source/core/CyclicBuffer.cpp#L369-L370
https://github.com/rdkcentral/Thunder/blob/master/Source/core/CyclicBuffer.cpp#L694
https://github.com/rdkcentral/Thunder/blob/master/Source/core/CyclicBuffer.h#L57
https://github.com/rdkcentral/Thunder/blob/master/Source/core/CyclicBuffer.h#L80

Suggest result is equivalent to length.

https://github.com/rdkcentral/Thunder/blob/master/Source/core/CyclicBuffer.cpp#L385

Under the assumption of available data bufferLength becomes equivalent to length.

This may affect memcpy if buffer size allows and then length 'dictates' what will be copied.

https://github.com/rdkcentral/Thunder/blob/master/Source/core/CyclicBuffer.cpp#L390
https://github.com/rdkcentral/Thunder/blob/master/Source/core/CyclicBuffer.cpp#L407
https://github.com/rdkcentral/Thunder/blob/master/Source/core/CyclicBuffer.cpp#L410

[msieben]

@pwielders, if analysis is correct and performance is paramount then an ASSERT might be used instead since the std::min is equivalent to a comparison and assignment (a = (b < c ? b : c)), and, the comparison suffices (ASSERT(b < c)).

[VeithMetro]

@msieben I am not sure if this is still necessary after our latest changes to CyclicBuffer from this commit: 9cd25af

If your analysis indicates that there could be a problem, maybe we could write a quick test to prove that there might in fact be an issue in a specific corner case? Even if it turns out there is no problem after the newest changes, this test could still prove useful in the future to prevent some potential issues 😄

[msieben]

Closing PR. Low priority. More information at https://jira.rdkcentral.com/jira/browse/METROL-979