[vlc-devel] [PATCH] lib: media_track: don't return an empty list

[Marvin Scholz]

Hi,
I am not sure it's a good idea to have no way to tell the error case
apart from the case when there are no items, as some applications
might want to handle that differently?
Opinions welcome, maybe it's just me.

Other minor nitpicks follow inline:

On 16 Jun 2020, at 12:30, Thomas Guillem wrote:

> This was done to make the distinction between errors (NULL) and 0 
> tracks (empty
> list). Making this distinction is quite useless, specially since only 
> alloc
> errors can occur.
> ---
>  include/vlc/libvlc_media.h        |  6 +++---
>  include/vlc/libvlc_media_player.h |  6 +++---
>  lib/media_track.c                 | 14 +++++++++-----
>  test/libvlc/media.c               |  3 ++-
>  4 files changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/include/vlc/libvlc_media.h b/include/vlc/libvlc_media.h
> index d7ae42bb058..cc953c33f57 100644
> --- a/include/vlc/libvlc_media.h
> +++ b/include/vlc/libvlc_media.h
> @@ -677,9 +677,9 @@ unsigned libvlc_media_tracks_get( libvlc_media_t 
> *p_md,
>   * \param p_md media descriptor object
>   * \param type type of the track list to request
>   *
> - * \return a valid libvlc_media_tracklist_t or NULL in case of error, 
> if there
> - * is no track for a category, the returned list will have a size of 
> 0, delete
> - * with libvlc_media_tracklist_delete()
> + * \return a valid libvlc_media_tracklist_t or NULL if there is no 
> tracks for

nit: "if there are no tracks"

> + * that category, or in case of error, delete with
> + * libvlc_media_tracklist_delete()

Should probably have a full stop after error, else it makes it
read like one should libvlc_media_tracklist_delete on error
which makes no sense.

>   */
>  LIBVLC_API libvlc_media_tracklist_t *
>  libvlc_media_get_tracklist( libvlc_media_t *p_md, libvlc_track_type_t 
> type );
> diff --git a/include/vlc/libvlc_media_player.h 
> b/include/vlc/libvlc_media_player.h
> index e861cd5fca7..a67d243426d 100644
> --- a/include/vlc/libvlc_media_player.h
> +++ b/include/vlc/libvlc_media_player.h
> @@ -1304,9 +1304,9 @@ LIBVLC_API void 
> libvlc_media_player_set_video_title_display( libvlc_media_player
>   * \param p_mi the media player
>   * \param type type of the track list to request
>   *
> - * \return a valid libvlc_media_tracklist_t or NULL in case of error, 
> if there
> - * is no track for a category, the returned list will have a size of 
> 0, delete
> - * with libvlc_media_tracklist_delete()
> + * \return a valid libvlc_media_tracklist_t or NULL if there is no 
> tracks for
> + * that category, or in case of error, delete with
> + * libvlc_media_tracklist_delete()

same remarks as above

>   */
>  LIBVLC_API libvlc_media_tracklist_t *
>  libvlc_media_player_get_tracklist( libvlc_media_player_t *p_mi,
> diff --git a/lib/media_track.c b/lib/media_track.c
> index a11c2973c65..76ad34e31d3 100644
> --- a/lib/media_track.c
> +++ b/lib/media_track.c
> @@ -199,10 +199,12 @@ libvlc_media_tracklist_from_es_array( 
> es_format_t **es_array,
>              count++;
>      }
>
> -    libvlc_media_tracklist_t *list = libvlc_media_tracklist_alloc( 
> count );
> +    if( count == 0 )
> +        return NULL;
>
> -    if( count == 0 || list == NULL )
> -        return list;
> +    libvlc_media_tracklist_t *list = libvlc_media_tracklist_alloc( 
> count );
> +    if( list == NULL )
> +        return NULL;
>
>      for( size_t i = 0; i < es_count; ++i )
>      {
> @@ -253,9 +255,11 @@ libvlc_media_tracklist_from_player( vlc_player_t 
> *player,
>      const enum es_format_category_e cat = libvlc_track_type_to_escat( 
> type );
>
>      size_t count = vlc_player_GetTrackCount( player, cat );
> -    libvlc_media_tracklist_t *list = libvlc_media_tracklist_alloc( 
> count );
> +    if( count == 0 )
> +        return NULL;
>
> -    if( count == 0 || list == NULL )
> +    libvlc_media_tracklist_t *list = libvlc_media_tracklist_alloc( 
> count );
> +    if( list == NULL )
>          return NULL;
>
>      for( size_t i = 0; i < count; ++i )
> diff --git a/test/libvlc/media.c b/test/libvlc/media.c
> index 65673c855ff..4442dd36422 100644
> --- a/test/libvlc/media.c
> +++ b/test/libvlc/media.c
> @@ -51,7 +51,8 @@ static void print_media(libvlc_media_t *media)
>          const libvlc_track_type_t type = types[i];
>          libvlc_media_tracklist_t *tracklist =
>              libvlc_media_get_tracklist(media, type);
> -        assert(tracklist);
> +        if (tracklist == NULL)
> +            continue;
>
>          for (size_t j = 0; j < 
> libvlc_media_tracklist_count(tracklist); ++j)
>          {
> -- 
> 2.20.1
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel