[Spice-devel] [PATCH spice-gtk] spicy: make codec to string a bit safer
Marc-André Lureau
mlureau at redhat.com
Mon Mar 13 12:02:36 UTC 2017
Hi
----- Original Message -----
> Hi Marc-André,
>
> On Mon, 2017-03-13 at 07:40 -0400, Marc-André Lureau wrote:
> > Hi
> >
> > ----- Original Message -----
> > > Hi
> > >
> > > ----- Original Message -----
> > > > Hi,
> > > >
> > > > On Mon, Mar 13, 2017 at 07:11:02AM -0400, Marc-André Lureau
> > > > wrote:
> > > > > Hi
> > > > >
> > > > > ----- Original Message -----
> > > > > > Hi,
> > > > > >
> > > > > > On Mon, Mar 13, 2017 at 07:05:08AM -0400, Frediano Ziglio
> > > > > > wrote:
> > > > > > > >
> > > > > > > > From: Marc-André Lureau <marcandre.lureau at redhat.com>
> > > > > > > >
> > > > > > > > Handle unknown values instead of out-of-array access.
> > > > > > > >
> > > > > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau at redha
> > > > > > > > t.com>
> > > > > > > > ---
> > > > > > > > tools/spicy.c | 28 +++++++++++++++++++---------
> > > > > > > > 1 file changed, 19 insertions(+), 9 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/tools/spicy.c b/tools/spicy.c
> > > > > > > > index a41a1a3..2f6be4e 100644
> > > > > > > > --- a/tools/spicy.c
> > > > > > > > +++ b/tools/spicy.c
> > > > > > > > @@ -181,14 +181,24 @@ static int ask_user(GtkWidget
> > > > > > > > *parent, char
> > > > > > > > *title,
> > > > > > > > char *message,
> > > > > > > > return retval;
> > > > > > > > }
> > > > > > > >
> > > > > > > > -static const gchar *video_codec_enum_to_str[] = {
> > > > > > > > - [0] = "none",
> > > > > > > > - [SPICE_VIDEO_CODEC_TYPE_MJPEG] = "mjpeg",
> > > > > > > > - [SPICE_VIDEO_CODEC_TYPE_VP8] = "vp8",
> > > > > > > > - [SPICE_VIDEO_CODEC_TYPE_H264] = "h264",
> > > > > > > > - [SPICE_VIDEO_CODEC_TYPE_VP9] = "vp9",
> > > > > > > > - [SPICE_VIDEO_CODEC_TYPE_ENUM_END] = "error",
> > > > > > > > -};
> > > > > > > > +static const gchar *
> > > > > > > > +video_codec_to_string(SpiceVideoCodecType type)
> > > > > > > > +{
> > > > > > > > + const char *str = NULL;
> > > > > > > > + static const gchar *to_string[] = {
> > > > > > >
> > > > > > > this can be also const (not a regression by the way).
> > > > > > >
> > > > > > > > + NULL,
> > > > > > >
> > > > > > > This should be automatic and before was "none" instead but
> > > > > > > for
> > > > > > > me is fine.
> > > > > >
> > > > > > Ah, true. None is better because 0 means that there is no
> > > > > > ongoing
> > > > > > stream. So I would keep that too.
> > > > >
> > > > > Is it a "raw" codec then?
> > > >
> > > > You mean to use 'raw' instead of 'none'?
> > > > IMHO, "streaming: none" > "streaming: raw", but I don't mind to
> > > > change
> > > > it (but none or raw is better then "unknown codec" when value is
> > > > 0)
> > >
> > > Yes, I didn't realize [0] = 'none' was meant for a streaming
> > > codec.
> > >
> > > I'll change it to 'raw' then.
> >
> > Sigh, this is still unclear to me so there is obviously something to
> > improve.
> >
> > Is there a raw codec? If not, then why would you use this function
> > when there are no streams?
> there is no "raw" codec. It is either streaming using mjpeg, vp8, vp9,
> h264 - or not streaming (sends usual image updates).
>
> Sometimes you expect the server to create a stream, it is useful to
> have an info about the stream type (streaming: codec) or that it is
> not streaming (streaming: none)
Ok, to me using SpiceVideoCodecType with a 0 value is misleading or wrong.
I see it is being used in gst_opts[0], I find it bad as it is not actually mapping to a codec type. It should be a seperate gst_default_opts for example.
In case of this series, I think we should expose an array of GObject Streams, with readable properties. If array is empty, there is no stream. This is would let use extend it with further informations when needed too.
But I wonder if this property in a public API is really that useful, we have should have enough informations in the debug log.
> > Imho, that needs more fixing. I think we should revert your series
> > for now.
> >
> > > > >
> > > > > >
> > > > > > > For SPICE_VIDEO_CODEC_TYPE_ENUM_END before an "error" was
> > > > > > > print,
> > > > > > > now "unknown codec". Again, fo me is fine.
> > > > > > >
> > > > > > > > + [SPICE_VIDEO_CODEC_TYPE_MJPEG] = "mjpeg",
> > > > > > > > + [SPICE_VIDEO_CODEC_TYPE_VP8] = "vp8",
> > > > > > > > + [SPICE_VIDEO_CODEC_TYPE_H264] = "h264",
> > > > > > > > + [SPICE_VIDEO_CODEC_TYPE_VP9] = "vp9",
> > > > > > > > + };
> > > > > > > > +
> > > > > > > > + if (type >= 0 && type < G_N_ELEMENTS(to_string)) {
> > > > > > > > + str = to_string[type];
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + return str ? str : "unknown codec";
> > > > > > > > +}
> > > > > > > >
> > > > > > > > static void update_status_window(SpiceWindow *win)
> > > > > > > > {
> > > > > > > > @@ -201,7 +211,7 @@ static void
> > > > > > > > update_status_window(SpiceWindow
> > > > > > > > *win)
> > > > > > > > g_string_printf(status, "mouse: %6s, agent: %3s,
> > > > > > > > streaming:
> > > > > > > > %5s",
> > > > > > > > win->conn->mouse_state,
> > > > > > > > win->conn->agent_state,
> > > > > > > > - video_codec_enum_to_str[win-
> > > > > > > > >video_codec]);
> > > > > > > > + video_codec_to_string(win-
> > > > > > > > >video_codec));
> > > > > > > >
> > > > > > > > if (win->mouse_grabbed) {
> > > > > > > > SpiceGrabSequence *sequence =
> > > > > > > > spice_display_get_grab_keys(SPICE_DISPLAY(win-
> > > > > > > > >spice));
> > > > > > >
> > > > > > > Reviewed-by: Frediano Ziglio <fziglio at redhat.com>
> > > > > > >
> > > > > > > Frediano
> > > > > > > _______________________________________________
> > > > > > > Spice-devel mailing list
> > > > > > > Spice-devel at lists.freedesktop.org
> > > > > > > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> >
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/spice-devel
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
More information about the Spice-devel
mailing list