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.

Sign up for GitHub

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

Jump to bottom

Move to another thread #261

Closed
wants to merge 3 commits into from
Closed

Move to another thread #261

wants to merge 3 commits into from

Conversation

hokyungh
Copy link
Contributor

@hokyungh hokyungh commented Apr 2, 2021 •
edited
Loading

Issue #, if available:

#260

Description of changes:

Put audioClient start logic into different thread.

Testing done:

There are few test cases I have done

  1. Start and stop (normal) and rejoin
  2. Use same audioVideo and start and stop multiple times really fast. (It seems to be queued and does not start while stopping)

Theoretical side effect (not observed)
Instead of main thread, we are running on IO thread, so it could be possible that it is executed later.

Unit test coverage

  • Class coverage: 84%
  • Line coverage: 67%

Manual test cases (add more as needed):

  • Join meeting
  • Leave meeting
  • Rejoin meeting
  • Send audio
  • Receive audio
  • See active speaker indicator when speaking
  • Mute/Unmute self
  • See local mute indicator when muted
  • See remote mute indicator when other is muted
  • See audio video events
  • See signal strength changes
  • See media metrics received
  • See roster updates when remote attendees join / leave the meeting
  • Enable local video
  • See local video tile
  • See remote video tile
  • Switch camera
  • See remote screen sharing content with attendee name
  • Pause remote video tile
  • Resume remote video tile
  • First time audio permissions
  • First time video permissions

Screenshots, if available:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

linsang21
linsang21 reviewed Apr 7, 2021
View reviewed changes
Copy link
Contributor

@linsang21 linsang21 left a comment

Choose a reason for hiding this comment

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

What is the motivation of the change?

@hokyungh
Copy link
Contributor Author

hokyungh commented Apr 7, 2021

What is the motivation of the change?

updated the description.

linsang21
linsang21 reviewed Apr 7, 2021
View reviewed changes
...ava/com/amazonaws/services/chime/sdk/meetings/internal/audio/DefaultAudioClientController.kt
@@ -30,7 +30,8 @@ class DefaultAudioClientController(
private val audioClientObserver: AudioClientObserver,
private val audioClient: AudioClient,
private val meetingStatsCollector: MeetingStatsCollector,
private val eventAnalyticsController: EventAnalyticsController
private val eventAnalyticsController: EventAnalyticsController,
private val audioClientScope: CoroutineScope = CoroutineScope(Dispatchers.IO + SupervisorJob())
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason we use SupervisorJob?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SupervisorJob is independent job, so any children of job failure will not impact this job.

Copy link
Contributor

Choose a reason for hiding this comment

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

What use case would require that?

Copy link
Contributor Author

@hokyungh hokyungh Jun 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for missing it, so you could end up having

CoroutineContext
     CoroutineContext
          CouroutineContext

In this case, if the parent fails somehow, the children will halt as well. If I understood correctly, SupervisorJob prevents it and run it independently.

...ava/com/amazonaws/services/chime/sdk/meetings/internal/audio/DefaultAudioClientController.kt

uiScope.launch {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should double check with Ankit in case there's a reason we chose ui thread in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We couldn't find good reason. It might be because we wanted to have higher priority on starting by putting it on UI thread?

@hokyungh hokyungh force-pushed the move-to-another-thread branch 2 times, most recently from 9656c68 to 204c19e Compare June 16, 2021 19:42
@stale
Copy link

stale bot commented Jan 9, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the inactive Stale label Jan 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@linsang21 linsang21 linsang21 left review comments

@dylonChime dylonChime dylonChime approved these changes

Assignees
No one assigned
Labels
inactive Stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hokyungh @dylonChime @linsang21 @hensmi-amazon