After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 787715 - Screen cast stream properties and screen capture bug fix
Screen cast stream properties and screen capture bug fix
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks:
 
 
Reported: 2017-09-15 09:23 UTC by Jonas Ådahl
Modified: 2017-09-20 10:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
screen-cast-stream: Add stream properties (8.96 KB, patch)
2017-09-15 09:23 UTC, Jonas Ådahl
committed Details | Review
clutter: Fix capture_into on non-origin view (3.04 KB, patch)
2017-09-15 09:23 UTC, Jonas Ådahl
committed Details | Review

Description Jonas Ådahl 2017-09-15 09:23:44 UTC
Two patches will accompany this bug:

1. Add a properties bag to org.gnome.Mutter.ScreenCast.Stream

This is technically an API change, but this since the screen cast API is still
experimental, and the fact that it's purely an addition, I'd argue that it's
within reason.

1. Fix capturing on non-origin monitor

There was a bug in the capture_into() code causing memory corruption when
capturing from a non-origin stage view. It's an API change, but this function
is only used by the screen cast code, and not exposed outside of mutter, so
should also be fine.
Comment 1 Jonas Ådahl 2017-09-15 09:23:49 UTC
Created attachment 359836 [details] [review]
screen-cast-stream: Add stream properties

For monitor streams, add position and size (in compositor coordinate
space) properties.
Comment 2 Jonas Ådahl 2017-09-15 09:23:54 UTC
Created attachment 359837 [details] [review]
clutter: Fix capture_into on non-origin view

The capture_into() function wrote out of bounds when capturing from a
non-origin view (not positioned at (0,0)). At the time of
implementation, this API was used to capture pixels across views into a
single data buffer, but the way it's used, and the way views may work
now, makes this impossible to do properly.

So remove the ability to capture into a pre-allocated buffer from
multiple views, and complain if the passed rectantgle overlapps with
multiple views. This removes the broken offset calculation all
together, fixing the bug motivating this change.
Comment 3 Rui Matos 2017-09-19 16:24:23 UTC
Review of attachment 359836 [details] [review]:

just a naming nit but looks fine

::: src/org.gnome.Mutter.ScreenCast.xml
@@ +114,3 @@
+		       coordinate space.
+    -->
+    <property name="Properties" type="a{sv}" access="read" />

maybe call this Parameters? it gets a bit confusing since this is also a DBus property and a GObject property called... properties.
Comment 4 Rui Matos 2017-09-19 16:40:19 UTC
Review of attachment 359837 [details] [review]:

::: clutter/clutter/clutter-stage.c
@@ +4882,3 @@
 
+      g_warn_if_fail (view_capture_rect.width == rect->width &&
+                      view_capture_rect.height == rect->height);

why warn if it works? if this is a mutter bug we should assert

@@ +4899,3 @@
+
+  view = get_view_at_rect (stage, rect);
+  g_return_if_fail (view);

same, if it's a mutter bug we should assert otherwise just return silently
Comment 5 Jonas Ådahl 2017-09-20 02:01:26 UTC
(In reply to Rui Matos from comment #3)
> Review of attachment 359836 [details] [review] [review]:
> 
> just a naming nit but looks fine
> 
> ::: src/org.gnome.Mutter.ScreenCast.xml
> @@ +114,3 @@
> +		       coordinate space.
> +    -->
> +    <property name="Properties" type="a{sv}" access="read" />
> 
> maybe call this Parameters? it gets a bit confusing since this is also a
> DBus property and a GObject property called... properties.

Good point.

(In reply to Rui Matos from comment #4)
> Review of attachment 359837 [details] [review] [review]:
> 
> ::: clutter/clutter/clutter-stage.c
> @@ +4882,3 @@
>  
> +      g_warn_if_fail (view_capture_rect.width == rect->width &&
> +                      view_capture_rect.height == rect->height);
> 
> why warn if it works? if this is a mutter bug we should assert
> 
> @@ +4899,3 @@
> +
> +  view = get_view_at_rect (stage, rect);
> +  g_return_if_fail (view);
> 
> same, if it's a mutter bug we should assert otherwise just return silently

Good point. I'll remove this (it'd segfault nicely anyway), and change the one above to an assert.
Comment 6 Jonas Ådahl 2017-09-20 10:29:59 UTC
Attachment 359837 [details] pushed as ba194bd - clutter: Fix capture_into on non-origin view