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

Editor client: Pause/Resume video on tab visibility change #1261

Merged
merged 1 commit into from
Apr 4, 2022

Conversation

kevin-legion
Copy link

@kevin-legion kevin-legion commented Mar 25, 2022

Closes #1250

Actually seems to fix the bug, but I'm really not confident about the fix itself 😕

@kevin-legion kevin-legion changed the title Editor client: Pause/Resume video on tab visibility change Editor client: Pause/Resume video on tab visibility change [wip] Mar 25, 2022
@ndubois-legion ndubois-legion self-assigned this Mar 28, 2022
@kevin-legion kevin-legion force-pushed the editor-client-resume-video-tab-change branch from c12432c to f310015 Compare March 28, 2022 22:43
@@ -145,6 +145,11 @@ impl SizeDependentResources {
}
}

pub enum RenderSurfacePresentingStatus {
Presenting,
Paused,
Copy link
Author

Choose a reason for hiding this comment

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

Made an enum instead of a bool just so that we add more status when needed. If you think that's not needed I can revert that 👍

RenderSurfacePresentingStatus::Paused
) {
return;
}
Copy link
Author

Choose a reason for hiding this comment

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

This really feels like a small hack. Is there any other way to prevent a render surface from "presenting" @jelmansouri @vduboisdendien-legion ?

Choose a reason for hiding this comment

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

What are you trying to do?

Copy link
Author

Choose a reason for hiding this comment

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

Basically prevent the presenters' "present" method from being called when the RenderSurface is paused 🙂

After discussing with @jelmansouri @smartel-legion @R3DP1XL this might be necessary or not in the future...

@@ -1,3 +1,5 @@
#![allow(clippy::use_self)]
Copy link
Author

Choose a reason for hiding this comment

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

Multiple open issues related to the use_self false positive bug (this one for instance)) but none actually fixed it. Plus, using #[allow(clippy::use_self)] above the guilty line will not shut the warning

@@ -104,14 +128,23 @@ pub(crate) enum Input {
KeyboardInput(KeyboardInput),
}

#[derive(Debug, Deserialize)]
#[serde(tag = "event")]
pub(crate) enum ControlEventInfo {
Copy link
Author

Choose a reason for hiding this comment

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

The line in question, not sure why it triggers a use_self warning in Clippy

@kevin-legion kevin-legion changed the title Editor client: Pause/Resume video on tab visibility change [wip] Editor client: Pause/Resume video on tab visibility change Mar 29, 2022
@kevin-legion kevin-legion changed the title Editor client: Pause/Resume video on tab visibility change Editor client: Pause/Resume video on tab visibility change [Do not merge] Mar 29, 2022
@kevin-legion kevin-legion requested review from smartel-legion and removed request for vduboisdendien-legion March 29, 2022 16:26
@kevin-legion
Copy link
Author

Waiting for some improvements by @smartel-legion , this pr might become obsolete or not, we'll see when the refactor window/render surface/presenter is more advanced 👍

@kevin-legion kevin-legion force-pushed the editor-client-resume-video-tab-change branch from f310015 to 8412db0 Compare April 4, 2022 14:42
@kevin-legion kevin-legion changed the title Editor client: Pause/Resume video on tab visibility change [Do not merge] Editor client: Pause/Resume video on tab visibility change Apr 4, 2022
@kevin-legion
Copy link
Author

Probably a dirty workaround that we'll get rid of as soon as we switch from data channels to media streaming 👍

@kevin-legion kevin-legion merged commit 56ed99a into main Apr 4, 2022
@kevin-legion kevin-legion deleted the editor-client-resume-video-tab-change branch April 4, 2022 15:43
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.

[editor client] Viewport is stuck when switching tabs [blocked]
3 participants