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

Transient screen cast stream restoration no longer works #1124

Closed
aiddya opened this issue Oct 4, 2023 · 11 comments · Fixed by #1125
Closed

Transient screen cast stream restoration no longer works #1124

aiddya opened this issue Oct 4, 2023 · 11 comments · Fixed by #1125
Assignees
Labels
Milestone

Comments

@aiddya
Copy link
Contributor

aiddya commented Oct 4, 2023

Screen cast stream restoration with persist mode as transient no longer works with xdg-desktop-portal 1.18. It works if the persist mode is until explicitly revoked.

Steps to reproduce

  1. Open this WebRTC test page with Chromium on a distro with xdg-desktop-portal 1.18
  2. Click on "screen capture" and switch to "Window" or "Entire screen" tabs.
  3. Select a source on portal dialog and wait for the thumbnail to appear
  4. Click on "Share"

Expected behavior

Shared screen/window is displayed.

Actual behavior

The portal dialog appears again.

DBus logs show that the application is passing restore token correctly and the portal implementation is returning restore data. The bug is either in storing the restore token along with restore data or while retrieving it.

cc @jadahl

@aiddya
Copy link
Contributor Author

aiddya commented Oct 4, 2023

It appears that the bug is a result of deleting all transient restore tokens once the screen cast session is closed. However, the documentation for transient persist mode says that "Permissions persist as long as the application is running".

@grulja
Copy link
Contributor

grulja commented Oct 4, 2023

I can reproduce. I expect this is related to #1004.

@GeorgesStavracas GeorgesStavracas self-assigned this Oct 4, 2023
@GeorgesStavracas GeorgesStavracas added this to the 1.18 milestone Oct 4, 2023
@GeorgesStavracas
Copy link
Member

This is not reproducible on Firefox. Are you sure this is not a Chromium issue?

@GeorgesStavracas
Copy link
Member

Now that I test this, I think I've seen that on Chromium for a long time... certainly before #1004.

@grulja
Copy link
Contributor

grulja commented Oct 4, 2023

This is not reproducible on Firefox. Are you sure this is not a Chromium issue?

Firefox works differently and doesn't rely on stream restoration. I've seen this issue before with Google Meet, which might be a bug in Chromium (on my TODO list), but for example the WebRTC test page has been working all the time. There was no Chromium (WebRTC) change to screen sharing for a while.

@GeorgesStavracas GeorgesStavracas added the needs diagnosis Root cause of the issue needs to be diagnosed label Oct 4, 2023
@aiddya
Copy link
Contributor Author

aiddya commented Oct 4, 2023

The flow in Chromium is that it starts a screen cast session to show thumbnails on the screen/window picker and it restores the stream in a new session to show the stream on the web page. Firefox does not show thumbnails, so it doesn't use stream restoration.

The regression was introduced by #1004, specifically the internal_closed_cb method which was not present before the refactor. It deletes all restore tokens created in a session when a session is closed. Since Chromium uses one session to capture thumbnails and another session to show the stream on the page, the restore tokens are being deleted at the end of the first session.

This is contrary to the documentation of transient persist mode, which says that "Permissions persist as long as the application is running".

@GeorgesStavracas
Copy link
Member

Thanks @aiddya, I think your diagnostic is correct. Patch incoming.

@GeorgesStavracas GeorgesStavracas added regression and removed need more info needs diagnosis Root cause of the issue needs to be diagnosed labels Oct 4, 2023
GeorgesStavracas added a commit to GeorgesStavracas/xdg-desktop-portal that referenced this issue Oct 4, 2023
This is what the documented behaviour mentions - if the peer dies,
transient permissions are gone.

This doesn't yet work since we're removing transient permissions
when the session is closed.

Related: flatpak#1124
GeorgesStavracas added a commit to GeorgesStavracas/xdg-desktop-portal that referenced this issue Oct 4, 2023
This is the counterpart to removing transient permissions when
the peer dies - not remove them when the session is closed.

Closes: flatpak#1124
@GeorgesStavracas
Copy link
Member

Can you please test #1125?

@aiddya
Copy link
Contributor Author

aiddya commented Oct 4, 2023

I can confirm that it fixes the issue in Chromium and Electron.

P.S.: It took me quite a while to understand that I had to provide datadir as a meson option for xdg-desktop-portal to pick up the portal configuration. It'd be helpful to document that in meson_options.txt and in CONTRIBUTING.md as something to watch out for.

@GeorgesStavracas
Copy link
Member

It'd be helpful to document that in meson_options.txt and in CONTRIBUTING.md as something to watch out for.

Patches welcomed :)

@aiddya
Copy link
Contributor Author

aiddya commented Oct 5, 2023

Patches welcomed :)

I may not have all the information write such documentation, but I can give it a shot.

@grulja I think the Google Meet issue that you're seeing might be fixed with a patch like this on Chromium: electron/electron#40098

GeorgesStavracas added a commit to GeorgesStavracas/xdg-desktop-portal that referenced this issue Oct 5, 2023
This is what the documented behaviour mentions - if the peer dies,
transient permissions are gone.

This doesn't yet work since we're removing transient permissions
when the session is closed.

Related: flatpak#1124
GeorgesStavracas added a commit to GeorgesStavracas/xdg-desktop-portal that referenced this issue Oct 5, 2023
This is the counterpart to removing transient permissions when
the peer dies - not remove them when the session is closed.

Closes: flatpak#1124
GeorgesStavracas added a commit to GeorgesStavracas/xdg-desktop-portal that referenced this issue Oct 5, 2023
This is what the documented behaviour mentions - if the peer dies,
transient permissions are gone.

This doesn't yet work since we're removing transient permissions
when the session is closed.

Related: flatpak#1124
GeorgesStavracas added a commit to GeorgesStavracas/xdg-desktop-portal that referenced this issue Oct 5, 2023
This is the counterpart to removing transient permissions when
the peer dies - not remove them when the session is closed.

Closes: flatpak#1124
github-merge-queue bot pushed a commit that referenced this issue Oct 5, 2023
This is what the documented behaviour mentions - if the peer dies,
transient permissions are gone.

This doesn't yet work since we're removing transient permissions
when the session is closed.

Related: #1124
github-merge-queue bot pushed a commit that referenced this issue Oct 5, 2023
This is the counterpart to removing transient permissions when
the peer dies - not remove them when the session is closed.

Closes: #1124
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Triaged
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants