Re: [spice-gtk v7 3/3] spicy: implement preferred video codec type

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On Mon, Feb 06, 2017 at 09:52:56PM +0100, Victor Toso wrote:
> Hi,
>
> On Mon, Feb 06, 2017 at 06:32:21PM +0100, Pavel Grunt wrote:
> > On Mon, 2017-02-06 at 17:52 +0100, Pavel Grunt wrote:
> > > On Mon, 2017-02-06 at 12:06 +0100, Victor Toso wrote:
> > > > From: Victor Toso <me@xxxxxxxxxxxxxx>
> > > > 
> > > > Similar to preferred video compression, a radio button showing
> > > > mjpeg,
> > > > vp8, vp9 and h264 in case server has the proper [0] capability
> > > > 
> > > > [0] SPICE_DISPLAY_CAP_PREF_VIDEO_CODEC_TYPE
> > > > 
> > > > Signed-off-by: Victor Toso <victortoso@xxxxxxxxxx>
> > > > ---
> > > >  tools/spicy.c | 64
> > > > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 64 insertions(+)
> > > > 
> > > > diff --git a/tools/spicy.c b/tools/spicy.c
> > > > index c502428..ce758a7 100644
> > > > --- a/tools/spicy.c
> > > > +++ b/tools/spicy.c
> > > > @@ -651,6 +651,9 @@ static const GtkActionEntry entries[] = {
> > > >          .name        = "CompressionMenu",
> > > >          .label       = "_Preferred image compression",
> > > >      },{
> > > > +        .name        = "VideoCodecTypeMenu",
> > > > +        .label       = "_Preferred video codec type",
> > > > +    },{
> > > >          .name        = "HelpMenu",
> > > >          .label       = "_Help",
> > > >      },{
> > > > @@ -816,6 +819,26 @@ static const GtkRadioActionEntry
> > > > compression_entries[] = {
> > > >      }
> > > >  };
> > > >  
> > > > +static const GtkRadioActionEntry video_codec_type_entries[] = {
> > > > +    {
> > > > +        .name  = "mjpeg",
> > > > +        .label = "mjpeg",
> > > > +        .value = SPICE_VIDEO_CODEC_TYPE_MJPEG,
> > > 
> > > if we had some structure like Frediano suggested, we could reuse it
> > > here
> > > > +    },{
> > > > +        .name  = "vp8",
> > > > +        .label = "vp8",
> > > > +        .value = SPICE_VIDEO_CODEC_TYPE_VP8,
> > > > +    },{
> > > > +        .name  = "vp9",
> > > > +        .label = "vp9",
> > > > +        .value = SPICE_VIDEO_CODEC_TYPE_VP9,
> > > > +    },{
> > > > +        .name  = "h264",
> > > > +        .label = "h264",
> > > > +        .value = SPICE_VIDEO_CODEC_TYPE_H264,
> > > > +    }
> > > > +};
> > > > +
> > > >  static char ui_xml[] =
> > > >  "<ui>\n"
> > > >  "  <menubar action='MainMenu'>\n"
> > > > @@ -864,6 +887,12 @@ static char ui_xml[] =
> > > >  #endif
> > > >  "        <menuitem action='off'/>\n"
> > > >  "      </menu>\n"
> > > > +"      <menu action='VideoCodecTypeMenu'>\n"
> > > > +"        <menuitem action='mjpeg'/>\n"
> > > > +"        <menuitem action='vp8'/>\n"
> > > > +"        <menuitem action='vp9'/>\n"
> > > > +"        <menuitem action='h264'/>\n"
> > > > +"      </menu>\n"
> > > >  "    </menu>\n"
> > > >  "    <menu action='HelpMenu'>\n"
> > > >  "      <menuitem action='About'/>\n"
> > > > @@ -916,6 +945,14 @@ static void compression_cb(GtkRadioAction
> > > > *action G_GNUC_UNUSED,
> > > >                                                 gtk_radio_action_g
> > > > et
> > > > _current_value(current));
> > > >  }
> > > >  
> > > > +static void video_codec_type_cb(GtkRadioAction *action
> > > > G_GNUC_UNUSED,
> > > > +                                GtkRadioAction *current,
> > > > +                                gpointer user_data)
> > > > +{
> > > > +    spice_display_change_preferred_video_codec_type(SPICE_CHANNEL
> > > > (u
> > > > ser_data),
> > > > +                                                    gtk_radio_act
> > > > io
> > > > n_get_current_value(current));
> > > > +}
> > > > +
> > > >  static void
> > > >  spice_window_class_init (SpiceWindowClass *klass)
> > > >  {
> > > > @@ -970,6 +1007,33 @@ static SpiceWindow
> > > > *create_spice_window(spice_connection *conn, SpiceChannel *ch
> > > >          GtkAction *compression_menu_action =
> > > > gtk_action_group_get_action(win->ag, "CompressionMenu");
> > > >          gtk_action_set_sensitive(compression_menu_action, FALSE);
> > > >      }
> > > > +    gtk_action_group_add_radio_actions(win->ag,
> > > > video_codec_type_entries,
> > > > +                                       G_N_ELEMENTS(video_codec_t
> > > > yp
> > > > e_entries), -1,
> > > > +                                       G_CALLBACK(video_codec_typ
> > > > e_
> > > > cb), win->display_channel);
> > > > +    if (!spice_channel_test_capability(win->display_channel,
> > > > +                                       SPICE_DISPLAY_CAP_PREF_VID
> > > > EO
> > > > _CODEC_TYPE)) {
> > > > +        GtkAction *video_codec_type_menu_action =
> > > > +            gtk_action_group_get_action(win->ag,
> > > > "VideoCodecTypeMenu");
> > > > +        gtk_action_set_sensitive(video_codec_type_menu_action,
> > > > FALSE);
> > > > +    } else {
> > > > +        gint i;
> > > 
> > > it should be unsigne
> > > > +        static const struct {
> > > > +            gint cap;
> > > > +            const gchar name[8];
> > > > +        } display_codecs[] = {
> > > > +            {SPICE_DISPLAY_CAP_CODEC_MJPEG, "mjpeg"},
> > > > +            {SPICE_DISPLAY_CAP_CODEC_VP8, "vp8"},
> > > > +            {SPICE_DISPLAY_CAP_CODEC_H264, "h264"},
> > > > +            {SPICE_DISPLAY_CAP_CODEC_VP9, "vp9"},
> > > > +        };
> > > > +        for (i = 0; i < G_N_ELEMENTS(display_codecs); i++) {
> > > > +            if (!spice_channel_test_capability(win-
> > > > > display_channel, display_codecs[i].cap)) {
> > > 
> > > isn't the condition wrong ?
> > > 
> > > The menu is gray all the time for me...
> > the gray menu is another issue, the problem is that
> > spice_channel_test_capability() tests remote caps (c->remote_caps) but
> > spicy is interested in local caps (c->caps)
>
> Aha. It should check both, remote and local caps. Nice catch :)

Never mind. SPICE_DISPLAY_CAP_CODEC_{MJPEG VP8 VP9 H264} is for
client side only. That's a little confusing, server could be unable to
do SPICE_DISPLAY_CAP_MULTI_CODEC or encode in VP8/VP9/H264 but it does
not need to explicit say it.

> Also, it seems that spice does not check the encoding elements like
> spice-gtk does with the decoding ones. I've manually removed from the
> path vp8 & vp9 plugins and spicy submenu does not goes gray. Spice-gtk
> knows that it can't find the elements...

So, it doesn't really need to check it til it tries to create the
pipeline. If it can't create a stream in VP9 it will simply try the next
one in the list.

> I'll try to fix spice in a separated patch and improve this check in
> spicy.

I'll fix spicy check and do some more testing before sending it again.

  toso
>
> Let me know if there is anything else in the rest of the patches as this
> changes are becoming minor. Once again, thanks for the review.
> 
> > 
> > > 
> > > Pavel
> > > 
> > > > +                GtkAction *action =
> > > > gtk_action_group_get_action(win->ag, display_codecs[i].name);
> > > > +                gtk_action_set_sensitive(action, TRUE);
> > > > +            }
> > > > +        }
> > > > +    }
> > > > +
> > > >      gtk_ui_manager_insert_action_group(win->ui, win->ag, 0);
> > > >      gtk_window_add_accel_group(GTK_WINDOW(win->toplevel),
> > > >                                 gtk_ui_manager_get_accel_group(win
> > > > -
> > > > > ui));
> > > 
> > > 


Attachment: signature.asc
Description: PGP signature

_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel

[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]