[weston,RFC] compositor: Move all views to a new primary output

Submitted by Armin Krezović on June 30, 2016, 4:29 a.m.

Details

Message ID 20160630042948.14663-1-krezovic.armin@gmail.com
State Superseded
Delegated to: Derek Foreman
Headers show
Series "compositor: Move all views to a new primary output" ( rev: 1 ) in Wayland

Not browsing as part of any series.

Commit Message

Armin Krezović June 30, 2016, 4:29 a.m.
When primary output gets disconnected and there
were views created, they won't get assigned a
new output when a new primary output gets plugged
in. This will lead to crashes when weston tries
to use an already destroyed output.

This fixes the problems by force-moving all views
to the newly attached output if there were no
outputs present.

Signed-off-by: Armin Krezović <krezovic.armin@gmail.com>
---
 libweston/compositor.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Patch hide | download patch | download mbox

diff --git a/libweston/compositor.c b/libweston/compositor.c
index eb9e8d9..62687cf 100644
--- a/libweston/compositor.c
+++ b/libweston/compositor.c
@@ -4313,8 +4313,18 @@  WL_EXPORT void
 weston_compositor_add_output(struct weston_compositor *compositor,
                              struct weston_output *output)
 {
+	struct weston_view *view;
+	int reassign_outputs = 0;
+
+	if (wl_list_empty(&compositor->output_list))
+		reassign_outputs = 1;
+
 	wl_list_insert(compositor->output_list.prev, &output->link);
 	wl_signal_emit(&compositor->output_created_signal, output);
+
+	if (reassign_outputs)
+		wl_list_for_each(view, &compositor->view_list, link)
+			weston_view_assign_output(view);
 }
 
 WL_EXPORT void

Comments

On Thu, 30 Jun 2016 06:29:48 +0200
Armin Krezović <krezovic.armin@gmail.com> wrote:

> When primary output gets disconnected and there
> were views created, they won't get assigned a
> new output when a new primary output gets plugged
> in. This will lead to crashes when weston tries
> to use an already destroyed output.

Hi Armin,

I wonder if we could avoid keeping stale output pointers in the first
place. The *_assing_output() machinery would already set them to NULL
if it just got called.

Maybe removing an output should cause all views to be marked dirty?

Removing or adding outputs can cause shells to move views too.
We want to avoid calling assign_outputs in the middle of a sequence of
reorganizing views, so that clients don't receive unnecessary
wl_surface.enter/leave events, which may cause redrawing.

> This fixes the problems by force-moving all views
> to the newly attached output if there were no
> outputs present.
> 
> Signed-off-by: Armin Krezović <krezovic.armin@gmail.com>
> ---
>  libweston/compositor.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/libweston/compositor.c b/libweston/compositor.c
> index eb9e8d9..62687cf 100644
> --- a/libweston/compositor.c
> +++ b/libweston/compositor.c
> @@ -4313,8 +4313,18 @@ WL_EXPORT void
>  weston_compositor_add_output(struct weston_compositor *compositor,
>                               struct weston_output *output)
>  {
> +	struct weston_view *view;
> +	int reassign_outputs = 0;
> +
> +	if (wl_list_empty(&compositor->output_list))
> +		reassign_outputs = 1;
> +
>  	wl_list_insert(compositor->output_list.prev, &output->link);
>  	wl_signal_emit(&compositor->output_created_signal, output);
> +
> +	if (reassign_outputs)
> +		wl_list_for_each(view, &compositor->view_list, link)
> +			weston_view_assign_output(view);
>  }
>  
>  WL_EXPORT void

How about we do this every time an output get plugged in or out, not
just when the first output gets plugged in?

That way the output assignments get automatically updated if e.g. a
view is hanging off the edge of one output and into the region where
the new output will be.

And instead of calling weston_view_assign_output() directly, call
weston_view_geometry_dirty()?

Then *_assign_output() would get called as part of the repaint next
time via weston_view_update_transform(), and it would coalesce any view
manipulations done by the shell.

When outputs are removed, it is possible that a view from that output
will no longer be on any output. That's not a problem if at least one
output remains, because weston_output_repaint() will end up calling
update_transform and assign_output. But when the very last output gets
unplugged, there is no repaint to call those anymore. In that case we
should probably call weston_view_assign_output() directly. It cannot
cause glitching output assignments either, because even if the shell
did move the view, it cannot get an output assigned anyway.

Or would it make better sense for weston_{surface,view} to also
subscribe to the destruction of the assigned output, or even maintain a
per-output list of assigned views and surfaces. I wonder...


Thanks,
pq
On 05.07.2016 13:42, Pekka Paalanen wrote:
> On Thu, 30 Jun 2016 06:29:48 +0200
> Armin Krezović <krezovic.armin@gmail.com> wrote:
> 
>> When primary output gets disconnected and there
>> were views created, they won't get assigned a
>> new output when a new primary output gets plugged
>> in. This will lead to crashes when weston tries
>> to use an already destroyed output.
> 
> Hi Armin,
> 
> I wonder if we could avoid keeping stale output pointers in the first
> place. The *_assing_output() machinery would already set them to NULL
> if it just got called.
> 
> Maybe removing an output should cause all views to be marked dirty?
> 
> Removing or adding outputs can cause shells to move views too.
> We want to avoid calling assign_outputs in the middle of a sequence of
> reorganizing views, so that clients don't receive unnecessary
> wl_surface.enter/leave events, which may cause redrawing.
> 
>> This fixes the problems by force-moving all views
>> to the newly attached output if there were no
>> outputs present.
>>
>> Signed-off-by: Armin Krezović <krezovic.armin@gmail.com>
>> ---
>>  libweston/compositor.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/libweston/compositor.c b/libweston/compositor.c
>> index eb9e8d9..62687cf 100644
>> --- a/libweston/compositor.c
>> +++ b/libweston/compositor.c
>> @@ -4313,8 +4313,18 @@ WL_EXPORT void
>>  weston_compositor_add_output(struct weston_compositor *compositor,
>>                               struct weston_output *output)
>>  {
>> +	struct weston_view *view;
>> +	int reassign_outputs = 0;
>> +
>> +	if (wl_list_empty(&compositor->output_list))
>> +		reassign_outputs = 1;
>> +
>>  	wl_list_insert(compositor->output_list.prev, &output->link);
>>  	wl_signal_emit(&compositor->output_created_signal, output);
>> +
>> +	if (reassign_outputs)
>> +		wl_list_for_each(view, &compositor->view_list, link)
>> +			weston_view_assign_output(view);
>>  }
>>  
>>  WL_EXPORT void
> 
> How about we do this every time an output get plugged in or out, not
> just when the first output gets plugged in?
> 
> That way the output assignments get automatically updated if e.g. a
> view is hanging off the edge of one output and into the region where
> the new output will be.
> 
> And instead of calling weston_view_assign_output() directly, call
> weston_view_geometry_dirty()?
> 
> Then *_assign_output() would get called as part of the repaint next
> time via weston_view_update_transform(), and it would coalesce any view
> manipulations done by the shell.
> 
> When outputs are removed, it is possible that a view from that output
> will no longer be on any output. That's not a problem if at least one
> output remains, because weston_output_repaint() will end up calling
> update_transform and assign_output. But when the very last output gets
> unplugged, there is no repaint to call those anymore. In that case we
> should probably call weston_view_assign_output() directly. It cannot
> cause glitching output assignments either, because even if the shell
> did move the view, it cannot get an output assigned anyway.
> 
> Or would it make better sense for weston_{surface,view} to also
> subscribe to the destruction of the assigned output, or even maintain a
> per-output list of assigned views and surfaces. I wonder...
> 
> 
> Thanks,
> pq
> 

Hi, setting transform.dirty to 1 makes sense. When an output is destroyed,
all views from that output are moved to another output. We can modify
the following snippet to set transform.dirty to 1 for a view if output
list is empty. That would make the patch even simpler.

        wl_list_for_each(view, &output->compositor->view_list, link) {
                if (view->output_mask & (1u << output->id))
                        weston_view_assign_output(view);
        }

Thanks, Armin.