Skip to content
This repository has been archived by the owner on Nov 1, 2021. It is now read-only.

Simpler screencopy #2615

Merged
merged 2 commits into from
Sep 10, 2021
Merged

Simpler screencopy #2615

merged 2 commits into from
Sep 10, 2021

Conversation

any1
Copy link
Contributor

@any1 any1 commented Jan 7, 2021

This simplifies screencopy by copying pixels to shm in the commit callback rather than the pre-commit callback, so both dmabuf and shm buffers are handled in commit.

Because commit always happens after pre-commit, damage accumulation is also simplified because we no longer need to ascertain whether the damage callback or the frame-copy callback happened first.

Supersedes #2591

@emersion
Copy link
Member

emersion commented Jan 7, 2021

I haven't looked in detail yet, but

renderer: Add buffer argument to read_pixels

I think we can just bind_buffer, read_pixels, then bind_buffer(NULL) to avoid this change?

@any1
Copy link
Contributor Author

any1 commented Jan 7, 2021

I haven't looked in detail yet, but

renderer: Add buffer argument to read_pixels

I think we can just bind_buffer, read_pixels, then bind_buffer(NULL) to avoid this change?

Hmm, yeah, you're most likely right. I wasn't thinking in terms of the "global picture"; just what would be needed to call read_pixels from pretty much anywhere at any time. This change should make it possible to skip scheduling a frame and waiting for the commit event when the user doesn't want to wait for damage or when there is already damage. But maybe that's not really a useful "optimisation".

@any1
Copy link
Contributor Author

any1 commented Jan 7, 2021

I tried using bind_buffer, but it failed on assert(wlr_egl_is_current(renderer->egl)). If we want to use these functions, we need to (at least) change them so that they set an egl context and they should probably also save/restore the old egl context.

@emersion
Copy link
Member

emersion commented Jan 8, 2021

we need to (at least) change them so that they set an egl context

I think this would be a good change, and it's also needed to remove the EGL dependency from the backends. Would you mind opening a PR for this?

save/restore the old egl context

I think saving/restoring makes sense when we need to temporarily set the EGL context in a single function (like blit_dmabuf). However I'm a little worried about saving in one function call and restoring in another one. This could cause issues if the saved EGL objects become invalid, for instance.

@any1
Copy link
Contributor Author

any1 commented Jan 8, 2021

we need to (at least) change them so that they set an egl context

I think this would be a good change, and it's also needed to remove the EGL dependency from the backends. Would you mind opening a PR for this?

Yeah, I'll give it a go

emersion added a commit to emersion/wlroots that referenced this pull request Jan 14, 2021
Instead of requiring callers to manually make the EGL context current
before binding a buffer and unsetting it after unbinding a buffer, do
it inside wlr_renderer_bind_buffer.

This hides renderer-specific implementation details inside the
wlr_renderer interface. Non-GLES2 renderers may not use EGL.
This removes almost all EGL dependencies from the backends, the only
remaining one is wlr_renderer_autocreate args.

References: swaywm#2618
References: swaywm#2615 (comment)
emersion added a commit to emersion/wlroots that referenced this pull request Jan 16, 2021
Instead of requiring callers to manually make the EGL context current
before binding a buffer and unsetting it after unbinding a buffer, do
it inside wlr_renderer_bind_buffer.

This hides renderer-specific implementation details inside the
wlr_renderer interface. Non-GLES2 renderers may not use EGL.
This removes almost all EGL dependencies from the backends, the only
remaining one is wlr_renderer_autocreate args.

References: swaywm#2618
References: swaywm#2615 (comment)
emersion added a commit to emersion/wlroots that referenced this pull request Jan 16, 2021
Instead of requiring callers to manually make the EGL context current
before binding a buffer and unsetting it after unbinding a buffer, do
it inside wlr_renderer_bind_buffer.

This hides renderer-specific implementation details inside the
wlr_renderer interface. Non-GLES2 renderers may not use EGL.
This removes all EGL dependencies from the backends.

References: swaywm#2618
References: swaywm#2615 (comment)
emersion added a commit that referenced this pull request Jan 16, 2021
Instead of requiring callers to manually make the EGL context current
before binding a buffer and unsetting it after unbinding a buffer, do
it inside wlr_renderer_bind_buffer.

This hides renderer-specific implementation details inside the
wlr_renderer interface. Non-GLES2 renderers may not use EGL.
This removes all EGL dependencies from the backends.

References: #2618
References: #2615 (comment)
@any1
Copy link
Contributor Author

any1 commented Jan 17, 2021

We are putting this on hold until #2505 is merged. Specifically 47f59dd ("output: make attach_render and rollback_render optional").

@emersion emersion marked this pull request as draft January 17, 2021 14:04
@emersion
Copy link
Member

emersion commented Jun 8, 2021

#2505 has been merged, I think now would be a good time to revisit this. #2955 adds wlr_output.front_buffer.

@emersion
Copy link
Member

emersion commented Jun 8, 2021

Ah, still one missing piece is the DRM backend, which still uses the old infrastructure. This will be fixed in #2903.

@emersion
Copy link
Member

@any1, would you be willing to continue work on this?

@any1
Copy link
Contributor Author

any1 commented Jul 27, 2021

@any1, would you be willing to continue work on this?

Yeah, sure.

@emersion
Copy link
Member

We should now be able to listen to output commits exclusively, check for event->committed & WLR_OUTPUT_STATE_BUFFER, then grab output->front_buffer.

@any1
Copy link
Contributor Author

any1 commented Aug 14, 2021

I believe I have adapted my previous changes to master now. It does seem to work with wayvnc on both the wayland and headless backends at least.

types/wlr_screencopy_v1.c Outdated Show resolved Hide resolved
types/wlr_screencopy_v1.c Outdated Show resolved Hide resolved
types/wlr_screencopy_v1.c Outdated Show resolved Hide resolved
Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

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

Overall looks pretty good!

@any1 any1 marked this pull request as ready for review September 5, 2021 12:30
@any1 any1 requested a review from emersion September 5, 2021 12:30
@emersion
Copy link
Member

emersion commented Sep 8, 2021

Ready to merge with that one last cleanup comment fixed

Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

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

Thanks!

@emersion emersion merged commit 105fdec into swaywm:master Sep 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants