VIDEO-4517: Add Client Track Switch Off Control Support #156

ceaglest · 2021-04-28T15:16:18Z

This PR adds support for Client Track Switch Off Control to the iOS Video App.

Client track switch off control is unset by default (resolves to auto)
Added feature to settings UI / settings store
The app does not implement any specific manual controls. Selecting manual is effectively the legacy behavior without a maxTracks limit
Removed the deprecated maxTracks property.
Removed extra UICollectionView padding on the leading edge.
CollectionView logic was updated to add and remove renderers at cell display time rather than cell dequeue time (thanks @timrozum)

Note: While testing this feature I ran into a background CPU usage crash which was resolved in #155.

Contributing to Twilio

All third-party contributors acknowledge that any contributions they provide will be made under the same open-source license that the open-source project is provided under.

I acknowledge that all my contributions will be made under the project's license.

* Subscribed track switch off is enabled by default * The app does not implement any specific manual controls, this is effectively the legacy behavior without maxTracks * Comment out usage of deprecated maxTracks property for now * No modifications to collection view cells yet * [skip ci]

VideoApp/VideoApp/Video/Room/ConnectOptionsFactory.swift

timrozum

This looks good so far.

I wouldn't really be concerned about tests at this point because most of this is settings which we don't have tests for and I expect will be replaced by InAppSettingsKit before long.

When the time comes we could consider unit tests for ConnectOptionsFactory but I can take care of that.

UI tests are probably not practical right now for this feature.

Happy to help if the cell reuse stuff needs work.

VideoApp/VideoApp/Video/Room/ConnectOptionsFactory.swift

VideoApp/VideoApp/Stores/AppSettings/SubscribedTrackSwitchOffMode.swift

ceaglest · 2021-04-29T14:01:32Z

VideoApp/VideoApp/Storyboards/Base.lproj/Main.storyboard

@@ -528,7 +528,7 @@
                                        </constraints>
                                    </view>
                                    <collectionView clipsSubviews="YES" multipleTouchEnabled="YES" contentMode="scaleToFill" showsHorizontalScrollIndicator="NO" showsVerticalScrollIndicator="NO" dataMode="prototypes" translatesAutoresizingMaskIntoConstraints="NO" id="wU6-sZ-Ash">
-                                        <rect key="frame" x="10" y="690" width="404" height="172"/>
+                                        <rect key="frame" x="0.0" y="690" width="414" height="172"/>


The UICollectionView had a gap of 10 points on the leading edge but was flush to the trailing edge. I think this was just an unintended bug (it's not obvious until you have more than a screen full of cells).

I think I set this gap intentionally but not sure what desired UX really is to be honest. I'm good with this adjustment.

Probably how it should really work someday is the collection view has no gaps on either side and instead we use spacing inside of the collection view content at start and end. This way it looks good at start/end position and when scrolling (cells are not clipped before the edge of the display).

But I think the team wants to completely redo UX of this screen someday so best to tackle then.

ceaglest · 2021-04-29T14:11:00Z

Hi @timrozum,

I continued to test this PR yesterday. What I noticed was that UICollectionViewDataSource prefetching is preventing some of the subscribed track switch off optimizations. Specifically, the behavior where cells can be opportunistically dequeued by UIKit before they come onscreen and are not recycled immediately after they go offscreen.

I think the best way to manage rendering in the UICollectionView is to implement the delegate methods for cell display. I was wondering if you would be interested in taking a shot at it?

func collectionView(_ collectionView: UICollectionView, willDisplay cell: UICollectionViewCell, forItemAt indexPath: IndexPath)
func collectionView(_ collectionView: UICollectionView, didEndDisplaying cell: UICollectionViewCell, forItemAt indexPath: IndexPath)

We should make sure that video starts / stops rendering within these methods. To me it is okay to keep the existing dequeue logic (cellForItemAtIndexPath) as an optimization to have a better chance that video will be ready by the time the cell is drawn to screen.

timrozum · 2021-04-29T17:26:01Z

@ceaglest yeah I had looked at didEndDisplaying yesterday and also thought that would be useful for this.

I guess willDisplay may also be necessary if it is possible for a cell to be in use but not visible from collection view perspective. But haven't looked into that.

ceaglest · 2021-04-29T18:00:59Z

I guess willDisplay may also be necessary if it is possible for a cell to be in use but not visible from collection view perspective. But haven't looked into that.

Yes it is needed as well. To replicate, try scrolling a cell just offscreen and then back on. Typically the cell does not go through the reuse queue during this action but your delegate receives a didEndDisplaying followed by a willDisplay.

timrozum · 2021-04-29T18:02:00Z

Thanks great to know.

Podfile

* SubscribedTrackSwitchOffMode -> ClientTrackSwitchOffControl

ceaglest · 2021-05-04T20:13:03Z

I am curious if we would cut a feature branch for media optimizations in advance of the 4.5.0 release? This app should also be updated for VideoContentPreferences, which I think deserves a separate PR.

# Conflicts: # Podfile.lock

ceaglest

Haven't run the changes locally but it looks like they will resolve the flickering issue. Thank you @timrozum. 👍

ceaglest added 2 commits April 27, 2021 21:36

Update Podfile to use release version (does not exist yet). [skip ci]

b0ed470

ceaglest commented Apr 28, 2021

View reviewed changes

VideoApp/VideoApp/Video/Room/ConnectOptionsFactory.swift Outdated Show resolved Hide resolved

timrozum self-requested a review April 28, 2021 17:46

ceaglest changed the title ~~VIDEO-4517: Add subscribed track switch-off to VideoApp.~~ VIDEO-4517: Add Subscribed Track Switch Off Support Apr 28, 2021

timrozum reviewed Apr 28, 2021

View reviewed changes

VideoApp/VideoApp/Video/Room/ConnectOptionsFactory.swift Outdated Show resolved Hide resolved

VideoApp/VideoApp/Stores/AppSettings/SubscribedTrackSwitchOffMode.swift Outdated Show resolved Hide resolved

ceaglest added 2 commits April 28, 2021 20:27

Address feedback on default title. [skip ci]

5815a71

Fix clipped layout of collection view. [skip ci]

07797ff

ceaglest commented Apr 29, 2021

View reviewed changes

ceaglest changed the title ~~VIDEO-4517: Add Subscribed Track Switch Off Support~~ VIDEO-4517: Add Client Track Switch Off Control Support Apr 29, 2021

Remove renderer when cell is not visible

f9ac16f

timrozum reviewed Apr 29, 2021

View reviewed changes

Podfile Show resolved Hide resolved

ceaglest added 4 commits May 3, 2021 18:28

Rename the feature.

d64e3d2

* SubscribedTrackSwitchOffMode -> ClientTrackSwitchOffControl

Remove maxTracks from the app.

5c959d1

Fix text that was missed in feature rename.

17051d6

Use 4.5.0-rc1.

0f5326a

ceaglest marked this pull request as ready for review May 4, 2021 20:08

ceaglest and others added 7 commits May 4, 2021 14:18

Rename SubscribedTrackSwitchOffControl store.

24f6ed7

Respect alpha ordering in project. [skip ci]

35c4b70

Update pods

870627e

Merge branch 'master' into task/video-4517-subscribed-track-switch-off

b11222a

# Conflicts: # Podfile.lock

Remove renderer when video track changes

7d647b7

Merge branch 'master' into task/video-4517-subscribed-track-switch-off

29019ed

Fix podfile

8e939c1

timrozum changed the base branch from master to feature/media-optimization May 28, 2021 20:14

ceaglest commented May 28, 2021

View reviewed changes

timrozum approved these changes May 28, 2021

View reviewed changes

ceaglest merged commit 581824e into feature/media-optimization May 28, 2021

ceaglest deleted the task/video-4517-subscribed-track-switch-off branch May 28, 2021 20:25

ceaglest mentioned this pull request Aug 27, 2021

Release 4.5.0, merge feature/media-optimizations #169

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VIDEO-4517: Add Client Track Switch Off Control Support #156

VIDEO-4517: Add Client Track Switch Off Control Support #156

ceaglest commented Apr 28, 2021 •

edited

Loading

timrozum left a comment

ceaglest Apr 29, 2021 •

edited

Loading

timrozum May 28, 2021

ceaglest commented Apr 29, 2021

timrozum commented Apr 29, 2021

ceaglest commented Apr 29, 2021

timrozum commented Apr 29, 2021

ceaglest commented May 4, 2021

ceaglest left a comment

VIDEO-4517: Add Client Track Switch Off Control Support #156

VIDEO-4517: Add Client Track Switch Off Control Support #156

Conversation

ceaglest commented Apr 28, 2021 • edited Loading

timrozum left a comment

Choose a reason for hiding this comment

ceaglest Apr 29, 2021 • edited Loading

Choose a reason for hiding this comment

timrozum May 28, 2021

Choose a reason for hiding this comment

ceaglest commented Apr 29, 2021

timrozum commented Apr 29, 2021

ceaglest commented Apr 29, 2021

timrozum commented Apr 29, 2021

ceaglest commented May 4, 2021

ceaglest left a comment

Choose a reason for hiding this comment

ceaglest commented Apr 28, 2021 •

edited

Loading

ceaglest Apr 29, 2021 •

edited

Loading