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

libservo: Handle GL video decoding setup internally #31209

Merged
merged 1 commit into from Feb 3, 2024

Conversation

mrobinson
Copy link
Member

Instead of making the client set up GL video decoding, have this done
inside libservo. The details necessary for decoding are fetched via
Surfman now. This also removes the setup for the context on Android --
which has no GStreamer support now anyway. In the future, this could be
enabled, but should likely be done using Surfman, instead of passing on
all these details manually.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes do not require tests because they should not change behavior.

@wusyong
Copy link
Contributor

wusyong commented Jan 30, 2024

I really love this change which simplifies the WindowMethod trait!
I want to take this chance to document media prefs as well. But I don't fully understand their usage.
There are currently two configs in prefs for media:

  • media.glvideo.enable
  • media.testing.enable

Do you know what exactly they are used for?

@jdm
Copy link
Member

jdm commented Jan 30, 2024

media.testing.enabled is only used to expose a non-standard event handler for verifying behavior of media elements during tests:

[Pref="media.testing.enabled"]

@jdm
Copy link
Member

jdm commented Jan 30, 2024

If media.glvideo.enabled is true, the media code will attempt to use hardware acceleration for video playback:

if !pref!(media.glvideo.enabled) {

@@ -67,6 +67,8 @@ impl App {

// Implements window methods, used by compositor.
let window = if opts::get().headless {
// GL video rendering is not supported on headless windows.
set_pref!(media.glvideo.enabled, false);
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean currently we have GL video rendering enabled for headless windows?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, this is disabled, but only implicitly because the previous WindowMethod trait method implementations for headless windows did not return information useful for GL rendering.

@mrobinson
Copy link
Member Author

Thanks for the review!

@mrobinson mrobinson added this pull request to the merge queue Feb 2, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 2, 2024
@mrobinson mrobinson force-pushed the do-not-expose-media-gl-player branch from b02840d to 4275d63 Compare February 3, 2024 13:33
Instead of making the client set up GL video decoding, have this done
inside libservo. The details necessary for decoding are fetched via
Surfman now. This also removes the setup for the context on Android --
which has no GStreamer support now anyway. In the future, this could be
enabled, but should likely be done using Surfman, instead of passing on
all these details manually.
@mrobinson mrobinson force-pushed the do-not-expose-media-gl-player branch from 4275d63 to 4b3ea30 Compare February 3, 2024 14:07
@mrobinson mrobinson added this pull request to the merge queue Feb 3, 2024
Merged via the queue into servo:main with commit d7d0451 Feb 3, 2024
9 checks passed
@mrobinson mrobinson deleted the do-not-expose-media-gl-player branch February 3, 2024 15:41
Taym95 pushed a commit to Taym95/servo that referenced this pull request Feb 11, 2024
Instead of making the client set up GL video decoding, have this done
inside libservo. The details necessary for decoding are fetched via
Surfman now. This also removes the setup for the context on Android --
which has no GStreamer support now anyway. In the future, this could be
enabled, but should likely be done using Surfman, instead of passing on
all these details manually.
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.

None yet

4 participants