Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Voice Broadcast - Improve live indicator icon rendering #7579

Merged
merged 11 commits into from Nov 15, 2022

Conversation

Florian14
Copy link
Contributor

@Florian14 Florian14 commented Nov 14, 2022

Type of change

  • WIP Feature
  • Bugfix
  • Technical
  • Other :

Content

Improve the rendering of the "live" indicator icon in the timeline.
Fixed a bug on the playlist recovery which was handling events from a potential previous voice broadcast.
Minor cleanup.

Motivation and context

Continue #7127

The live indicator icon is not updated according to the following rules:

image

Screenshots / GIFs

voice_broadcast_live_indicator.mp4

Tests

  • Start recording a Voice Broadcast from a device A
  • Open the room timeline on a device B and observe that the indicator is visible & grey
  • Start listening the VB from the device B
  • Observe that the live indicator is visible & red while buffering and when playing a new chunk
  • Move backward in the playback & observe that the indicator switches to grey
  • Move forward and observe that the indicator is red only if you're in the last chunk (last 2 minutes)
  • Pause and observe that the indicator is grey, resume and observe that it is red only if you're in the last chunk (last 2 minutes)
  • From the device A, pause the VB and observe on device B that the icon is grey and stays grey regardless of the playing state
  • Resume the recording and observe that the indicator is red only if you're in the last chunk (last 2 minutes)
  • Stop the recording, the indicator should disappear for both devices

Tested devices

  • Physical
  • Emulator
  • OS version(s):

Checklist

@Florian14 Florian14 force-pushed the feature/fre/voice_broadcast_live_indicator branch from a9b1637 to 288fc35 Compare November 14, 2022 09:56
@Florian14 Florian14 requested review from a team and jmartinesp and removed request for a team November 14, 2022 10:26
@jmartinesp
Copy link
Contributor

jmartinesp commented Nov 14, 2022

The behaviour here is a bit weird:
weird_behaviour

If you're on a position in the latest chunk, you'll see the Live icon enabled. If you go back to a position in a previous chunk, that's disabled. However, if you're on the latest chunk and go to a previous position in the latest chunk too, the Live icon will become disabled.

EDIT: it seems to be a expected behaviour.

@jmartinesp
Copy link
Contributor

Also, it might be a bit subjective, but the debounced clicks on the -30s and +30s buttons might be a bit too 'eager'? I hit the -30s several times trying to go back ~1:30min and it seemed to get 'stuck', I just realised what was happening because I'm familiar with our 'debouncedClicks` function in the codebase, but at first it seemed like some weird bug.

@jmartinesp
Copy link
Contributor

I also found this:
Grabación de pantalla 2022-11-14 a las 15 44 22 mov

The buffering icon appeared although I'm not at the end of the stream - in fact, the audio kept playing. It fixed itself after some time, but maybe there's a missing check for the current position in the stream?

Copy link
Contributor

@jmartinesp jmartinesp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since 2 of the issues I mentioned are not strictly related to the new feature and the other one seems to be expected behaviour, I'd say it LGTM. Just a couple of suggestions.

private fun updateLiveListeningMode(seekPosition: Int? = null) {
isLiveListening = when {
// the current voice broadcast is not live (ended)
!currentVoiceBroadcastEvent?.isLive.orFalse() -> false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Little nit pick here: I'd either use .not() at the end or .isLive == false, the current version makes the starting ! easy to miss.

val seekDirection = seekPosition.compareTo(getCurrentPlaybackPosition() ?: 0)
when {
// backward
seekDirection < 0 -> false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that's what caused one of the issues I mentioned. I guess it's a expected behaviour then.

when {
// backward
seekDirection < 0 -> false
// forward: check if new sequence is the last one
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: last -> latest?

@Florian14
Copy link
Contributor Author

@jmartinesp Thanks for the review, I took your feedback into account:

  • I removed the click debounces on the player (I agree it was not nice for the UX)
  • I removed the trick about the live changes when we seek backward in the same chunk, It was a try to improve the live icon rendering but it can be disturbing to be back in live by just seeking a bit forward again.

About the buffering, I thought it was fixed but I also reproduced it, I still have an issue related to the chunks recovery in my player, I will consider it in a dedicated bugfix.

@sonarcloud
Copy link

sonarcloud bot commented Nov 15, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@Florian14 Florian14 merged commit 4ac9c8d into develop Nov 15, 2022
@Florian14 Florian14 deleted the feature/fre/voice_broadcast_live_indicator branch November 15, 2022 16:25
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.

None yet

3 participants