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

Add surface interface stubs in compositor example #41

Merged
merged 3 commits into from
Aug 4, 2017

Conversation

acrisci
Copy link

@acrisci acrisci commented Aug 2, 2017

Add the wayland surface interface to the example compositor.

Implement the create_surface method to create a new wlr surface from the
wayland surface and add the interface.

@acrisci
Copy link
Author

acrisci commented Aug 2, 2017

I had to add a wlr_renderer to the wl_compositor struct to create the wlr_surface in the create_surface method but I don't know if that was your intent.

I think the wlr_surface needs to be saved in a list somewhere and then accessed in the handle_output_frame method for rendering but I'm not sure exactly what the best way to do that is.

I also have a semi-working implementation locally of attaching an shm buffer to the surface where I can at least access the buffer contents and the gl calls don't fail but I have yet to successfully paint anything on the screen. I'm working on that now.

My test client is located in my notes repository here.

@ddevault
Copy link
Contributor

ddevault commented Aug 2, 2017

  • I recommend weston-simple-shm for testing.
  • It makes sense that wl_compositor would need a wlr_renderer to work
  • Yes the surface needs to be stored in a list and rendered each frame

@ddevault
Copy link
Contributor

ddevault commented Aug 2, 2017

There are some style issues here, but GitHub fucked up my web browser so I can't comment inline. Put { on the same line and wrap to 80 columns.

@acrisci acrisci force-pushed the feature/surface-interface-stub branch 2 times, most recently from f1268c6 to 9ff29fc Compare August 3, 2017 13:52
@acrisci acrisci changed the title [WIP] Add surface interface stubs in compositor example Add surface interface stubs in compositor example Aug 3, 2017
@acrisci
Copy link
Author

acrisci commented Aug 3, 2017

Ok I cleaned it up a bit. I think this is a good starting point for the surface implementation.

Next I will add the surfaces to a list that can be accessed when they need to be rendered and implement attaching the shm buffer.

surface_commit,
surface_set_buffer_transform,
surface_set_buffer_scale,
surface_damage_buffer
Copy link
Contributor

Choose a reason for hiding this comment

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

Double indented?

@acrisci acrisci force-pushed the feature/surface-interface-stub branch from 9ff29fc to 3d01e56 Compare August 3, 2017 14:43
@ddevault
Copy link
Contributor

ddevault commented Aug 3, 2017

LGTM

Add the wayland surface interface to the example compositor.

Implement the create_surface method to create a new wlr surface from the
wayland surface and add the interface.
@acrisci acrisci force-pushed the feature/surface-interface-stub branch from 3d01e56 to 555914a Compare August 3, 2017 14:46
@ddevault
Copy link
Contributor

ddevault commented Aug 3, 2017

Are you expecting this to be merged before you attach buffers?

Also, side note, in the future wlr_surface is going to need to handle damage, likely in cooperation with wlr_renderer. A surface is going to have to keep track of what parts of it are damaged and bubble it up to the user somehow, and partial rerendering is something to be considered.

@acrisci
Copy link
Author

acrisci commented Aug 3, 2017

Are you expecting this to be merged before you attach buffers?

However you want to work is fine. We can do one big pull request or I can just request comments on every commit. I have about a month off work and this is my main project right now so I should be moving fairly quickly.

Also, side note, in the future wlr_surface is going to need to handle damage

I'll look at the weston and wlc implementation and use my best judgement on how to do this.

@ddevault
Copy link
Contributor

ddevault commented Aug 3, 2017

Let's implement this up to the point of attaching buffers, and do the rest separately. Right now there's not really much going on here, not really worth merging.

@ddevault
Copy link
Contributor

ddevault commented Aug 3, 2017

Also, the usual wisdom is to track damage with pixman. To some degree it will be necessary for compositors to combine this with their own information about where views are being rendered to composite the damaged regions with underlying transparent surfaces and so on.

@acrisci
Copy link
Author

acrisci commented Aug 3, 2017

Ok, I implemented attaching the buffer to the surface. This commit actually makes shit show up on the screen.

@ddevault
Copy link
Contributor

ddevault commented Aug 3, 2017

Pic?

@ddevault
Copy link
Contributor

ddevault commented Aug 3, 2017

LGTM codewise

@acrisci
Copy link
Author

acrisci commented Aug 3, 2017

image

This is made using my example client which attaches a buffer that consists of all green pixels. The weston-simple-shm won't work yet because we don't implement a shell interface.

When you close the window, the server segfaults.

@ddevault
Copy link
Contributor

ddevault commented Aug 3, 2017

Can you address the segfault before this is merged?

@acrisci acrisci force-pushed the feature/surface-interface-stub branch from 47eb3f9 to 1b38485 Compare August 4, 2017 15:01
surface->wlr_surface->format = format;
surface->pixel_format = fmt;

GL_CALL(glActiveTexture(GL_TEXTURE0));
Copy link
Author

Choose a reason for hiding this comment

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

I'm not exactly sure why I need this call to glActiveTexture, but it seems to be necessary. Here is my analysis.

GL_TEXTURE0 is the default active texture when the compositor starts. Upon the first render, the surface is bound with gles2_surface_bind() which changes the active texture with GL_CALL(glActiveTexture(GL_TEXTURE0 + 1));.

So the first surface is attached with active texture 0 and then it is rendered. Then the second texture is attached with active texture 1 which doesn't seem to work and I just get a black window. In fact, if I attach the first surface with texture 1, I get a black window as well so it seems we must do this with texture 0 active.

Anyway, this works but maybe there is a better way to manage this state. I guess I need to learn more about opengl.

Copy link
Contributor

Choose a reason for hiding this comment

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

FTR both wlc and weston do some special shit with GL textures that I don't fully understand and have not replicated here.

Copy link
Author

Choose a reason for hiding this comment

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

This is a really good explanation of texture units that I read: https://open.gl/textures#Textureunits

Implement surface_attach method. This is called when a client attaches an shm
buffer with wl_surface_attach().

Implement the GLES2 interface for attaching shm buffers. This creates an opengl
texture with the shm buffer contents for the surface.

This commit also includes some working code to render the surfaces onto the
screen for demonstration purposes.
@acrisci acrisci force-pushed the feature/surface-interface-stub branch from 1b38485 to 6610aa7 Compare August 4, 2017 15:41
uint8_t *pixels = wl_shm_buffer_get_data(buffer);
int width = wl_shm_buffer_get_width(buffer);
int height = wl_shm_buffer_get_height(buffer);
int pitch = wl_shm_buffer_get_stride(buffer) / (fmt->bpp / 8);
Copy link
Author

@acrisci acrisci Aug 4, 2017

Choose a reason for hiding this comment

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

Weston calls this variable pitch although I don't know if that term is the same as stride. This division I added is necessary for setting GL_UNPACK_ROW_LENGTH_EXT because otherwise I get memory errors from stride being too large and the opengl skipping too many pixels.

Wayland seems to include some sanity checks for us so a buggy stride will never cause these memory errors as long as we do this calculation.

Add a signal for wlr_surface destruction on the wlr_surface that compositors
can listen to to remove the surface from their state.

Implement a listener for this in the example wl_compositor to remove the
surface from its internal list of surfaces.

Destroy the surface in the compositor destroy_surface callback given when the
surface resource was created.

Add a reference to the surface resource to the wlr_surface so a compositor can
find it in its list of resources upon wlr_resource destruction.
@acrisci
Copy link
Author

acrisci commented Aug 4, 2017

The next commit implements destroying surfaces and is ready for review.

I hate to add another callback for surface destruction (there are 3 now), but it was the best way I could find to get both the compositor state and the wlr_surface in the same callback.

This fixes the segfault when surfaces are destroyed.

@ddevault ddevault merged commit de17ce1 into swaywm:master Aug 4, 2017
@ddevault
Copy link
Contributor

ddevault commented Aug 4, 2017

Great work, thanks!

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.

2 participants