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

[Romain Vimont]

On Tue, Jun 16, 2020 at 12:44:41PM +0200, Marvin Scholz wrote:
> 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.

I agree, the caller should be able to distinguish between alloc error
and empty list.

> 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
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel