Fix #438 by accounting for the fact that only half of the buffer of a continuous flow is readable at any one point in time

[KimonHoffmann]

The configured history duration is used to determine the buffer size, but before this change this failed to account for the fact that only half of the buffer is readable at any one point in time, effectively halving the history duration for all continuous flows.

[github-actions]

Test Results

70 tests  ±0   70 ✅ ±0   17s ⏱️ ±0s
 1 suites ±0    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit 0d5b266. ± Comparison against base commit 3394ee9.

♻️ This comment has been updated with latest results.

[pedro-alves-ferreira]

Thanks for fixing this. LGTM, with one comment.

[pedro-alves-ferreira]

I would add a comment to make it clearer. Something like: "The denominator is multiplied by 500M, rather than 1B, so that the buffer length is twice as large as the history duration."

[KimonHoffmann]

Fair point, I added an explanatory comment.

[vt-tv]

@KimonHoffmann could you add the unit test that was attached to the bug report? that would validate your fix

[KimonHoffmann]

@KimonHoffmann could you add the unit test that was attached to the bug report? that would validate your fix

Unfortunately not, because it lacks a DCO signoff and does not follow the practices of our unit tests (it has intermittent console output, for example). I can try to attach a clean room test instead.

[SpungeCake]

Hi, Serge from GV here, I'm the one who reported the problem to Vincent Trussart. This is not an acceptable fix, I tried it with the unit test I wrote that reproduces the issue, still not fixed. Doubling the history will make the size match the video and anc duration but for an audio only configuration the returned size is invalid since only half of it is usable. That makes the test fail.

[KimonHoffmann]

Hi, Serge from GV here, I'm the one who reported the problem to Vincent Trussart. This is not an acceptable fix, I tried it with the unit test I wrote that reproduces the issue, still not fixed. Doubling the history will make the size match the video and anc duration but for an audio only configuration the returned size is invalid since only half of it is usable. That makes the test fail.

You mean that only half of mxlContinuousFlowConfigInfo::bufferLength is actually accessible for reading?
While I agree that this does not match the way in which mxlDiscreteFlowConfigInfo::grainCount can be used, we claim nowhere that all of bufferLength is accessible for reading. On the contrary the documentation in architecture.md explicitly states otherwise.

We can consider how to improve the UX on this, I'm not really in favor of having the physical buffer length diverge from mxlContinuousFlowConfigInfo::bufferLength, but maybe an easy to use inline function to obtain the usable min-index for a given flow could be a way to help users?

[SpungeCake]

My point is mainly that a flow writer and reader user need to know exactly how much data a flow can hold and when it is outside the range. This fixes the continuous buffer duration not matching the discrete flow, I'm ok with this. However we still need to provide a way for a continuous user to know the exact history duration. I just find it awkward that I have to divide by 2 the bufferLength given by the mxlContinuousFlowConfigInfo, we need a cleaner way, maybe inline function will do the work. The fact that mxlFlowConfigInfo is a union of the two types of config makes you think it returns a similar information about the buffer duration, I'm pretty sure more will do the same mistake and not divide the bufferLength by two. At least flowinfo.h header should be updated to explain this for mxlContinuousFlowConfigInfo.

[KimonHoffmann]

Yes, I agree that the UX as it is right now is error prone.
I think an inline function and a comment in mxlContinuousFlowConfigInfo referring to it should do the trick.