[Spice-devel] [PATCH spice-server 2/7] VideoStream: store channel in stream

[Uri Lublin]

On 12/04/2017 06:07 PM, Frediano Ziglio wrote:
>>
>> On 11/29/2017 10:16 PM, Jonathon Jongsma wrote:
>>> This allows us to unref the stream directly rather than needing to pass
>>> the associated DisplayChannel to stream_unref(). The same is also true
>>> for stream_agent_unref, since the only reason that stream_agent_unref()
>>> required a DisplayChannel parameter was to pass it to stream_unref().
>>> This also resulted in some additional minor redesign due to the fact
>>> that streams are pre-allocated in a pool in the display channel, and
>>> freeing a stream simply means moving it to a different ring to be
>>> re-used.
>>>
>>> Signed-off-by: Jonathon Jongsma <jjongsma at redhat.com>
>>> ---
>>>    server/display-channel.c | 27 ++++++++++++++++++++++++++-
>>>    server/display-channel.h |  1 +
>>>    server/video-stream.c    | 44
>>>    ++++++++++++--------------------------------
>>>    server/video-stream.h    |  8 ++++----
>>>    4 files changed, 43 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/server/display-channel.c b/server/display-channel.c
>>> index 2caaa6433..d3db1e0db 100644
>>> --- a/server/display-channel.c
>>> +++ b/server/display-channel.c
>>
>>> +
>>> +void display_channel_free_video_stream(DisplayChannel *display,
>>> VideoStream *stream)
>>
>> Hi Jonathon,
>>
>> You do not need to pass "display" here. Just use stream->display.
>>
>>> +{
>>> +    display_channel_mark_stream_unused(stream->display, stream);
>>> +    display->priv->stream_count--;
>>> +}
>>> +
>>
> 
> Jonathon is also trying to separate streaming in a "right" way so
> if display-channel.c needs to access a new VideoStream field is not
> really ideal.
> 
>>
>>> diff --git a/server/display-channel.h b/server/display-channel.h
>>> index 4f7def216..a0470ec67 100644
>>> --- a/server/display-channel.h
>>> +++ b/server/display-channel.h
>>> @@ -168,6 +168,7 @@ gboolean
>>> display_channel_surface_has_canvas(DisplayChannel *display, uint32_t su
>>>    void display_channel_reset_image_cache(DisplayChannel *self);
>>>    
>>>    void display_channel_debug_oom(DisplayChannel *display, const char *msg);
>>> +void display_channel_free_video_stream(DisplayChannel *display,
>>> VideoStream *stream);
>>
>> And here.
>>
>>>    
>>>    G_END_DECLS
>>>    
>>> diff --git a/server/video-stream.c b/server/video-stream.c
>>> index 65da3f8a0..a24c787ef 100644
>>> --- a/server/video-stream.c
>>> +++ b/server/video-stream.c
>>
>> <snip>
>>
>>> -void video_stream_unref(DisplayChannel *display, VideoStream *stream)
>>> +void video_stream_unref(VideoStream *stream)
>>>    {
>>>        if (--stream->refs != 0)
>>>            return;
>>>    
>>>        spice_warn_if_fail(!ring_item_is_linked(&stream->link));
>>>    
>>> -    video_stream_free(display, stream);
>>> -    display->priv->stream_count--;
>>> +    display_channel_free_video_stream(stream->display, stream);
>>
>> This is the only caller and it uses stream->display.
>>
>> Thanks,
>>       Uri.
> 
> Uri,
>     why is not clear to you the design point? Is the comment not clear
> about? I personally think that "This also resulted in some additional
> minor redesign"... sentence does not explain why this resulted in
> redesign and which is the redesign.
> 
> Frediano
> 

My comment is not about the design, but about the specific patch.
The patch itself does not seems to achieve the design goal, but as
Jonathon mentioned (in one of his replies) he feels it's a move
in the right direction.

Now for the specific patch.
Since struct VideoStream  now contains DisplayChannel *display
1. You may as well use it in when calling 
display_channel_free_video_stream()
2. In display_channel_free_video_stream itself:
+    display_channel_mark_stream_unused(stream->display, stream);
+    display->priv->stream_count--;

    The first line uses stream->display
    The second line uses display directly.


If you want to pass "display" and not use stream->display
the first line should be changed accordingly.

Uri.