Hi,
On 04/16/2018 12:37 PM, Frediano Ziglio wrote:
Differences from v1
-Recognize streaming mode by the streaming-mode surface flag
The propagation of this property (as English word, not gobject) is weird.
Seems that is a SpiceDisplay property but in reality is a surface propery
(which is stored in display_surface structure in spice-gtk), the previous
patch version stored the property on display_surface which seems more
correct.
Storing it in display_surface was suggested in your patch,
i agree it's more suitable as surface property but then it
wouldn't be accessible from the widget\SpiceDisplay, that
needs to know if streaming mode is enabled in order to
prepare the area and extract the handle from the window.
(maybe will be more suitable under SpiceDisplay.canvas?)
Storing the "handle" (currently xid) in channel seems more of an hack
than the proper way, a channel in theory can have multiple primary
surfaces (which was a request to implement). Also the handle keep
to be set even if the streaming surface is reset (this can happen
True, and needs to be checked.
AFAIU the idea was to separate the actual window (widget) from the
display-channel related to it, so that the only interface connected
between them is a canvas (pre-defined size buffer) and a
few gobject pre-defined status signals.
The thing is that decoding is currently done on the channel-side and
displaying/rendering is done on the widget side, while using the
gstvideooverlay can let gstreamer handle both decoding and
displaying/rendering and "breaks" the client's architecture (this was
mentioned before)
That's why it's kind of hackish now, I have several other ideas, all
of them requires pretty massive changes, not sure what is the proper
way, any thoughts are welcome.
for Virgl, although not implemented currently by the code). Similar,
what happen if the "Display" (in client terminology, the client window)
is closed? Does the xid is still valid? Is there a way to reduce the
yes, no xid in this case, but when could it happen?
CPU/GPU usage in this case (not strict requirement, mainly OT).
-Modifying the streaming mode signal
Feels more confusing that previous version.
Looking at current code the code (master) calls stream_display_frame
which emit a "display-invalidate" signal, captured by SpiceDisplay
(OT: why SpiceDisplay is implemented in spice-widget.c and not
in a spice-display.c ??). I imagine all SpiceDisplay(s) attached
to the same channel will handle the update filtering when the
rectangle does not intersect.
Maybe a signal like that emitted from channel-display-gst.c to
get the xid handle would make more sense and would support multiple
monitors?
True, it's indeed make more sense..
Although if we are necessarily going to stream each display separately
it should be equal i guess.
-Applying patches from Frediano (sent on v1 thread)
-Applying Uri's patch fixing a memory leak
-This feature can forced to be disabled now by setting the
DISABLE_GSTVIDEOOVERLAY environment variable
Small note: I would avoid to call g_getenv every time but
cache the value.
Can be done, although It's only when realize so it shouldn't
be called too many times.
-Does not create a new drawing area
Some issues
- Canvas is allocated although it's not always used
I won't consider this an issue, is not even a regression and
not strictly related to direct rendering.
- Needs to be tested with different plugins, environments...
I tested with Intel card with and without vaapi drivers and
with Xorg and Wayland.
Maybe would be useful to get a matrix?
- Not sure what is needed in order to make it to support
multi-monitor in the future.
- Currently works only with x (xid is transferred from
spice-widget to spice-gst-decoder which sets the overlay)
- There is no synchronization with audio! (decodes and
renders AFAP)
On my tests the PTS is taken into account so seems to
sync audio and video... weird!
Shouldn't be, it's in use only when appsink is in use, try to suspend
the client for a few seconds to get it out of sync
I'd be happy to hear more comments, ideas, patches :)
The results with this patch are great! With drivers installed
I'm getting 10% CPU usage using full HD!
Hope to see this patch in master soon!
\o/
Thanks for your patches, testing and reviewing
Thanks, Snir.
Snir Sheriber (1):
Gstreamer: Use GstVideoOverlay if possible
src/channel-display-gst.c | 99
++-
src/channel-display.c | 55 ++
src/channel-display.h | 3 ++
src/spice-widget-priv.h | 1 +
src/spice-widget.c| 40 ++-
5 files changed, 179 insertions(+), 19 deletions(-)
Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel
