SntpClient periodically re-syncs the offset to NTP server

[stevemayhew]

This is the equivalent pull request to #697, just rebased (with simple conflict resolve) to the main branch.

@marcbaechinger feel free to pick one and close the other.

[marcbaechinger]

This looks good to me as in how this would work regarding reinitialization. Many thanks, Steve.

I was wondering whether we want to use C.TIME_UNSET to never reinitialize as a default. Mainly to not change the behaviour of DashMediaSource without opting in. Then we could do such a getter similar to those for timeout and sntp host to allow opt-in in.

We would then wouldn't need to discuss how you came to choose 10 minutes as a default. :)

You as a user could then opt-in once at app level using the SntpClient API? I'm not really convinced. However, the static design of that client makes it a bit difficult to provide an API that could be enabled on a media source level or similar. SntpClient being a proper object instance would probably be the right thing to do.

How does adding a getter to override the default sound to you?

[marcbaechinger]

As mentioned in the main comment. Consider adding a getter to override the default of C.TIME_UNSET and disable reinitialization for C.TIME_UNSET?

[marcbaechinger]

UNTIL ? :)

[marcbaechinger]

As mentioned in the PR comment. Is this a value that comes from your experience when operating live streams? I'm mostly curious...

[nikitapn]

This is what we also need, but could you remove hardcoded MAX_ELAPSED_MS_TILL_UPDATE?

From our experience with media boxes that we have, the sync interval of 12 hours is adequate.
We have some statistics gathered from deltas of offsets after some time. In our case, the drift is ~0.2 seconds per 12 hours. So we would like to decide sync interval for ourselves by having a setter.
Also Android OS only does "a single time query approximately once a day" , and frequent NTP sync might possibly affect zap time every 10 minutes user switches a channel.
Thanks.

[stevemayhew]

Having it default to original behavior with a setter to enable the refresh (and set the time) sounds fine to me. It is possible this issue is limited to Android Streamers as phones likely time sync with the carrier.

Also, for phones issuing the network request has at least some small battery penalty

Feel free to add the commit with the change, you should have push access to the repo

[marcbaechinger]

Ok, thanks! Will proceed that way.

[marcbaechinger]

I pushed a commit with the getter and the release notes. I'm going to send this for internal review now. You may see some more commits being added as I make changes in response to review feedback. Please refrain from pushing any more substantive changes as it will complicate the internal review.

Let me know if you see something, Steve. Else I proceed as described above. Thanks again!

[stevemayhew]

No worries. Feel free to force push or whatever you need, branch is yours know. We’ll update our side so code matches

…

On Mon, Oct 21, 2024 at 10:55 AM Marc Baechinger ***@***.***> wrote: I pushed a commit with the getter and the release notes. I'm going to send this for internal review now. You may see some more commits being added as I make changes in response to review feedback. Please refrain from pushing any more substantive changes as it will complicate the internal review. Let me know if you see something, Steve. Else I proceed as described above. Thanks again! — Reply to this email directly, view it on GitHub <#1794 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADBF6EJ6IMUQZXBWHKKVTDZ4U52HAVCNFSM6AAAAABPU4XPSWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMRXGM3DMOJZGI> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[marcbaechinger]

@stevemayhew @nikitapn

I have that CL ready for submission (see commits in this PR).

May I ask you both how you are going to use that feature? Are you having your own media source that is using the client? I'm asking because when used with DashMediaSource then loadNtpTimeOffset() is called in [1]. If that succeeds the callback will set elapsedRealtimeOffsetMs in [2].

Once elapsedRealtimeOffsetMs is set it will be used in the if expression that would call the SntpClient if it was C.TIME_UNSET. But once set by SntpClient it never passes that expression inn [3].

Is the use case for this change usage in DashMediaSource or are you having your own media source?

[1] https://github.com/androidx/media/blob/release/libraries/exoplayer_dash/src/main/java/androidx/media3/exoplayer/dash/DashMediaSource.java#L715
[2] https://github.com/androidx/media/blob/release/libraries/exoplayer_dash/src/main/java/androidx/media3/exoplayer/dash/DashMediaSource.java#L863
[3] https://github.com/androidx/media/blob/release/libraries/exoplayer_dash/src/main/java/androidx/media3/exoplayer/dash/DashMediaSource.java#L710

[nikitapn]

In our case, users experience the issue when they bring the media box out of standby mode. During this process, we recreate the player (our wrapper with all the stuff) completely because the instance has been destroyed before entering standby, and elapsedRealtimeOffsetMs will be TIME_UNSET in the new DashMediaSource instance.

I suspect we may still encounter some problems when users have been watching a live stream without turning the box off for a few weeks, right? In my opinion, this is a rare situation.

[marcbaechinger]

Ah, I see. Yeah, obviously. Thanks for clarifying. I didn''t realized that it wouldn't re-initialize even when a new source is created. This case can be handled with this change as you say. The design of that SntpClient class with these static methods is, lets say, peculiar. But yeah, what you saying makes totally sense! Thanks!

Will submit as it is. If we need to enable this re-initialization inside the same DashMediaSource instance, we can look into this again.