>
> 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.
>
The patch specifically says "This also resulted in some additional
minor redesign", your comment refers to part of the patch.
I think we all agree the patch does not achieve a goal but from
your "he feels it's a move in the right direction" I can understand
you also are not sure about that direction.
>From Jonathon comments:
- "my long-term goal was to have video-stream.[ch] only contain the
types and functions directly related to VideoStream (and potentially
VideoStreamAgent). In other words: member functions"
So member functions and VideoStream means to me VideoStream encapsulation
that is all or most of the fields in this structure should be accesses
from video-stream.c. Accessing an additional "display" field from
display-channel.c goes in the exact opposite direction of this.
Also the "potentially VideoStreamAgent" clearly indicate that there
are some uncertainty on the design. Not a bit issue but we should not
follow the uncertainty.
- "It should not need to access the internals of any other types (for
example: DisplayChannel or DisplayChannelClient)". This talks about
responsibility and dependency. Adding a display field to VideoStream
goes in the exact opposite direction of the dependency (if the previous
sentence was supposing dependency).
As I mentioned above it seems to me that there's are no much certainty
on the "design" and that is hard to say if this patch goes into the
right direction if you don't know what do you want. And as I say above I
also see that some changes are going in the exact opposite direction of
the partial design idea.
Looking at the partial design mentioned above and at the actual code
I would really try to add a VideoStreams objects. It seems to me that
there is a strong indication of a global VideoStream state that is
partially in DisplayChannel. For instance:
- VideoStream has a next field. So.. what's this next field? To me
it seems to indicate that VideoStream is by definition contained
in a container; Somebody could argue that this field is only used
by the pooling so could be moved to DisplayChannel somehow;
- VideoStream has a link field. This looks similar to the above case
but this field is used a lot in video-stream.c which more strongly
indicate that being in a container is a property of this object
(if we want to have an object);
- VideoStream has an id. Yes, currently this id is retrieved with
display_channel_get_video_stream_id but this still have clear
VideoStream -> DisplayChannel dependency (which seems to go again
the above design") and is still suggesting to have a container;
If we agree on the VideoStream inside a container property would
be sensible to have this field inside VideoStream.
As said I don't have exactly a 100% design in mind and are not ack/nack
about this series but I want to have some more clear understand of
the design we want to be able to say "yes, this is going in the
direction we want" instead of a "ok, YOU feel that way I trust you, ack"
which is IMHO a wrong way to review patches (should be "ok, I feel that
way too, ack").
My initial series (never posted) for this "separation" was dragged
by the idea that "some fields in DisplayChannel are dealing with streaming
and they should be in stream.c". I think the design from Jonathon
are more on the "I feel VideoStream should be an object".
Both are partial design/solution I think and as such they both partially
goes in some not really specified direction. I think would be great to
have a well defined target.
> 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.
>
Frediano