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

feat(web) ✨ new config to start with tileView #14504

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

HannesOberreiter
Copy link
Contributor

Currently there is no config option to set tile view layout as default starting point. This PR introduces a new config tileView.force which will set tileView if enabled. It is called force as it will also be forced when a video is shared, 1:1 meeting, jibri etc.

The user can still manually toggle tileView if the button is available or if tileView is disabled in the config it will be disabled, as this check comes before the force check.

We actually need this feature, as we want jibri to capture the video in tile view.

Other issues related to this setting:

Contribution Agreement was signed as company, Certible GmbH.

(Note: One commit includes a small typo and type fix as it was in the same file.)

Cheers
Hannes

@jitsi-jenkins
Copy link

Hi, thanks for your contribution!
If you haven't already done so, could you please make sure you sign our CLA (https://jitsi.org/icla for individuals and https://jitsi.org/ccla for corporations)? We would unfortunately be unable to merge your patch unless we have that piece :(.

@saghul
Copy link
Member

saghul commented Mar 21, 2024

Thanks for the PR!

I think this can create some weird situations, like pinning a user actually being ignored, which feels wrong.

Is using follow-me not an option for you?

Perhaps we could think of this as an option specific for the recorder?

@HannesOberreiter
Copy link
Contributor Author

HannesOberreiter commented Mar 21, 2024

Hi @saghul,

thanks for the quick response.

Yeah I though also a lot about some edge cases, thats why I called it force and not default. As for example YouTube Video sharing would also be questionable.

I first started with some exception like breaking up sharing video and youtube sharing and pinned users etc. but then I questioned myself what to include in the exception and what not. Therefore, I choose to simply force it over everything else, which is no arbitrary decision.

As for our own use case, I was anyway thinking about looking into the jibri repo to expose the config flags in the url to an ENV variable (but currently don't understand how). I would still need the option in jitsi, as making this a jibri only option for the recorder and not for jitsi as a config also feels wrong as it could have other use cases.

(Edit: As for our own use case follow-me would not work as screen sharing etc will break it (#12576) and we want a recording of all users and screens shared at any time)

Cheers
Hannes

@saghul
Copy link
Member

saghul commented Mar 21, 2024

I see. Let's break it down:

    const shouldDisplayNormalMode = Boolean(

        // Reasons for normal mode:

        // Editing etherpad
        state['features/etherpad']?.editing

        // We pinned a participant
        || getPinnedParticipant(state)

        // It's a 1-on-1 meeting
        || participantCount < 3

        // There is a shared YouTube video in the meeting
        || isVideoPlaying(state)

        // We want jibri to use stage view by default
        || iAmRecorder
    );

So the reasons for preferring stage view are:

  • Document sharing: this doesn't work on stage view at all, so I assume it needs to be obeyed.
  • A participant is pinned: this one is interesting, because some actions like screen-sharing auto-pin. Perhaps we should have a setting for that too.
  • It's a 1-1 meeting (participant count < 3): this could be made configurable.
  • Is there a shared video or YouTube playing? We don't redner those in tile view (just the thumbnail, but no video), so I guess it should be obeyed, or made configurable too.
  • The participant is a recorder. This could be made configurable.

I feel like we need a few more options here, a single one won't cut it.

@HannesOberreiter
Copy link
Contributor Author

Here is my first brainstorm with more fine options. Is only a quick write up. As for the etherpad, I did not understand this setting anyway. As for me it did work in tile and stage view (only tested on browser).

     // User setting for stage or tile layout will overwrite all cases were an automatic switch to another form of view is attempted, you can disable this behaviour with this setting
    overwriteUserPreferenceForVideoLayout: false,

    // Whether screen sharing should lead to an automatic pin of the shared video.
    automaticallyPinSharedScreen: true,

    // Jibri recording layout, options are "participant", "stage" and "tile" which will force this layout over all else. If you set "participant" the jibri client will follow the same rules as defined in the config. The client is detected by the iAmRecorder config, which is set by jibri.
    jibriVideoLayout: 'stage',

    // Stage view related config options, in which automatic layout switching to stage view is enabled.
    stageView: {
        // Switch to stage view when a video is shared
        sharedVideos: true,

        // Switch to stage view when a participants shares a video
        sharedScreen: true,

        // Switch to stage view when threshold is reached or lower, default when two or less participants
        participantThreshold: 2,

        // Switch to stage view when a participant is pinned
        pinnedUser: true,
    },

    // Tile view related config options.
    tileView: {
        // Default view is tile view if no other view mode is automatically selected, eg. pinned user.
        default: false,

        // Whether tileview should be disabled.
        disabled: false,

        // The optimal number of tiles that are going to be shown in tile view. Depending on the screen size it may
        // not be possible to show the exact number of participants specified here.
        numberOfVisibleTiles: 25,
    },

Cheers
Hannes

Copy link

This PR 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.

@github-actions github-actions bot added the stale label Jun 20, 2024
@HannesOberreiter
Copy link
Contributor Author

WIP

@github-actions github-actions bot removed the stale label Jun 21, 2024
Copy link

This PR 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.

@github-actions github-actions bot added the stale label Sep 19, 2024
@HannesOberreiter
Copy link
Contributor Author

WIP

@github-actions github-actions bot removed the stale label Sep 20, 2024
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.

3 participants