[vlc-devel] [Patch] Correctly fill out input_item's es info, and slim down input_item_es_t.

[Laurent Aimar]

Hi,

On Mon, Feb 22, 2010, Pierre d'Herbemont wrote:
>    Hey,
>    Please review the attached patch.
>    I consider it important because it properly fix libvlc_media_get_es(). We
>    are currently advising our poor user to create a new media_player with
>    --sout=#description which makes us look overcomplicated. Mostly because
>    you still have to attach an event and such to know when we are done, and
>    also because a full playback is launched, not saving CPU and memory.
>    Our alternative to this patch is to read the input_item's info dictionary.
>    And parse the string in it. Hence the modification stay in libvlc umbrella
>    and not in the core.
>    However I consider this patch as low risk:
>    - It is following how we fill the input_item's info which is an already
>    existing string dictionary.
>    - Its impact in term of performance is low. We are not using a full
>    es_format_t, but just a stripped down version of fixed size, which is what
>    we expose in libvlc_media.
>    - It's chirugical and circumvented.
>    - It's fixing a real issue in libvlc.
 Well, it's adding a feature not fixing an issue (I don't say that it's not a
wanted one).

>    If it's not accepted I'll fix libvlc_media_get_es() in 1.1 by parsing
>    input_item's info field, which are correctly filled after preparsing for
>    most demuxer. I am ok with this, but wanted to get the patch archived on
>    the ML at least.
Except the following remarks, I am ok with the way it is implemented.

> diff --git a/include/vlc_input_item.h b/include/vlc_input_item.h
> index d4858c9..1e10acd 100644
> --- a/include/vlc_input_item.h
> +++ b/include/vlc_input_item.h
> @@ -51,6 +51,44 @@ struct info_category_t
>      struct info_t **pp_infos;     /**< Pointer to an array of infos */
>  };
>  
> +/* Stripped down version of es_format_t because es_format_t can become very
> + * large in term of memory. This is mostly libvlc_media_es_t */
> +typedef struct input_item_es_t {
 Usually useless to name the struct part.
> +    vlc_fourcc_t i_codec;
> +    int          i_id;
> +    int          i_cat; /* @see es_format_category_e */
> +
> +    /* Codec specific */
> +    int         i_profile;
> +    int         i_level;
> +
> +    /* Audio specific */
> +    unsigned    i_channels;
> +    unsigned    i_rate;
> +
> +    /* Video specific */
> +    unsigned    i_height;
> +    unsigned    i_width;
 A union between video and audio type will save memory and allows to know
which fields cannot be used together, maybe somehting like:
union
{
   struct
   {
      unsigned i_width;
      unsigned i_height;
   } video;
   struct
   {
      unsigned i_channels;
      unsigned i_rate;
   } audio;
} u;

> +} input_item_es_t;

> +
> +static inline input_item_es_t *input_item_es_new_from_format(const es_format_t *fmt)
input_item_es_NewFromFormat to be consistant.
> +{
> +    input_item_es_t *ret = (input_item_es_t *)malloc(sizeof(input_item_es_t));
malloc(*ret) is safer.
> +    ret->i_codec = fmt->i_codec;
> +    ret->i_id = fmt->i_id;
> +    ret->i_cat = fmt->i_cat;
> +
> +    ret->i_profile = fmt->i_profile;
> +    ret->i_level = fmt->i_level;
> +
> +    ret->i_channels = fmt->audio.i_channels;
> +    ret->i_rate = fmt->audio.i_rate;
> +
> +    ret->i_height = fmt->video.i_height;
> +    ret->i_width = fmt->video.i_width;
> +    return ret;
> +}
 I think the function should be moved to src/input/item.c as it is not speed critical.

>  struct input_item_t
>  {
>      VLC_GC_MEMBERS
> diff --git a/src/input/es_out.c b/src/input/es_out.c
> index 3062d8e..c2fb21d 100644
> --- a/src/input/es_out.c
> +++ b/src/input/es_out.c
> @@ -2223,7 +2223,7 @@ static int EsOutControlLocked( es_out_t *out, int i_query, va_list args )
>              }
>              return VLC_SUCCESS;
>          }
> - 
> +
Cosmetic.

>          case ES_OUT_SET_ES_DEFAULT:
>          {
>              es_out_id_t *es = va_arg( args, es_out_id_t * );
> @@ -2811,6 +2811,31 @@ static void EsOutUpdateInfo( es_out_t *out, es_out_id_t *es, const es_format_t *
>      const es_format_t *p_fmt_es = &es->fmt;
>      lldiv_t         div;
>  
> +    /* Update input item */
> +    input_item_t *item = p_input->p->p_item;
 I would prefer input_GetItem() as I plan to remove as much as p_input->p accesses as possible.

> +    vlc_mutex_lock( &item->lock );
> +    bool found = false;
> +
> +    input_item_es_t *input_item_fmt = input_item_es_new_from_format(fmt);
> +
> +    for( int i = 0; i < item->i_es; i++ )
> +    {
> +        if (item->es[i]->i_id == es->i_id)
> +        {
> +            /* We've found the right ES, replace it */
> +            free(item->es[i]);
> +            item->es[i] = input_item_fmt;
> +            found = true;
> +            break;
> +        }
> +    }
> +    if( !found )
> +    {
> +        /* ES not found, create a new one. */
> +        TAB_APPEND(item->i_es, item->es, input_item_fmt);
> +    }
> +    vlc_mutex_unlock( &item->lock );
> +
 Creating an helper (input_item_UpdateEs or something like that) in item.c is
cleaner than putting the code in es_out.
 Also, current item->es is not of the right type as it is a es_format_t (I haven't
saw any change to it). Changing the type of item->es will make #describe unusable,
so either we need to add new fields to input_item_t or breaks the VLM VoD part.
 Btw, you also need to clean up the array when destructing the input_item_t.

Regards,

-- 
fenrir