Andrew, Thanks for reviewing. Comments follow. On Mar 27, 2014 7:02 AM, "Andrew Wedgbury" <<a href="mailto:andrew.wedgbury@realvnc.com">andrew.wedgbury@realvnc.com</a>> wrote: > > Hi Jason, > > A few comments I have from trying out your fullscreen-shell changes: > > > On Tue, 18 Mar 2014, Jason Ekstrand wrote: > >> +static struct ss_seat * >> +ss_seat_create(struct shared_output *so, uint32_t id) >> +{ >> + struct ss_seat *seat; >> + >> + seat = zalloc(sizeof *seat); >> + if (seat == NULL) >> + return NULL; >> + >> + weston_seat_init(&seat->base, so->output->compositor, "default"); > > > It would be great if we could transfer the seat name from the parent seat. In the case of VNC or RDP this could identify the remote user. That's not a bad idea. I'll look into it. > > >> + seat->output = so; >> + seat->parent.seat = wl_registry_bind(so->parent.registry, id, >> + &wl_seat_interface, 1); >> + wl_list_insert(so->seat_list.prev, &seat->link); >> + >> + wl_seat_add_listener(seat->parent.seat, &ss_seat_listener, seat); >> + wl_seat_set_user_data(seat->parent.seat, seat); >> + >> + return seat; >> +} > > >> +static struct shared_output * >> +shared_output_create(struct weston_output *output, int parent_fd) >> +{ >> + struct shared_output *so; >> + struct wl_event_loop *loop; >> + struct ss_seat *seat; >> + int epoll_fd; >> + >> + so = zalloc(sizeof *so); >> + if (so == NULL) >> + goto err_close; >> + >> + wl_list_init(&so->seat_list); >> + + so->parent.display = wl_display_connect_to_fd(parent_fd); >> + if (!so->parent.display) >> + goto err_alloc; >> + + so->parent.registry = wl_display_get_registry(so->parent.display); >> + if (!so->parent.registry) >> + goto err_display; >> + wl_registry_add_listener(so->parent.registry, >> + &registry_listener, so); >> + wl_display_roundtrip(so->parent.display); >> + if (so->parent.shm == NULL) { >> + weston_log("Screen share failed: No wl_shm found\n"); >> + goto err_display; >> + } >> + if (so->parent.fshell == NULL) { >> + weston_log("Screen share failed: " >> + "Parent does not support wl_fullscreen_shell\n"); >> + goto err_display; >> + } >> + if (so->parent.compositor == NULL) { >> + weston_log("Screen share failed: No wl_compositor found\n"); >> + goto err_display; >> + } >> + >> + /* Get SHM formats */ >> + wl_display_roundtrip(so->parent.display); >> + if (!(so->parent.shm_formats & (1 << WL_SHM_FORMAT_XRGB8888))) { >> + weston_log("Screen share failed: " >> + "WL_SHM_FORMAT_XRGB8888 not available\n"); >> + goto err_display; >> + } >> + >> + so->parent.surface = >> + wl_compositor_create_surface(so->parent.compositor); >> + if (!so->parent.surface) { >> + weston_log("Screen share failed: %m"); >> + goto err_display; >> + } >> + >> + so->parent.mode_feedback = >> + _wl_fullscreen_shell_present_surface_for_mode(so->parent.fshell, >> + so->parent.surface, >> + so->parent.output, >> + output->current_mode->refresh); >> + if (!so->parent.mode_feedback) { >> + weston_log("Screen share failed: %m"); >> + goto err_display; >> + } >> + _wl_fullscreen_shell_mode_feedback_add_listener(so->parent.mode_feedback, >> + &mode_feedback_listener, >> + so); >> + >> + loop = wl_display_get_event_loop(output->compositor->wl_display); >> + >> + epoll_fd = wl_display_get_fd(so->parent.display); >> + so->event_source = >> + wl_event_loop_add_fd(loop, epoll_fd, WL_EVENT_READABLE, >> + shared_output_handle_event, so); >> + if (!so->event_source) { >> + weston_log("Screen share failed: %m"); >> + goto err_display; >> + } >> + + /* Ok, everything's created. We should be good to go */ >> + wl_list_init(&so->shm.buffers); >> + wl_list_init(&so->shm.free_buffers); >> + >> + so->output = output; >> + so->output_destroyed.notify = output_destroyed; >> + wl_signal_add(&so->output->destroy_signal, &so->output_destroyed); >> + >> + so->frame_listener.notify = shared_output_repainted; >> + wl_signal_add(&output->frame_signal, &so->frame_listener); >> + output->disable_planes++; > > > I think you should only disable planes if the fullscreen_shell doesn't have the cursor_plane capability. In that case, there also needs to be a way to update the pointer surface on the parent seat. Although I think the difficulty may be in detecting when the pointer image changes - at least I remember that being a problem in my previous attempt at doing this. Actually, it's more subtle than that. Weston uses more than just the cursor plane. It can also throw entire windows into planes to avoid unnecisary re-rendering. In order to do a screen capture, we have to do a full composite. We could, in theory, only use a plane for the one cursor, but I'm not sure whether or not that would work with screen sharing. I should definitely look into it for the case where the compositor is running stand-alone on top of the VNC/RDP server. > > I guess you also need a corresponding disable_planes-- when screen sharing > stops? Yes, I should. Do I not have one? I should add one to shared_output_destroy. Thanks for reviewing, --Jason Ekstrand > > >> + weston_output_damage(output); >> + + return so; >> + >> +err_display: >> + wl_list_for_each(seat, &so->seat_list, link) >> + ss_seat_destroy(seat); >> + wl_display_disconnect(so->parent.display); >> +err_alloc: >> + free(so); >> +err_close: >> + close(parent_fd); >> + return NULL; >> +} > > > --- > Andrew Wedgbury <<a href="mailto:andrew.wedgbury@realvnc.com">andrew.wedgbury@realvnc.com</a>> > >