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

fix: Calling video not streamed when enabling camera on preview screen (WPB-7114) #2801

Merged
merged 11 commits into from
Mar 25, 2024

Conversation

ohassine
Copy link
Member

@ohassine ohassine commented Mar 20, 2024

BugWPB-7114 [Android] When video is enabled on precall screen, video is not streamed during 1:1 and group call


PR Submission Checklist for internal contributors

  • The PR Title

    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • contains a reference JIRA issue number like SQPIT-764
    • answers the question: If merged, this PR will: ... ³
  • The PR Description

    • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Issues

Calling video not streamed when enabling camera on preview screen of an incoming call.

Causes (Optional)

TLDR: A race condition.
Establishing an incoming calls takes some times an important time, causing to call the function of video stream before the call get established.

Solutions

I moved the logic of sending video feed to OngoingCallViewModel, this way we are sure setVideoSendState will be called only when the call get established.

Needs releases with:

  • GitHub link to other pull request

Testing

Test Coverage (Optional)

  • I have added automated test to this contribution

How to Test

  • Start a call to an android device.
  • On Android enable your camera and accept the call
  • You should still see your camera on, and the other person see your video feed.

PR Post Submission Checklist for internal contributors (Optional)

  • Wire's Github Workflow has automatically linked the PR to a JIRA issue

PR Post Merge Checklist for internal contributors

  • If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.

References
  1. https://sparkbox.com/foundry/semantic_commit_messages
  2. https://github.com/wireapp/.github#usage
  3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

Copy link
Contributor

@ohassine looks like you are rolling back kalium to a previous commitish.

This means that the PR's target branch (develop) is using a newer version of Kalium, and the changes in this PR will rollback Kalium to an older version.

develop This PR
18b7699e312bedf08a441433e25d96fb37dc920f d35b34badde0f7e46c3434602b5e6a2bd4dcf1f7

Is this intentional?

Copy link
Contributor

github-actions bot commented Mar 20, 2024

Test Results

878 tests  ±0   878 ✅ ±0   15m 43s ⏱️ + 5m 46s
118 suites ±0     0 💤 ±0 
118 files   ±0     0 ❌ ±0 

Results for commit ceb5b02. ± Comparison against base commit 17f1c2a.

This pull request removes 4 and adds 4 tests. Note that renamed tests count towards both.
com.wire.android.ui.calling.SharedCallingViewModelTest ‑ given a video call, when stopping video, then clear Video Preview and turn off speaker()
com.wire.android.ui.calling.SharedCallingViewModelTest ‑ given an active call, when clearVideoPreview is called, then update video state to STOPPED()
com.wire.android.ui.calling.SharedCallingViewModelTest ‑ given an active call, when setVideoPreview is called, then set the video preview and update video state to STARTED()
com.wire.android.ui.calling.SharedCallingViewModelTest ‑ given an audio call, when stopVideo is invoked, then do not do anything()
com.wire.android.ui.calling.OngoingCallViewModelTest ‑ givenAnOngoingCall_WhenTurningOffCamera_ThenSetVideoSendStateToStopped()
com.wire.android.ui.calling.OngoingCallViewModelTest ‑ givenAnOngoingCall_WhenTurningOnCamera_ThenSetVideoSendStateToStarted()
com.wire.android.ui.calling.SharedCallingViewModelTest ‑ given a call, when clearVideoPreview is called, then clear view()
com.wire.android.ui.calling.SharedCallingViewModelTest ‑ given a call, when setVideoPreview is called, then set the video preview()

♻️ This comment has been updated with latest results.

@ohassine ohassine requested review from a team, typfel, yamilmedina, mchenani, Garzas and e-lisa and removed request for a team March 20, 2024 13:29
Copy link
Contributor

@ohassine looks like you are rolling back kalium to a previous commitish.

This means that the PR's target branch (develop) is using a newer version of Kalium, and the changes in this PR will rollback Kalium to an older version.

develop This PR
18b7699e312bedf08a441433e25d96fb37dc920f d35b34badde0f7e46c3434602b5e6a2bd4dcf1f7

Is this intentional?

@ohassine ohassine removed the request for review from e-lisa March 20, 2024 13:29
@AndroidBob
Copy link
Collaborator

Build 3661 failed.

@AndroidBob
Copy link
Collaborator

Build 3662 failed.

@AndroidBob
Copy link
Collaborator

Build 3663 failed.

Copy link
Contributor

@ohassine looks like you are rolling back kalium to a previous commitish.

This means that the PR's target branch (develop) is using a newer version of Kalium, and the changes in this PR will rollback Kalium to an older version.

develop This PR
18b7699e312bedf08a441433e25d96fb37dc920f 4fc22b91cf55bda1d512ad37d287e5418a355d12

Is this intentional?

@AndroidBob
Copy link
Collaborator

Build 3664 failed.

Copy link
Contributor

@ohassine looks like you are rolling back kalium to a previous commitish.

This means that the PR's target branch (develop) is using a newer version of Kalium, and the changes in this PR will rollback Kalium to an older version.

develop This PR
18b7699e312bedf08a441433e25d96fb37dc920f 4fc22b91cf55bda1d512ad37d287e5418a355d12

Is this intentional?

Copy link

codecov bot commented Mar 20, 2024

Codecov Report

Attention: Patch coverage is 64.70588% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 43.61%. Comparing base (17f1c2a) to head (ceb5b02).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2801      +/-   ##
===========================================
+ Coverage    43.45%   43.61%   +0.16%     
===========================================
  Files          418      418              
  Lines        14000    14007       +7     
  Branches      2532     2536       +4     
===========================================
+ Hits          6083     6109      +26     
+ Misses        7210     7181      -29     
- Partials       707      717      +10     
Files Coverage Δ
.../wire/android/ui/calling/SharedCallingViewModel.kt 63.57% <60.00%> (-1.26%) ⬇️
...android/ui/calling/ongoing/OngoingCallViewModel.kt 70.00% <66.66%> (-1.43%) ⬇️

... and 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17f1c2a...ceb5b02. Read the comment docs.

Copy link
Contributor

APKs built during tests are available here. Scroll down to Artifacts!

@AndroidBob
Copy link
Collaborator

Build 3667 succeeded.

The build produced the following APK's:

Copy link
Member

@MohamadJaara MohamadJaara left a comment

Choose a reason for hiding this comment

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

Please remember to cherry-pick this change into RC

@ohassine ohassine enabled auto-merge March 25, 2024 09:06
@ohassine ohassine added this pull request to the merge queue Mar 25, 2024
Copy link
Contributor

APKs built during tests are available here. Scroll down to Artifacts!

Merged via the queue into develop with commit 0758737 Mar 25, 2024
14 of 15 checks passed
@ohassine ohassine deleted the calling_video_not_streamed branch March 25, 2024 10:14
ohassine added a commit that referenced this pull request Mar 25, 2024
@AndroidBob
Copy link
Collaborator

Build 3719 succeeded.

The build produced the following APK's:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants