On Wed, 11 Dec 2013 12:20:41 +0100 Giulio Camuffo <giuliocamu...@gmail.com> wrote:
> 2013/12/11 Pekka Paalanen <ppaala...@gmail.com>: > > On Wed, 11 Dec 2013 11:23:50 +0100 > > Giulio Camuffo <giuliocamu...@gmail.com> wrote: > > > >> 2013/12/11 Pekka Paalanen <ppaala...@gmail.com>: > >> > On Tue, 10 Dec 2013 15:55:29 +0100 > >> > Giulio Camuffo <giuliocamu...@gmail.com> wrote: > >> > > >> >> if a surface has not a buffer yet and a weston_view gets created > >> >> for it, the surface's width and height will be 0 and the view's > >> >> output_mask will be 0, because the surface's area is 0. Later > >> >> commits on the surface with valid buffers will then trigger a > >> >> surface repaint, which will do nothing because of the > >> >> output_mask set to 0. So by calling weston_update_transform() > >> >> on the views of the surface at the end of > >> >> weston_surface_commit() we update the output_mask of the > >> >> surface and of the views, so they will be repainted. --- > >> >> src/compositor.c | 7 ++++++- > >> >> 1 file changed, 6 insertions(+), 1 deletion(-) > >> >> > >> >> diff --git a/src/compositor.c b/src/compositor.c > >> >> index 6beea9c..20d9efb 100644 > >> >> --- a/src/compositor.c > >> >> +++ b/src/compositor.c > >> >> @@ -2005,7 +2005,6 @@ weston_surface_commit(struct > >> >> weston_surface *surface) surface->pending.buffer = NULL; > >> >> surface->pending.sx = 0; > >> >> surface->pending.sy = 0; > >> >> - surface->pending.newly_attached = 0; > >> >> > >> >> /* wl_surface.damage */ > >> >> pixman_region32_union(&surface->damage, &surface->damage, > >> >> @@ -2046,7 +2045,13 @@ weston_surface_commit(struct > >> >> weston_surface *surface) > >> >> weston_surface_commit_subsurface_order(surface); > >> >> > >> >> + if (surface->pending.newly_attached) { > >> >> + wl_list_for_each(view, &surface->views, > >> >> surface_link) > >> >> + weston_view_update_transform(view); > >> >> + } > >> >> weston_surface_schedule_repaint(surface); > >> >> + > >> >> + surface->pending.newly_attached = 0; > >> >> } > >> >> > >> >> static void > >> > > >> > Hi, > >> > > >> > Why is this fix needed in the first place, in what use case does > >> > this bug manifest? Is this a regression? What introduced it? > >> > >> It isn't a regression. I noticed it now because it happens when > >> there are no other mapped surfaces on the same output. > > > > No other mapped surfaces at all, or surfaces that are just not > > updating? I noticed some comment in irc about updating. > > Right, there can be other mapped surfaces as long as they don't > update. If they update weston_output_repaint() is called, and that > will call update_transform() on also the view of the other surface. I guess output->repaint_needed never gets set, then. If output_mask is zero, weston_{surface,view}_schedule_repaint() functions never call weston_output_schedule_repaint(). I see. > >> > Weren't we relying on the weston_surface->configure call that > >> > maps the surface the first time to enforce a non-zero > >> > output_mask? > >> > >> You mean the configure handler should call > >> weston_update_transform? It seems to me the handlers should call > >> weston_view_geometry_dirty() if needed, and then > >> weston_update_transform() should be called by weston itself (which > >> is what happens, usually). The problem here is that it wasn't > >> called, ever. > > > > There are two essentially different cases for the configure handler: > > 1. if surface is not mapped yet, ->configure() maps it > > 2. if surface is mapped, ->configure() configures it > > > > For mapping case, AFAIK we have always relied on the ->configure() > > ending up calling weston_view_update_transform() directly, to force > > the surface become mapped (which is somehow strangely equivalent to > > assigning an output). Previously it was > > weston_surface_update_transform() and before that IIRC we called > > weston_surface_assign_output() directly or something. > > > > For the second case, ->configure() must not call > > weston_view_update_transform() to avoid updating the surface state > > prematurely. Instead, it ends up calling > > weston_view_geometry_dirty(). In this case, the surface state gets > > updated when it gets painted onto an output. If the surface is not > > on an output, there is no need to update its state either. > > > > Or, that is how it used to be, a long time ago. I believe there was > > a short discussion at the time whether we'd need a ->map() hook in > > addition to ->configure(), but it was deemed not useful. Or maybe we > > even had a ->map() and it got removed, can't recall. > > > > That is why the shell.c code had traces of two different code paths > > I think, one for map and the other for configure, both originating > > in the ->configure() callback. > > The problem is how to find out which case it is in the configure > handler. My surface is not a wl_shell_surface, and i get it with a > request which also sends an output. So i set the surface's output to > the given one immediately, and weston_surface_is_mapped() in the > configure handler returns always true even if output_mask is still 0. > Changing the condition in weston_surface_is_mapped() to "if > (surface->output && (surface->output_mask & (1 << > surface->output->id)))" would fix it, though. Otherwise I would need > to wrap my surface in a struct and use that to store the output and > assign it to the surface later, but that add complexity where it is > not needed, really. Yeah, I think the solution to that is to make weston_surface_is_mapped() more sane, and stop relying on the output assignment... or make it only look at output_mask. Not sure. So many incremental changes have piled up, that it might be time to clean it up a bit. And what about "surface is in a layer" vs. "surface is mapped"... weston_surface_is_mapped() existed before output_mask, and you were not supposed to set weston_surface::output yourself. Setting it was the job of weston_surface_assign_output() or similar, which computed the output based on the surface geometry. So yes, it was sort of expected, that you always have your own struct to go with a weston_surface, if you need associated data like the intended output. That is what the configure_private is for. I can't say which way would be the way to go. Thanks, pq > >> > If you look at all (most?) of the functions in shell.c that > >> > actually map surfaces, starting from an unmapped state, you see > >> > that they a) put the view into a layer, and b) forcefully call > >> > weston_view_update_transform(), which will then set up the > >> > output_mask. Or at least it used to be like that, now the only > >> > case I can clearly see in the code is lock_surface_configure(), > >> > but looks like map() with at least shell_map_popup() works > >> > roughly the same. > >> > > >> > There are likely also other details that have to be right in the > >> > right order when a surface is mapped, but I forget. Every surface > >> > type gets mapped slightly differently which is why we have not > >> > had a generic weston_surface_map() function. > >> > > >> > I don't think calling weston_view_update_transform() from the > >> > commit handler *always* when there is a new attach is right. It > >> > should happen only when mapping a surface. I believe there used > >> > to be the idea, that e.g. input events are generated with the > >> > old surface state, until output_repaint actually puts the new > >> > state on screen. Though, is that idea relevant anymore? > >> > >> I guess it could call it only if there wasn't a buffer before, if > >> this is still relevant. > >> > >> > > >> > (That actually shows a bug I think, a newly mapped surface may > >> > get input events before it is painted on screen, does it not? No > >> > wait, we only do input /from/ output_repaint? or do we? umm...) > >> > > >> > > >> > Thanks, > >> > pq > > _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/wayland-devel