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

[Laurent Aimar]

Hi,

On Thu, Feb 25, 2010, Pierre d'Herbemont wrote:
>    See the updated version attached to that email. Most concern should be
>    addressed. With this patch test/libvlc/media.c pass.
>    If we choose not to apply we should probably do something like:
>    1. Work around in test/libvlc/media.c and properly document it. (hard)
>    2. Work around in the libvlc API implementation. (hard)
>    3. Remove the test and remove the API. (easy)
>    4. Nothing. (more than easy)

> +/* 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_track_info_t
> +{
> +    vlc_fourcc_t i_codec;
> +    int          i_id;
> +    int          i_cat; /* @see es_format_category_e */
> +
> +    /* Codec specific */
> +    int         i_profile;
> +    int         i_level;
> +
> +    union
> +    {
> +        struct
> +        {
> +            /* Video specific */
> +            unsigned    i_height;
> +            unsigned    i_width;
> +        } video;
> +        struct
> +        {
> +            /* Audio specific */
> +            unsigned    i_channels;
> +            unsigned    i_rate;
> +        } audio;
> +    } u;
> +} input_item_track_info_t;
 I don't see the point of the _info_ part.
input_item_track_t seems way better to me.

>  struct input_item_t
>  {
>      VLC_GC_MEMBERS
> @@ -73,6 +102,9 @@ struct input_item_t
>      int         i_es;                /**< Number of es format descriptions */
>      es_format_t **es;                /**< Es formats */
>  
> +    int i_tracks_info;               /**< Number of track info descriptions */
> +    input_item_track_info_t **tracks_info;/**< Tracks Info */
Same here (and in the following).

> +/*
> + * Item tracks_info
> + */
> +static input_item_track_info_t *track_info_new_from_format(const es_format_t *fmt)
input_item_track_NewFromFormat to be coherent.
> +{
> +    input_item_track_info_t *ret = malloc(sizeof(input_item_track_info_t));
malloc(sizeof(*ret)) to be safer.

As a rule, IMHO it should be
 type_t *t = malloc(sizeof(*t));
or
 type_t *t = (type_t*)malloc(sizeof(type_t)); (c++)
to be sure to avoid any typo.

> +    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->u.audio.i_channels = fmt->audio.i_channels;
> +    ret->u.audio.i_rate = fmt->audio.i_rate;
> +
> +    ret->u.video.i_height = fmt->video.i_height;
> +    ret->u.video.i_width = fmt->video.i_width;
You must switch on fmt->i_cat.

> +/* Called by es_out when a new Elementary Stream is added or updated. */
> +void input_item_UpdateTracksInfo(input_item_t *item, const es_format_t *fmt)
> +{
> +    vlc_mutex_lock( &item->lock );
> +    bool found = false;
> +
> +    input_item_track_info_t *track_info = track_info_new_from_format(fmt);
> +
> +    for( int i = 0; i < item->i_tracks_info; i++ )
> +    {
> +        if (item->tracks_info[i]->i_id == track_info->i_id)
> +        {
> +            /* We've found the right ES, replace it */
> +            free(item->tracks_info[i]);
> +            item->tracks_info[i] = track_info;
> +            found = true;
Unlocking and returning (or a goto) will probably be more readable than
using the 'found' variable.

> +            break;
> +        }
> +    }
> +    if( !found )
> +    {
> +        /* ES not found, create a new one. */
> +        TAB_APPEND(item->i_tracks_info, item->tracks_info, track_info);
> +    }
> +    vlc_mutex_unlock( &item->lock );
> +}

 Adding a input_item_track_Delete() and using it everywhere it is applicable
will ease thing when/if we need to add allocated fields.

-- 
fenrir