[Spice-devel] [spice v8] dcc: handle preferred video codec message

[Christophe de Dinechin]

> On 10 Feb 2017, at 14:14, Victor Toso <victortoso at redhat.com> wrote:
> 
> Hi,
> 
> On Fri, Feb 10, 2017 at 07:51:05AM -0500, Frediano Ziglio wrote:
>>> [0] SPICE_MSGC_DISPLAY_PREFERRED_VIDEO_CODEC_TYPE
>>> 
>>> This message provides a list of video codecs based on client's order
>>> of preference.
>>> 
>>> We duplicate the video codecs array from reds.c and sort it using the
>>> order of codecs as reference.
>>> 
>>> This message will not change an ongoing streaming but it could change
>>> newly created streams depending the rank value of each video codec
>>> that can be set by spice_server_set_video_codecs()
>>> 
>>> Signed-off-by: Victor Toso <victortoso at redhat.com>
>>> ---
>>> server/dcc-private.h     |   5 ++
>>> server/dcc.c             | 126
>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>> server/dcc.h             |   1 +
>>> server/display-channel.c |   2 +
>>> server/stream.c          |   5 +-
>>> 5 files changed, 138 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/server/dcc-private.h b/server/dcc-private.h
>>> index 64b32a7..dd54c70 100644
>>> --- a/server/dcc-private.h
>>> +++ b/server/dcc-private.h
>>> @@ -51,6 +51,11 @@ struct DisplayChannelClientPrivate
>>>         int num_pixmap_cache_items;
>>>     } send_data;
>>> 
>>> +    /* Host prefererred video-codec order sorted with client preferred */
>>> +    GArray *preferred_video_codecs;
>> 
>> I know this patch is important but every time I arrive here I ask
>> "array of what?" and I close the email.
> 
> Ah :(
> 
>> 
>> C has type[] or type*... GArray is an array of everything you want...
>> I don't like it!
> 
> I used GArray here to keep consistency as we already use GArray for this
> RedVideoCodec struct in other places and this code actually interact
> with it.

GArray and type[] are not identical. GArray is dynamically allocated and can grow/shrink. It is closer to calloc/realloc than to type[].

In that case, I think that the number of codecs is fixed within the lifetime of the array. However, GArray naturally includes a size (the len field), whereas we don’t have the information for a plain array or pointer. So in that specific case, as we pass arrays around, even if they have a fixed size, I think GArray is the right choice.



> 
> I don't mind moving it to GPtrArray, at least you know that it will be
> an array of pointers.
> 
> Let me know what you think as while I was discussion with Pavel this
> morning, he find out another issue in the patch, which I plan to fix it
> in a followup patch later on.
> 
> PS: You do read the spice code tons of time more then I do, so I might
> not agree but I would do the change for what you think is more legible
> here (unless others disagree, of course)
> 
> Cheers,
>        toso
> 
>> 
>> Frediano
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel at lists.freedesktop.org <mailto:Spice-devel at lists.freedesktop.org>
>> https://lists.freedesktop.org/mailman/listinfo/spice-devel <https://lists.freedesktop.org/mailman/listinfo/spice-devel>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org <mailto:Spice-devel at lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/spice-devel <https://lists.freedesktop.org/mailman/listinfo/spice-devel>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170220/8a768573/attachment-0001.html>