[Spice-devel] [PATCH spice-gtk v3 1/6] display-gst: check codec type before creating decoder

[Victor Toso]

Hi,

On Tue, Jun 06, 2017 at 04:40:33PM +0200, Christophe Fergeau wrote:
> On Tue, May 16, 2017 at 04:48:13PM +0200, Victor Toso wrote:
> > From: Victor Toso <me at victortoso.com>
> >
> > Inserting this check in channel-display-gst.c as the GStreamer decoder
> > is the only one handling all the different video formats supported by
> > spice-protocol.
> >
> > If a unsupported/bad codec type value was sent, spice-gtk will fail to
> > create the decoder and any messages related to this stream-id will be
> > ignored.
> >
> > Signed-off-by: Victor Toso <victortoso at redhat.com>
> > Signed-off-by: Victor Toso <me at victortoso.com>
> > ---
> >  src/channel-display-gst.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/src/channel-display-gst.c b/src/channel-display-gst.c
> > index 9b79403..d3e83e3 100644
> > --- a/src/channel-display-gst.c
> > +++ b/src/channel-display-gst.c
> > @@ -512,6 +512,8 @@ VideoDecoder* create_gstreamer_decoder(int codec_type, display_stream *stream)
> >  {
> >      SpiceGstDecoder *decoder = NULL;
> >
> > +    g_return_val_if_fail(VALID_VIDEO_CODEC_TYPE(codec_type), NULL);
> > +
>
> g_return_val_if_fail() is usually used to detect/report programming
> erros, ie situations that are not expected to happen if the program
> works as expected by the developer.

I think this is the case. Spice should not try to create a stream with a
unsupported video-codec

> In this case, it seems the user could trigger this warning by sending
> an invalid codec type in a SpiceMsgDisplayStreamCreate message?

Wouldn't that be a bug? As client has capabilities to explicit say to
Spice which video codecs it can handle Spice shouldn't try to create a
video stream with unsupported video codec.

> If that's correct, I would use a regular test:
>
> if (!VALID_VIDEO_CODEC_TYPE(codec_type)) {
>     g_warning(...);
>     return NULL;
> }
>
> Christophe

I don't mind changing it if I got the rationale wrong :)

Cheers,
    toso
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20170608/5f3c3d7a/attachment.sig>