[vlc-devel] [PATCH] Implement basic Opus support in MKV

[Rafaël Carré]

Hello,

patch looks OK, thank you.

I just have some small nitpicks

Le 09/11/2013 17:16, Denis Charmet a écrit :
> ---
>  modules/codec/opus.c                         |  5 ++++
>  modules/demux/mkv/matroska_segment.cpp       | 22 +++++++++++++++-
>  modules/demux/mkv/matroska_segment_parse.cpp | 36 +++++++++++++++++++++++++
>  modules/demux/mkv/mkv.cpp                    | 39 +++++++++++++++++++---------
>  modules/demux/mkv/mkv.hpp                    |  6 +++--
>  modules/demux/xiph.h                         |  2 +-
>  6 files changed, 94 insertions(+), 16 deletions(-)
> 
> diff --git a/modules/demux/mkv/matroska_segment.cpp b/modules/demux/mkv/matroska_segment.cpp
> index 756a9c5..ff7b9ba 100644
> --- a/modules/demux/mkv/matroska_segment.cpp
> +++ b/modules/demux/mkv/matroska_segment.cpp
> @@ -929,10 +929,22 @@ void matroska_segment_c::Seek( mtime_t i_date, mtime_t i_time_offset, int64_t i_
>      /* now parse until key frame */
>      const int es_types[3] = { VIDEO_ES, AUDIO_ES, SPU_ES };
>      i_cat = es_types[0];
> +    mtime_t i_seek_preroll = 0;
>      for( int i = 0; i < 2; i_cat = es_types[++i] )
>      {
>          for( i_track = 0; i_track < tracks.size(); i_track++ )
>          {
> +            if( tracks[i_track]->i_seek_preroll )
> +            {
> +                bool b_enabled;
> +                if( es_out_Control( sys.demuxer.out,
> +                                    ES_OUT_GET_ES_STATE,
> +                                    tracks[i_track]->p_es,
> +                                    &b_enabled ) == VLC_SUCCESS &&
> +                    b_enabled )
> +                    i_seek_preroll = ( i_seek_preroll > tracks[i_track]->i_seek_preroll ) ?
> +                        i_seek_preroll : tracks[i_track]->i_seek_preroll;

if (i_seek_preroll < tracks[i_track]->i_seek_preroll)
    i_seek_preroll = tracks[i_track]->i_seek_preroll;

is simpler to read than ternary

> +            }
>              if( tracks[i_track]->fmt.i_cat == i_cat )
>              {
>                  spoint * seekpoint = new spoint(i_track, i_seek_time, i_seek_position, i_seek_position);

> @@ -1477,6 +1493,26 @@ int32_t matroska_segment_c::TrackInit( mkv_track_t * p_tk )
>          p_tk->fmt.i_codec = VLC_CODEC_VORBIS;
>          fill_extra_data( p_tk, 0 );
>      }
> +    else if( !strncmp( p_tk->psz_codec, "A_OPUS", 6 ) )
> +    {
> +        p_tk->fmt.i_codec = VLC_CODEC_OPUS;
> +        if( !p_tk->fmt.audio.i_rate )
> +        {
> +            msg_Err( &sys.demuxer,"No sampling rate, defaulting to 48kHz");
> +            p_tk->fmt.audio.i_rate = 48000;
> +        }
> +        const uint8_t tags[16] = {'O','p','u','s','T','a','g','s',
> +                                   0, 0, 0, 0, 0, 0, 0, 0};
> +        unsigned ps[2] = { p_tk->i_extra_data, 16 };
> +        const void *pkt[2] = { (const void *)p_tk->p_extra_data,
> +                              (const void *) tags };
> +
> +        if( xiph_PackHeaders( (int *)&p_tk->fmt.i_extra,
> +                              (void **) &p_tk->fmt.p_extra,

Why do you need a cast to (int*) and (void**) ?

> +                              ps, pkt, 2 ) )
> +            msg_Err( &sys.demuxer, "Couldn't pack OPUS headers");
> +
> +    }
>      else if( !strncmp( p_tk->psz_codec, "A_AAC/MPEG2/", strlen( "A_AAC/MPEG2/" ) ) ||
>               !strncmp( p_tk->psz_codec, "A_AAC/MPEG4/", strlen( "A_AAC/MPEG4/" ) ) )
>      {