[vlc-devel] [PATCH v2] Add support for GoPro HiLight tags as chapters, so they can appear in the progress bar

[Jean-Baptiste Kempf]

On 08 Nov, Emeric Grange wrote :
> +/* GoPro HiLight tags support */
> +static void MP4_FreeBox_hmmt( MP4_Box_t *p_box )
> +{
> +    MP4_Box_data_HMMT_t *p_hmmt = p_box->data.p_hmmt;
> +    for( unsigned i = 0; i < p_hmmt->i_chapter_count; i++ )
> +        if ( p_hmmt->chapter[i].psz_name )
> +            free( p_hmmt->chapter[i].psz_name );
> +}

free( NULL ) is valid, I believe.

> +static int MP4_ReadBox_HMMT( stream_t *p_stream, MP4_Box_t *p_box )
> +{
> +    MP4_Box_data_HMMT_t *p_hmmt;
> +
> +    MP4_READBOX_ENTER( MP4_Box_data_HMMT_t, MP4_FreeBox_hmmt );
> +
> +    if ( i_read < 5 )
> +        MP4_READBOX_EXIT( 0 );
> +
> +    p_hmmt = p_box->data.p_hmmt;
> +
> +    MP4_GET4BYTES( p_hmmt->i_chapter_count );
> +
> +    if (p_hmmt->i_chapter_count <= 100)
> +    {
> +        for( uint32_t i = 0; i < p_hmmt->i_chapter_count; i++ )
> +        {
> +            MP4_GET4BYTES( p_hmmt->chapter[i].i_start );

You need to check here that you are not outside of the box, a box could
declare 45 chapters and only have 4 actual defined :)

You should have a look at MP4_ReadBox_stts or something similar also to
set the count limit, somehow like this:
if ( i < p_box->data.p_stts->i_entry_count )
       p_box->data.p_stts->i_entry_count = i; 

> +
> +            asprintf( &p_hmmt->chapter[i].psz_name, "HiLight tag #%u", i+1 );

Do you need to do it here?

> +        }
> +
> +        msg_Dbg( p_stream, "read box: \"HMMT\" %d HiLight tags",
> +                           p_hmmt->i_chapter_count );
> +    }
> +
> +    MP4_READBOX_EXIT( 1 );
> +}
> +
>  static void MP4_FreeBox_tref_generic( MP4_Box_t *p_box )
>  {
>      FREENULL( p_box->data.p_tref_generic->i_track_ID );
> @@ -3915,6 +3953,7 @@ static const struct
>      { ATOM_name,    MP4_ReadBox_String,    ATOM_udta },
>      { ATOM_vndr,    MP4_ReadBox_String,    ATOM_udta },
>      { ATOM_SDLN,    MP4_ReadBox_String,    ATOM_udta },
> +    { ATOM_HMMT,    MP4_ReadBox_HMMT,      ATOM_udta }, /* GoPro HiLights */
>  
>      /* udta, non meta */
>      { ATOM_tsel,    MP4_ReadBox_tsel,    ATOM_udta },
> diff --git a/modules/demux/mp4/libmp4.h b/modules/demux/mp4/libmp4.h
> index d066e2a..ff98e5d 100644
> --- a/modules/demux/mp4/libmp4.h
> +++ b/modules/demux/mp4/libmp4.h
> @@ -315,6 +315,7 @@ typedef int64_t stime_t;
>  #define ATOM_0xa9xyz VLC_FOURCC( 0xa9, 'x', 'y', 'z' )
>  #define ATOM_aART VLC_FOURCC( 'a', 'A', 'R', 'T' )
>  #define ATOM_chpl VLC_FOURCC( 'c', 'h', 'p', 'l' )
> +#define ATOM_HMMT VLC_FOURCC( 'H', 'M', 'M', 'T' )
>  #define ATOM_disk VLC_FOURCC( 'd', 'i', 's', 'k' )
>  #define ATOM_WLOC VLC_FOURCC( 'W', 'L', 'O', 'C' )
>  
> @@ -1104,6 +1105,18 @@ typedef struct
>  
>  typedef struct
>  {
> +    uint32_t i_chapter_count;
> +
> +    struct
> +    {
> +        char    *psz_name;
> +        uint32_t i_start;
> +    } chapter[100];

btw, why 100 ?

> +
> +} MP4_Box_data_HMMT_t;
> +
> +typedef struct
> +{
>      uint8_t i_version;
>      uint8_t i_profile;
>      uint8_t i_profile_compatibility;
> @@ -1470,6 +1483,7 @@ typedef union MP4_Box_data_s
>  
>      MP4_Box_data_pnot_t *p_pnot;
>      MP4_Box_data_chpl_t *p_chpl;
> +    MP4_Box_data_HMMT_t *p_hmmt;
>      MP4_Box_data_tref_generic_t *p_tref_generic;
>  
>      MP4_Box_data_tfrf_t *p_tfrf;
> diff --git a/modules/demux/mp4/mp4.c b/modules/demux/mp4/mp4.c
> index 3175ed3..480dee4 100644
> --- a/modules/demux/mp4/mp4.c
> +++ b/modules/demux/mp4/mp4.c
> @@ -1681,6 +1681,24 @@ static void LoadChapterGpac( demux_t  *p_demux, MP4_Box_t *p_chpl )
>          TAB_APPEND( p_sys->p_title->i_seekpoint, p_sys->p_title->seekpoint, s );
>      }
>  }
> +static void LoadChapterGoPro( demux_t *p_demux, MP4_Box_t *p_hmmt )
> +{
> +    demux_sys_t *p_sys = p_demux->p_sys;
> +
> +    p_sys->p_title = vlc_input_title_New();
> +    for( unsigned i = 0; i < BOXDATA(p_hmmt)->i_chapter_count; i++ )
> +    {
> +        seekpoint_t *s = vlc_seekpoint_New();
> +        if (s)
> +        {
> +            s->psz_name = strdup( BOXDATA(p_hmmt)->chapter[i].psz_name );

Maybe you should do the asprintf here, no?
So you wouldn't need to store the psz_name in the chapter struct and
strdup it for every chapter?

> +            EnsureUTF8( s->psz_name );
> +            // HiLights are expressed in ms, need convertion to µs
> +            s->i_time_offset = BOXDATA(p_hmmt)->chapter[i].i_start * 1000;
> +            TAB_APPEND( p_sys->p_title->i_seekpoint, p_sys->p_title->seekpoint, s );
> +        }
> +    }
> +}


>  static void LoadChapterApple( demux_t  *p_demux, mp4_track_t *tk )
>  {
>      demux_sys_t *p_sys = p_demux->p_sys;
> @@ -1724,12 +1742,18 @@ static void LoadChapter( demux_t  *p_demux )
>  {
>      demux_sys_t *p_sys = p_demux->p_sys;
>      MP4_Box_t *p_chpl;
> +    MP4_Box_t *p_hmmt;
>  
>      if( ( p_chpl = MP4_BoxGet( p_sys->p_root, "/moov/udta/chpl" ) ) &&
>            BOXDATA(p_chpl) && BOXDATA(p_chpl)->i_chapter > 0 )
>      {
>          LoadChapterGpac( p_demux, p_chpl );
>      }
> +    else if( ( p_hmmt = MP4_BoxGet( p_sys->p_root, "/moov/udta/HMMT" ) ) &&
> +             BOXDATA(p_hmmt) && BOXDATA(p_hmmt)->i_chapter_count > 0 )
> +    {
> +        LoadChapterGoPro( p_demux, p_hmmt );
> +    }
>      else if( p_sys->p_tref_chap )
>      {
>          MP4_Box_data_tref_generic_t *p_chap = p_sys->p_tref_chap->data.p_tref_generic;
> -- 
> 2.6.2
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel

-- 
With my kindest regards,

-- 
Jean-Baptiste Kempf
http://www.jbkempf.com/ - +33 672 704 734
Sent from my Electronic Device