On Wed May 24 18:44:37 2023 +0000, Rémi Bernon wrote:
We don't have to send the segment end timestamp, and it works just the same if we use -1 like for when it was started. What matters it to restore the timestamp for stream start so that buffer times are kept consistent. I don't see how it is different to keep the timestamp in a field vs keep the segment. I prefer keeping the segment as it saves useless local variable (and segment initialization every time we want to create one).
Well, currently, "segment" doesn't need to be a local variable. I don't see how "saving a useless local variable" is preferable to saving a useless struct field. If I see a struct field, I expect there's some reason it needs to be a struct field.
Along the same lines, if I see a struct field, I expect there to be some reason it has the type it does. I don't see how that applies to "segment" as of patch 5/10. We're not using it as a segment; we're only using the start time. Well, patch 6/10 uses it as a segment proper, but the point is we never actually use the end time. That seems potentially unclear or confusing to a reader, who would expect that if we're saving a whole segment in the struct that we're going to update all its members, and wastes a bit of mental time trying to tell if the stop time is ever updated.
I don't hate the current organization, so I'm not going to block the patch series simply based on that, but I do think it would be better to only track the members we are using.