[Spice-devel] [RFC spice-gtk v2 0/1] Direct rendering

[Snir Sheriber]

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&format 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