-
Notifications
You must be signed in to change notification settings - Fork 7.6k
feat(pip): Add Picture-in-Picture support for Electron #16704
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
base: master
Are you sure you want to change the base?
Conversation
saghul
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some early feedback. Nothing stands out as a big problem, good work!
| if (!defaultIconRef.current) { | ||
| let svgText = IconUserSVG; | ||
|
|
||
| if (!svgText.includes('fill=')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this all of this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which exactly?
We are generally retrieve the default icon svg content in order to draw it in the canvas (by rendering it and then using drawImage in the canvas).
If you mean the part with the fill, currently the svg doesn't have any color, so we specify it.
| } else if (videoTrack?.jitsiTrack) { | ||
| // Attach real video track. | ||
| videoTrack.jitsiTrack.attach(videoElement) | ||
| .then(() => videoElement.play()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the play necessary here? When would it not play?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this one and also the previous .play()
| * @param {IReduxState} state - Redux state. | ||
| * @returns {boolean} Whether audio should be shown as muted. | ||
| */ | ||
| export function isAudioMutedForPiP(state: IReduxState): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than duplicate it, can we factor it out? I worry of both going out of sync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well I've tried to mimic the audio button state. Basically the core logic is in isLocalTrackMuted(state['features/base/tracks'], MEDIA_TYPE.AUDIO) so they kind of share the implementation trough this function already. The additional part is about the GUM check but this is used only in the web toolbar button.
Do you want me to include somehow the GUM check in a function and use it here for the PiP and also for the toolbar button? The downside is that the native button don't need the GUM check so it won't use the function or I can also add react-native check.
The whole thing sounds like over-engineering but I'm not against it. Let me know if this is what you have in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need the gUM check?
react/features/pip/functions.ts
Outdated
| ctx.fill(); | ||
|
|
||
| ctx.fillStyle = '#FFFFFF'; | ||
| ctx.font = 'bold 80px -apple-system, BlinkMacSystemFont, "Segoe UI", sans-serif'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probnably don't want to set the font here and let the default one?
react/features/pip/functions.ts
Outdated
| canvasWidth: number | ||
| ) { | ||
| ctx.fillStyle = '#FFFFFF'; | ||
| ctx.font = '24px -apple-system, BlinkMacSystemFont, "Segoe UI", sans-serif'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
react/features/pip/functions.ts
Outdated
| const textY = centerY + avatarRadius + spacing; | ||
|
|
||
| // Clear and fill background. | ||
| ctx.fillStyle = '#474747'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a constant?
react/features/pip/functions.ts
Outdated
| // This bypasses the transient activation requirement by executing | ||
| // requestPictureInPicture with userGesture: true in the main process. | ||
| if (browser.isElectron()) { | ||
| exposeRequestPiPForElectron(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels weird to be exposing the function on every click. We should do it only once, likely elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well inside exposeRequestPiPForElectron I kind of check if we set it. Although on second thought probably this is even less efficient than just setting it.
I agree with you here. Do you have any suggestions where to put it? Maybe here in the global scope of the file instead of in the function? WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've put it in the subscriber. When PiP is enabled we attach it and when it is disabled we delete it. It will normally happens once but in theory you can update the config to disable/enable PiP...
7b86902 to
15a7ba0
Compare
166c2d4 to
f3b8d24
Compare
filmstrip/actions.web was imported in TileView native component. filmstrip/actions.web was imported in config middleware.any.
Implements Picture-in-Picture functionality for the Electron wrapper to maintain video engagement when users are not actively focused on the conference window. This feature addresses the need to keep users visually connected to the conference even when multitasking. Key features: - Automatic PiP mode activation and deactivation based on user interaction - Displays large video participant's stream or renders their avatar on canvas when video unavailable - Provides audio/video mute controls via MediaSession API directly in PiP window - Adds API events (_pip-requested) for Electron wrapper integration Implementation includes new pip feature module with Redux architecture, canvas-based avatar rendering with custom backgrounds support, and integration with existing mute/unmute logic. Depends on jitsi-meet-electron-sdk#479 for proper user gesture handling in Electron.
f3b8d24 to
815ae7d
Compare
| * @private | ||
| * @returns {*} The return value of {@code next(action)}. | ||
| */ | ||
| function _setDynamicBrandingData({ dispatch }: IStore, next: Function, action: AnyAction) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this already handled in the shared middleware?
| @@ -0,0 +1,251 @@ | |||
| import { AnyAction } from 'redux'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand the motivation behind this change, since there is no pip code involved.
| export function toggleAudioFromPiP() { | ||
| return (dispatch: IStore['dispatch'], getState: IStore['getState']) => { | ||
| const state = getState(); | ||
| const audioMuted = isAudioMutedForPiP(state); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WHy do we need specific "for pip" functions?
| document.exitPictureInPicture().catch((err: Error) => { | ||
| logger.error(`Error while exiting PiP: ${err.message}`); | ||
| }); | ||
| logger.log('Exited Picture-in-Picture mode'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should log this in a then() otherwise it will be printed always.
| * | ||
| * @param {IReduxState} state - Redux state. | ||
| * @param {IParticipant | undefined} participant - Participant to get track for. | ||
| * @returns {any} The video track or undefined. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have types for track don't we?
| * @param {IReduxState} state - Redux state. | ||
| * @returns {boolean} Whether audio should be shown as muted. | ||
| */ | ||
| export function isAudioMutedForPiP(state: IReduxState): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need the gUM check?
|
|
||
| // Since this is internal event we don't need to emit it to the consumer of the API. | ||
| return true; | ||
| case 'config-overwrite': { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't we need the same logic when a user changes the setting? When we add it, that is. In that case, doesn't it make more sense to emit an event when the PiP setting changes? Overriding the config should effectively change the setting.
| * It's bundled into external_api.min.js and we want to keep that bundle slim. | ||
| * Only import lightweight modules here. | ||
| */ | ||
| import BrowserDetection from '@jitsi/js-utils/browser-detection/BrowserDetection'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
THis will pull the entire UA parser library, won't it? Can't we just inline the Electron check?
Summary
Implements Picture-in-Picture functionality for the Electron wrapper to maintain video engagement when users are not actively focused on the conference window. The feature automatically manages PiP mode based on user interaction, displays either the large video participant's video stream or their avatar, and provides MediaSession API integration for audio/video mute controls directly from the PiP window.
Key Features
_pip-requestedAPI event for proper user gesture handling in Electron wrapperImplementation Details
New Feature Module (
react/features/pip/)PiPVideoElementcomponent manages hidden video element for PiPAPI Integration
notifyPictureInPictureRequested()method to internal API_pip-requestedevent to external API for Electron communicationVideo Source Management
State Management
Dependencies
Test Plan