[vlc-devel] [PATCH] libvlc: expand media player API to retrieve full information about available chapter of a given title

[Felix Paul Kühne]

Hello Rémi,

The patch in question was merged on June 9 after 5 days without feedback. I’ll make the changes you requested in a subsequent commit this week.

> On 12 Jun 2015, at 21:29, Rémi Denis-Courmont <remi at remlab.net> wrote:
> 
> Summary description is too long.

Sorry, I can’t fix that one anymore.

> Le jeudi 04 juin 2015, 14:08:38 Felix Paul Kühne a écrit :
>> ---
>> include/vlc/libvlc_media_player.h | 40 ++++++++++++++++++++
>> include/vlc_input.h               |  3 ++
>> lib/libvlc.sym                    |  2 +
>> lib/media_player.c                | 78
>> +++++++++++++++++++++++++++++++++++++++ src/input/control.c               |
>> 44 ++++++++++++++++++++++
>> 5 files changed, 167 insertions(+)
>> 
>> diff --git a/include/vlc/libvlc_media_player.h
>> b/include/vlc/libvlc_media_player.h index 8bb7c29..48ce461 100644
>> --- a/include/vlc/libvlc_media_player.h
>> +++ b/include/vlc/libvlc_media_player.h
>> @@ -73,6 +73,17 @@ typedef struct libvlc_title_description_t
>> } libvlc_title_description_t;
>> 
>> /**
>> + * Description for chapters.
>> + * It contains information about time set as well as
>> + * name (description string).
> 
> This is incomplete documentation (unit?), and documented the fields in Doxygen 
> would be better.

The unit is correctly documented in the pushed variant. I’ll doxygenise the fields.

>> + */
>> +typedef struct libvlc_chapter_description_t
>> +{
>> +    int64_t i_time_offset;
>> +    char *psz_name;
>> +} libvlc_chapter_description_t;
>> +
>> +/**
>>  * Description for audio output. It contains
>>  * name, description and pointer to next record.
>>  */
>> @@ -1170,6 +1181,35 @@ LIBVLC_API
>>                                             unsigned i_count );
>> 
>> /**
>> + * Get the full description of available chapters
>> + *
>> + * \version LibVLC 3.0.0 and later.
>> + *
>> + * \param p_mi the media player
>> + * \param index of the title to query for chapters
>> + * \param address to store an allocated array of chapter descriptions
>> + *        descriptions (must be freed with
>> libvlc_chapter_descriptions_release
> 
> Missing () for Doxygen.

This is fixed in the pushed variant.

>> +
>> +    /* fill array */
>> +    for( int i = 0; i < ci_chapter_count; i++)
>> +    {
>> +        libvlc_chapter_description_t *p_chapter = calloc( 1,
>> sizeof(*p_chapter) );
> 
> Why calloc?

Mainly to match similar implementations in the same file. I can switch to malloc if you prefer.

>> +        if( unlikely(p_chapter == NULL) )
>> +        {
>> +            libvlc_chapter_descriptions_release( *pp_chapters,
>> ci_chapter_count );
>> +            free( p_chapter );
> 
> ??

ehm, will fix :D

>> +        case INPUT_GET_SEEKPOINTS:
>> +        {
>> +            seekpoint_t ***array = (seekpoint_t ***)va_arg( args,
>> seekpoint_t *** );
>> +            int *pi_title_to_fetch = (int *) va_arg(
>> args, int * );
> 
> Useless casts.

Idem.

>> +            *array = calloc( p_title->i_seekpoint, sizeof(**array) );
>> +            if( !array )
> 
> Wrong and missing unlikely().

Idem.


Thanks for the thorough review!

Cheers,

Felix