[Spice-devel] [PATCH spice-gtk] spicy: make codec to string a bit safer
Victor Toso
victortoso at redhat.com
Mon Mar 13 12:18:51 UTC 2017
Hi,
On Mon, Mar 13, 2017 at 08:02:36AM -0400, Marc-André Lureau wrote:
> 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.
Let's revert. I don't see the need to export GObjecs to query its
properties to know the video-codec type so, debug should do.
> > > 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
> >
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170313/2dc8d793/attachment.sig>
More information about the Spice-devel
mailing list