[Spice-devel] [RFC spice-server 1/2] streaming: Restart streams on video-codec changes

[Uri Lublin]

On 6/27/19 12:19 PM, Kevin Pouget wrote:
> Hello Uri,
> 
> On Thu, Jun 27, 2019 at 11:04 AM Uri Lublin <uril at redhat.com> wrote:
>>
>> On 6/27/19 11:40 AM, Frediano Ziglio wrote:
>>>>
>>>> Interrupt/restart the video streams when the user changes the
>>>> preferred video-codecs (dcc_handle_preferred_video_codec_type) or when
>>>> the host admin updates the list of video-codecs allowed
>>>> (display_channel_set_video_codecs)
>>
>> Hi,
>>
>> This patch only stops the video stream.
>> A new stream will be started automatically according to spice rules
>> (video-stream detection).
> 
> indeed, I wasn't sure about the wording, because the purpose of the
> patch is to trigger a stream restart, it's not to stop it, so I don't
> know which word to put in the commit title! (I'll add your sentence in
> the body anyway to clarify)

Yes, the title is good -- it states the purpose of this patch.
Thanks for adding the additional sentence to the body.

Uri.

> 
>>>> ---
>>>>    server/dcc.c             | 2 ++
>>>>    server/display-channel.c | 2 ++
>>>>    server/video-stream.c    | 5 +++++
>>>>    3 files changed, 9 insertions(+)
>>>>
>>>> diff --git a/server/dcc.c b/server/dcc.c
>>>> index a94e27e8..0deeed88 100644
>>>> --- a/server/dcc.c
>>>> +++ b/server/dcc.c
>>>> @@ -1201,6 +1201,8 @@ static int
>>>> dcc_handle_preferred_video_codec_type(DisplayChannelClient *dcc,
>>>>
>>>>        /* New client preference */
>>>>        dcc_update_preferred_video_codecs(dcc);
>>>> +    video_stream_detach_and_stop(DCC_TO_DC(dcc));
>>>> +
>>>>        return TRUE;
>>>>    }
>>>>
>>>> diff --git a/server/display-channel.c b/server/display-channel.c
>>>> index 071c0140..4b8e6e90 100644
>>>> --- a/server/display-channel.c
>>>> +++ b/server/display-channel.c
>>>> @@ -255,6 +255,8 @@ void display_channel_set_video_codecs(DisplayChannel
>>>> *display, GArray *video_cod
>>>>        g_clear_pointer(&display->priv->video_codecs, g_array_unref);
>>>>        display->priv->video_codecs = g_array_ref(video_codecs);
>>>>        g_object_notify(G_OBJECT(display), "video-codecs");
>>>> +
>>>> +    video_stream_detach_and_stop(display);
>>>>    }
>>>>
>>>>    GArray *display_channel_get_video_codecs(DisplayChannel *display)
>>>> diff --git a/server/video-stream.c b/server/video-stream.c
>>>> index 4ac25af8..f227713b 100644
>>>> --- a/server/video-stream.c
>>>> +++ b/server/video-stream.c
>>>> @@ -925,6 +925,11 @@ void video_stream_detach_and_stop(DisplayChannel
>>>> *display)
>>>>        RingItem *stream_item;
>>>>
>>>>        spice_debug("trace");
>>>> +
>>>> +    if (!ring_is_initialized(&display->priv->streams)) {
>>>> +        return;
>>>> +    }
>>>> +
>>>
>>> If this happens I would consider a program error and I would abort().
>>>
>>>>        while ((stream_item = ring_get_head(&display->priv->streams))) {
>>
>> When the ring is empty, ring_get_head returns NULL, and the
>> while loop exits immediately. So this case is already covered.
>> (It also aborts (spice_assert) if the the ring is "not initialized")
> 
> yes, it was a bug that the stream was uninitialized at this stage;
> hence ring_is_empty was aborting the execution
> 
> I will let video_stream_detach_and_stop untouched
> 
> 
> thanks,
> 
> Kevin
> 
>>>>            VideoStream *stream = SPICE_CONTAINEROF(stream_item, VideoStream,
>>>>            link);
>>>>
>>>
>>> Frediano
>>> _______________________________________________
>>> Spice-devel mailing list
>>> Spice-devel at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>>>
>>