[vlc-devel] [PATCH] opus: add opus encoder

[Tristan Matthews]

Hi,

On Tue, Sep 17, 2013 at 6:07 AM, Jean-Baptiste Kempf <jb at videolan.org>wrote:

> On 16 Sep, Tristan Matthews wrote :
> > diff --git a/modules/codec/avcodec/fourcc.c
> b/modules/codec/avcodec/fourcc.c
> > index 3746ac8..487966b 100644
> > --- a/modules/codec/avcodec/fourcc.c
> > +++ b/modules/codec/avcodec/fourcc.c
> > @@ -311,6 +311,8 @@ static const struct
> >
> >      { VLC_CODEC_DVAUDIO, AV_CODEC_ID_DVAUDIO, AUDIO_ES },
> >
> > +    { VLC_CODEC_OPUS, AV_CODEC_ID_OPUS, AUDIO_ES },
> > +
> >      { VLC_CODEC_MACE3, AV_CODEC_ID_MACE3, AUDIO_ES },
> >      { VLC_CODEC_MACE6, AV_CODEC_ID_MACE6, AUDIO_ES },
>
> This should be a in a separated commit.
>

OK.


>
> > +#ifdef ENABLE_SOUT
> > +
> > +// only ever encode 20 ms at a time
>
> Why?
>

I'll change that comment to be more informative, but basically you can only
encode multiples of 2.5ms at a time with opus (other sizes are rejected),
and 20ms is a sane default according to Tim. I'll try and find a more
concrete answer for the comment.

(You should prefer C comments, tbh)
>
> > +        // TODO: integer pcm
>
> Comment is a bit confusing
>
> > +    enc->fmt_in.i_codec = VLC_CODEC_FL32; // ?
>
> Why "?" ?
>

If it's ok that we only ever encode float32 then I can remove the previous
two comments.


>
> > +#define readint(buf, base) (((buf[base+3]<<24)&0xff000000)| \
> > +                           ((buf[base+2]<<16)&0xff0000)| \
> > +                           ((buf[base+1]<<8)&0xff00)| \
> > +                           (buf[base]&0xff))
>
> Is there any reason this is not a static inline?
>
> > +#define writeint(buf, base, val) do{ buf[base+3]=((val)>>24)&0xff; \
> > +                                     buf[base+2]=((val)>>16)&0xff; \
> > +                                     buf[base+1]=((val)>>8)&0xff; \
> > +                                     buf[base]=(val)&0xff; \
> > +                                 }while(0)
> > +#define writeshort(buf, base, val) do{ buf[base+1]=((val)>>8)&0xff; \
> > +                                       buf[base]=(val)&0xff; \
> > +                                 }while(0)
>
> idem?
>

No good reason, they are only ever called in for header writing when the
encoder is open. I'll fix that.


>
> > +    if (comment_add(&comments, &comments_length, "ENCODER=", "opus
> encoder from VLC media player"))
> Please no.
>

Should it just be libopus then?


>
> > diff --git a/modules/mux/ogg.c b/modules/mux/ogg.c
> > index 99252b0..a805996 100644
> > --- a/modules/mux/ogg.c
> > +++ b/modules/mux/ogg.c
> > @@ -361,6 +361,10 @@ static int AddStream( sout_mux_t *p_mux,
> sout_input_t *p_input )
> >      case AUDIO_ES:
> >          switch( p_stream->i_fourcc )
> >          {
> > +        case VLC_CODEC_OPUS:
> > +            msg_Dbg( p_mux, "opus stream" );
> > +            break;
> > +
> >          case VLC_CODEC_VORBIS:
> >              msg_Dbg( p_mux, "vorbis stream" );
> >              break;
> > @@ -626,6 +630,7 @@ static block_t *OggCreateHeader( sout_mux_t *p_mux )
> >
> >              if( p_stream->i_fourcc == VLC_CODEC_VORBIS ||
> >                  p_stream->i_fourcc == VLC_CODEC_SPEEX ||
> > +                p_stream->i_fourcc == VLC_CODEC_OPUS ||
> >                  p_stream->i_fourcc == VLC_CODEC_THEORA )
> >              {
> >                  /* First packet in order: vorbis/speex/theora info */
> > @@ -713,6 +718,7 @@ static block_t *OggCreateHeader( sout_mux_t *p_mux )
> >
> >          if( p_stream->i_fourcc == VLC_CODEC_VORBIS ||
> >              p_stream->i_fourcc == VLC_CODEC_SPEEX ||
> > +            p_stream->i_fourcc == VLC_CODEC_OPUS ||
> >              p_stream->i_fourcc == VLC_CODEC_THEORA )
> >          {
> >              unsigned pi_size[XIPH_MAX_HEADER_COUNT];
> > @@ -977,6 +983,7 @@ static int MuxBlock( sout_mux_t *p_mux, sout_input_t
> *p_input )
> >      if( p_stream->i_fourcc != VLC_CODEC_VORBIS &&
> >          p_stream->i_fourcc != VLC_CODEC_FLAC &&
> >          p_stream->i_fourcc != VLC_CODEC_SPEEX &&
> > +        p_stream->i_fourcc != VLC_CODEC_OPUS &&
> >          p_stream->i_fourcc != VLC_CODEC_THEORA &&
> >          p_stream->i_fourcc != VLC_CODEC_DIRAC )
> >      {
> > @@ -994,6 +1001,7 @@ static int MuxBlock( sout_mux_t *p_mux,
> sout_input_t *p_input )
> >      {
> >          if( p_stream->i_fourcc == VLC_CODEC_VORBIS ||
> >              p_stream->i_fourcc == VLC_CODEC_FLAC ||
> > +            p_stream->i_fourcc == VLC_CODEC_OPUS ||
> >              p_stream->i_fourcc == VLC_CODEC_SPEEX )
> >          {
> >              /* number of sample from begining + current packet */
>
>
Thanks for the feedback.

Best,
Tristan


-- 
Tristan Matthews
web: http://tristanswork.blogspot.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20130917/616fe748/attachment.html>